stdio-common: tests: don't double-define _FORTIFY_SOURCE

Message ID 20230221092726.420571-1-sam@gentoo.org
State Committed
Commit ecf8ae6704d5034fc2d5e29e5dc88dbca981581e
Headers
Series stdio-common: tests: don't double-define _FORTIFY_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

Sam James Feb. 21, 2023, 9:27 a.m. UTC
  Exactly the same as 35bcb08eaa953c9b8bef6ab2486dc4361e1f26c0.

If using -D_FORITFY_SOURCE=3 (in my case, I've patched GCC to add
=3 instead of =2 (we've done =2 for years in Gentoo)), building
glibc tests will fail on tst-bz11319-fortify2 like:
```
<command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror]
<built-in>: note: this is the location of the previous definition
cc1: all warnings being treated as errors
```

It's just because we're always setting -D_FORTIFY_SOURCE=2
rather than unsetting it first. If F_S is already 2, it's harmless,
but if it's another value (say, 1, or 3), the compiler will bawk.

(I'm not aware of a reason this couldn't be tested with =3,
but the toolchain support is limited for that (too new), and we want
to run the tests everywhere possible.)

As Siddhesh noted previously, we could implement some fallback
logic to determine the maximal F_S value supported by the toolchain,
which is a bit easier now that autoconf-archive has been updated for F_S=3
(https://github.com/autoconf-archive/autoconf-archive/pull/269), but let's
revisit this if it continues to crop up.

Signed-off-by: Sam James <sam@gentoo.org>
---
 stdio-common/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Siddhesh Poyarekar March 27, 2023, 1:11 p.m. UTC | #1
On 2023-02-21 04:27, Sam James via Libc-alpha wrote:
> Exactly the same as 35bcb08eaa953c9b8bef6ab2486dc4361e1f26c0.
> 
> If using -D_FORITFY_SOURCE=3 (in my case, I've patched GCC to add
> =3 instead of =2 (we've done =2 for years in Gentoo)), building
> glibc tests will fail on tst-bz11319-fortify2 like:
> ```
> <command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror]
> <built-in>: note: this is the location of the previous definition
> cc1: all warnings being treated as errors
> ```
> 
> It's just because we're always setting -D_FORTIFY_SOURCE=2
> rather than unsetting it first. If F_S is already 2, it's harmless,
> but if it's another value (say, 1, or 3), the compiler will bawk.
> 
> (I'm not aware of a reason this couldn't be tested with =3,
> but the toolchain support is limited for that (too new), and we want
> to run the tests everywhere possible.)
> 
> As Siddhesh noted previously, we could implement some fallback
> logic to determine the maximal F_S value supported by the toolchain,
> which is a bit easier now that autoconf-archive has been updated for F_S=3
> (https://github.com/autoconf-archive/autoconf-archive/pull/269), but let's
> revisit this if it continues to crop up.
> 
> Signed-off-by: Sam James <sam@gentoo.org>
> ---
>   stdio-common/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

Sam, if you're going to post patches regularly enough, please consider 
asking for write access.  You could add me as a sponsor.

https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Thanks,
Sid

> 
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 92a3499a94..abb398fdb7 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -436,7 +436,7 @@ CFLAGS-tst-gets.c += -Wno-deprecated-declarations
>   
>   # BZ #11319 was first fixed for regular vdprintf, then reopened because
>   # the fortified version had the same bug.
> -CFLAGS-tst-bz11319-fortify2.c += -D_FORTIFY_SOURCE=2
> +CFLAGS-tst-bz11319-fortify2.c += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
>   
>   CFLAGS-tst-memstream-string.c += -fno-builtin-fprintf
>
  
Sam James March 31, 2023, 6:38 p.m. UTC | #2
Siddhesh Poyarekar <siddhesh@gotplt.org> writes:

> On 2023-02-21 04:27, Sam James via Libc-alpha wrote:
>> Exactly the same as 35bcb08eaa953c9b8bef6ab2486dc4361e1f26c0.
>> If using -D_FORITFY_SOURCE=3 (in my case, I've patched GCC to add
>> =3 instead of =2 (we've done =2 for years in Gentoo)), building
>> glibc tests will fail on tst-bz11319-fortify2 like:
>> ```
>> <command-line>: error: "_FORTIFY_SOURCE" redefined [-Werror]
>> <built-in>: note: this is the location of the previous definition
>> cc1: all warnings being treated as errors
>> ```
>> It's just because we're always setting -D_FORTIFY_SOURCE=2
>> rather than unsetting it first. If F_S is already 2, it's harmless,
>> but if it's another value (say, 1, or 3), the compiler will bawk.
>> (I'm not aware of a reason this couldn't be tested with =3,
>> but the toolchain support is limited for that (too new), and we want
>> to run the tests everywhere possible.)
>> As Siddhesh noted previously, we could implement some fallback
>> logic to determine the maximal F_S value supported by the toolchain,
>> which is a bit easier now that autoconf-archive has been updated for F_S=3
>> (https://github.com/autoconf-archive/autoconf-archive/pull/269), but let's
>> revisit this if it continues to crop up.
>> Signed-off-by: Sam James <sam@gentoo.org>
>> ---
>>   stdio-common/Makefile | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>
> Sam, if you're going to post patches regularly enough, please consider
> asking for write access.  You could add me as a sponsor.
>
> https://sourceware.org/cgi-bin/pdw/ps_form.cgi

Thanks Sid, very kind of you & much appreciated. All set up now!

\o/

>
> Thanks,
> Sid
>
best,
sam

>> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
>> index 92a3499a94..abb398fdb7 100644
>> --- a/stdio-common/Makefile
>> +++ b/stdio-common/Makefile
>> @@ -436,7 +436,7 @@ CFLAGS-tst-gets.c += -Wno-deprecated-declarations
>>     # BZ #11319 was first fixed for regular vdprintf, then reopened
>> because
>>   # the fortified version had the same bug.
>> -CFLAGS-tst-bz11319-fortify2.c += -D_FORTIFY_SOURCE=2
>> +CFLAGS-tst-bz11319-fortify2.c += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
>>     CFLAGS-tst-memstream-string.c += -fno-builtin-fprintf
>>
  

Patch

diff --git a/stdio-common/Makefile b/stdio-common/Makefile
index 92a3499a94..abb398fdb7 100644
--- a/stdio-common/Makefile
+++ b/stdio-common/Makefile
@@ -436,7 +436,7 @@  CFLAGS-tst-gets.c += -Wno-deprecated-declarations
 
 # BZ #11319 was first fixed for regular vdprintf, then reopened because
 # the fortified version had the same bug.
-CFLAGS-tst-bz11319-fortify2.c += -D_FORTIFY_SOURCE=2
+CFLAGS-tst-bz11319-fortify2.c += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
 
 CFLAGS-tst-memstream-string.c += -fno-builtin-fprintf