features: version-gate _DYNAMIC_STACK_SIZE_SOURCE

Message ID 20220129023727.1496360-1-andrew@ziglang.org
State Rejected
Headers
Series features: version-gate _DYNAMIC_STACK_SIZE_SOURCE |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Andrew Kelley Jan. 29, 2022, 2:37 a.m. UTC
  Without this patch, software compiled against glibc 2.34 headers but
then executed on a system with an older glibc version will attempt to
invoke `sysconf(_SC_SIGSTKSZ)` when MINSIGSTKSZ or SIGSTKSZ are used. On
glibcs older than 2.34, this will return -1 (with an errno of EINVAL),
effectively causing MINSIGSTKSZ and SIGSTKSZ to have the value of -1.

This patch version-gates the _DYNAMIC_STACK_SIZE_SOURCE preprocessor
definition so that this problem cannot occur.

Downstream patch:
https://github.com/ziglang/zig/commit/39083c31a550ed80f369f60d35791e98904b8096
---
 include/features.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
  

Comments

H.J. Lu Jan. 29, 2022, 3 a.m. UTC | #1
On Fri, Jan 28, 2022 at 6:38 PM Andrew Kelley <andrew@ziglang.org> wrote:
>
> Without this patch, software compiled against glibc 2.34 headers but
> then executed on a system with an older glibc version will attempt to
> invoke `sysconf(_SC_SIGSTKSZ)` when MINSIGSTKSZ or SIGSTKSZ are used. On
> glibcs older than 2.34, this will return -1 (with an errno of EINVAL),
> effectively causing MINSIGSTKSZ and SIGSTKSZ to have the value of -1.
>
> This patch version-gates the _DYNAMIC_STACK_SIZE_SOURCE preprocessor
> definition so that this problem cannot occur.
>
> Downstream patch:
> https://github.com/ziglang/zig/commit/39083c31a550ed80f369f60d35791e98904b8096
> ---
>  include/features.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/features.h b/include/features.h
> index ab2b2caac4..77a8f8cc32 100644
> --- a/include/features.h
> +++ b/include/features.h
> @@ -220,8 +220,10 @@
>  # define _DEFAULT_SOURCE       1
>  # undef  _ATFILE_SOURCE
>  # define _ATFILE_SOURCE        1
> -# undef  _DYNAMIC_STACK_SIZE_SOURCE
> -# define _DYNAMIC_STACK_SIZE_SOURCE 1
> +# if __GNUC_PREREQ (2,34)
> +#  undef  _DYNAMIC_STACK_SIZE_SOURCE
> +#  define _DYNAMIC_STACK_SIZE_SOURCE 1

You are changing a glibc 2.35 header file.   Isn't __GNUC_PREREQ (2,34)
always true? Am I missing something?

> +# endif
>  #endif
>
>  /* If nothing (other than _GNU_SOURCE and _DEFAULT_SOURCE) is defined,
> --
> 2.33.1
>
  
Andrew Kelley Jan. 29, 2022, 4:04 a.m. UTC | #2
On 1/28/22 20:00, H.J. Lu wrote:
> On Fri, Jan 28, 2022 at 6:38 PM Andrew Kelley <andrew@ziglang.org> wrote:
>>
>> Without this patch, software compiled against glibc 2.34 headers but
>> then executed on a system with an older glibc version will attempt to
>> invoke `sysconf(_SC_SIGSTKSZ)` when MINSIGSTKSZ or SIGSTKSZ are used. On
>> glibcs older than 2.34, this will return -1 (with an errno of EINVAL),
>> effectively causing MINSIGSTKSZ and SIGSTKSZ to have the value of -1.
>>
>> This patch version-gates the _DYNAMIC_STACK_SIZE_SOURCE preprocessor
>> definition so that this problem cannot occur.
>>
>> Downstream patch:
>> https://github.com/ziglang/zig/commit/39083c31a550ed80f369f60d35791e98904b8096
>> ---
>>   include/features.h | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/features.h b/include/features.h
>> index ab2b2caac4..77a8f8cc32 100644
>> --- a/include/features.h
>> +++ b/include/features.h
>> @@ -220,8 +220,10 @@
>>   # define _DEFAULT_SOURCE       1
>>   # undef  _ATFILE_SOURCE
>>   # define _ATFILE_SOURCE        1
>> -# undef  _DYNAMIC_STACK_SIZE_SOURCE
>> -# define _DYNAMIC_STACK_SIZE_SOURCE 1
>> +# if __GNUC_PREREQ (2,34)
>> +#  undef  _DYNAMIC_STACK_SIZE_SOURCE
>> +#  define _DYNAMIC_STACK_SIZE_SOURCE 1
> 
> You are changing a glibc 2.35 header file.   Isn't __GNUC_PREREQ (2,34)
> always true? Am I missing something?

You are correct.

Downstream we also have a patch that removes the __GLIBC_MINOR__ 
preprocessor definition. Instead, we pass it on the command line.

https://github.com/ziglang/zig/commit/4d48948b526337947ef59a83f7dbc81b70aa5723

The Zig project aims to support targeting many versions of glibc; not 
only the latest one. Our strategy is multi-faceted. For the shared 
objects, there is this project:
https://github.com/ziglang/glibc-abi-tool/

And then we have the headers. For the most part, the latest glibc 
headers are still correct for targeting systems with older glibc 
versions. Some of the other usages of __GNUC_PREREQ look to be 
supporting this use case to me. An occasional patch such as this one is 
needed to make it work correctly, however.

I can understand there would be hesitation to support such a use case. I 
hope that glibc maintainers would consider it however, because 
ultimately it is helping glibc users cross-compile, targeting their 
production systems from their development machines.

Thanks for your time.

> 
>> +# endif
>>   #endif
>>
>>   /* If nothing (other than _GNU_SOURCE and _DEFAULT_SOURCE) is defined,
>> --
>> 2.33.1
>>
> 
>
  
Andrew Kelley Jan. 29, 2022, 4:15 a.m. UTC | #3
On 1/28/22 21:04, Andrew Kelley wrote:
> On 1/28/22 20:00, H.J. Lu wrote:
>> On Fri, Jan 28, 2022 at 6:38 PM Andrew Kelley <andrew@ziglang.org> wrote:
>>>
>>> Without this patch, software compiled against glibc 2.34 headers but
>>> then executed on a system with an older glibc version will attempt to
>>> invoke `sysconf(_SC_SIGSTKSZ)` when MINSIGSTKSZ or SIGSTKSZ are used. On
>>> glibcs older than 2.34, this will return -1 (with an errno of EINVAL),
>>> effectively causing MINSIGSTKSZ and SIGSTKSZ to have the value of -1.
>>>
>>> This patch version-gates the _DYNAMIC_STACK_SIZE_SOURCE preprocessor
>>> definition so that this problem cannot occur.
>>>
>>> Downstream patch:
>>> https://github.com/ziglang/zig/commit/39083c31a550ed80f369f60d35791e98904b8096 
>>>
>>> ---
>>>   include/features.h | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/features.h b/include/features.h
>>> index ab2b2caac4..77a8f8cc32 100644
>>> --- a/include/features.h
>>> +++ b/include/features.h
>>> @@ -220,8 +220,10 @@
>>>   # define _DEFAULT_SOURCE       1
>>>   # undef  _ATFILE_SOURCE
>>>   # define _ATFILE_SOURCE        1
>>> -# undef  _DYNAMIC_STACK_SIZE_SOURCE
>>> -# define _DYNAMIC_STACK_SIZE_SOURCE 1
>>> +# if __GNUC_PREREQ (2,34)
>>> +#  undef  _DYNAMIC_STACK_SIZE_SOURCE
>>> +#  define _DYNAMIC_STACK_SIZE_SOURCE 1
>>
>> You are changing a glibc 2.35 header file.   Isn't __GNUC_PREREQ (2,34)
>> always true? Am I missing something?
> 
> You are correct.
> 
> Downstream we also have a patch that removes the __GLIBC_MINOR__ 
> preprocessor definition. Instead, we pass it on the command line.
> 
> https://github.com/ziglang/zig/commit/4d48948b526337947ef59a83f7dbc81b70aa5723 
> 
> 
> The Zig project aims to support targeting many versions of glibc; not 
> only the latest one. Our strategy is multi-faceted. For the shared 
> objects, there is this project:
> https://github.com/ziglang/glibc-abi-tool/
> 
> And then we have the headers. For the most part, the latest glibc 
> headers are still correct for targeting systems with older glibc 
> versions. Some of the other usages of __GNUC_PREREQ look to be 
> supporting this use case to me. An occasional patch such as this one is 
> needed to make it work correctly, however.
> 
> I can understand there would be hesitation to support such a use case. I 
> hope that glibc maintainers would consider it however, because 
> ultimately it is helping glibc users cross-compile, targeting their 
> production systems from their development machines.
> 
> Thanks for your time.


I just noticed that, while the downstream patch that I linked has the 
correct improvement, I bungled the upstream patch here because I 
misunderstood what __GNUC_PREREQ does. This is the actual patch that I 
am proposing:

--- a/include/features.h
+++ b/include/features.h
@@ -220,8 +220,10 @@
  # define _DEFAULT_SOURCE       1
  # undef  _ATFILE_SOURCE
  # define _ATFILE_SOURCE        1
-# undef  _DYNAMIC_STACK_SIZE_SOURCE
-# define _DYNAMIC_STACK_SIZE_SOURCE 1
+# if (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 34) || __GLIBC__ > 2
+#  undef  _DYNAMIC_STACK_SIZE_SOURCE
+#  define _DYNAMIC_STACK_SIZE_SOURCE 1
+# endif
  #endif

  /* If nothing (other than _GNU_SOURCE and _DEFAULT_SOURCE) is defined,
  
Szabolcs Nagy Jan. 29, 2022, 10:41 a.m. UTC | #4
* Andrew Kelley <andrew@ziglang.org> [2022-01-28 21:15:36 -0700]:
> On 1/28/22 21:04, Andrew Kelley wrote:
> > Downstream we also have a patch that removes the __GLIBC_MINOR__
> > preprocessor definition. Instead, we pass it on the command line.
> > 
> > https://github.com/ziglang/zig/commit/4d48948b526337947ef59a83f7dbc81b70aa5723
> > 
> > 
> > The Zig project aims to support targeting many versions of glibc; not
> > only the latest one. Our strategy is multi-faceted. For the shared
> > objects, there is this project:
> > https://github.com/ziglang/glibc-abi-tool/
> > 
> > And then we have the headers. For the most part, the latest glibc
> > headers are still correct for targeting systems with older glibc

new headers may rely on new symbols, behaviours and abi that
did not exist in an old glibc.

the only way multiple glibc versions are supported at runtime
is to compile against old glibc and run the binary against a
newer one. not the other way around.

and you cannot detect at compile time what will be the glibc
version at runtime, so version checking does not help.
what problem are you solving by passing __GLIBC_MINOR__ at
compile time?
  
Florian Weimer Feb. 1, 2022, 4:37 p.m. UTC | #5
* Andrew Kelley:

> The Zig project aims to support targeting many versions of glibc; not
> only the latest one. Our strategy is multi-faceted. For the shared 
> objects, there is this project:
> https://github.com/ziglang/glibc-abi-tool/
>
> And then we have the headers. For the most part, the latest glibc
> headers are still correct for targeting systems with older glibc 
> versions. Some of the other usages of __GNUC_PREREQ look to be
> supporting this use case to me. An occasional patch such as this one
> is needed to make it work correctly, however.

__GNUC_PREREQ certainly does not work for this, it's for GCC version
checks.

Why do you need *C* header files?

Thanks,
Florian
  

Patch

diff --git a/include/features.h b/include/features.h
index ab2b2caac4..77a8f8cc32 100644
--- a/include/features.h
+++ b/include/features.h
@@ -220,8 +220,10 @@ 
 # define _DEFAULT_SOURCE	1
 # undef  _ATFILE_SOURCE
 # define _ATFILE_SOURCE	1
-# undef  _DYNAMIC_STACK_SIZE_SOURCE
-# define _DYNAMIC_STACK_SIZE_SOURCE 1
+# if __GNUC_PREREQ (2,34)
+#  undef  _DYNAMIC_STACK_SIZE_SOURCE
+#  define _DYNAMIC_STACK_SIZE_SOURCE 1
+# endif
 #endif
 
 /* If nothing (other than _GNU_SOURCE and _DEFAULT_SOURCE) is defined,