diff mbox series

[02/15] debug: Increase tst-fortify checks for compiler without __va_arg_pack support

Message ID 20231221185929.1307116-3-adhemerval.zanella@linaro.org
State Accepted
Commit bf320000b47ce46aa6dbe1b7068e6539bf2df9bb
Headers show
Series Improve fortify support with clang | expand

Commit Message

Adhemerval Zanella Dec. 21, 2023, 6:59 p.m. UTC 
The fortify wrappers for varargs functions already add fallbacks to
builtins calls if __va_arg_pack is not supported.

Checked on aarch64, armhf, x86_64, and i686.
---
 debug/tst-fortify.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Siddhesh Poyarekar Dec. 21, 2023, 8:02 p.m. UTC | #1 
On 2023-12-21 13:59, Adhemerval Zanella wrote:
> The fortify wrappers for varargs functions already add fallbacks to
> builtins calls if __va_arg_pack is not supported.

... and in fact helps test the #else part with a different compiler, 
like clang.  BTW, I'm not sure if you've seen this, but Serge Guelton 
used to maintain a _FORTIFY_SOURCE testsuite to do comparative testing 
between clang and gcc:

https://github.com/serge-sans-paille/fortify-test-suite

It would be nice to subsume all of that, if there's additional coverage 
there.

LGTM.

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

> 
> Checked on aarch64, armhf, x86_64, and i686.
> ---
>   debug/tst-fortify.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c
> index 20e926751a..5cd9d22feb 100644
> --- a/debug/tst-fortify.c
> +++ b/debug/tst-fortify.c
> @@ -130,7 +130,7 @@ static int num2 = 987654;
>         chk_fail_ok = 0;				\
>         FAIL ();					\
>       }
> -#if __USE_FORTIFY_LEVEL >= 2 && (!defined __cplusplus || defined __va_arg_pack)
> +#if __USE_FORTIFY_LEVEL >= 2
>   # define CHK_FAIL2_START CHK_FAIL_START
>   # define CHK_FAIL2_END CHK_FAIL_END
>   #else
> @@ -419,7 +419,6 @@ do_test (void)
>     stpncpy (buf + 6, "cd", l0 + 5);
>     CHK_FAIL_END
>   
> -# if !defined __cplusplus || defined __va_arg_pack
>     CHK_FAIL_START
>     sprintf (buf + 8, "%d", num1);
>     CHK_FAIL_END
> @@ -439,7 +438,6 @@ do_test (void)
>     CHK_FAIL_START
>     swprintf (wbuf + 8, l0 + 3, L"%d", num1);
>     CHK_FAIL_END
> -# endif
>   
>     memcpy (buf, str1 + 2, 9);
>     CHK_FAIL_START
> @@ -550,7 +548,6 @@ do_test (void)
>         FAIL ();
>     }
>   
> -# if !defined __cplusplus || defined __va_arg_pack
>     CHK_FAIL_START
>     sprintf (a.buf1 + (O + 7), "%d", num1);
>     CHK_FAIL_END
> @@ -562,7 +559,6 @@ do_test (void)
>     CHK_FAIL_START
>     snprintf (a.buf1 + (O + 7), l0 + 3, "%d", num2);
>     CHK_FAIL_END
> -# endif
>   
>     memcpy (a.buf1, str1 + (3 - O), 8 + O);
>     CHK_FAIL_START
Joseph Myers Dec. 21, 2023, 9:43 p.m. UTC | #2 
On Thu, 21 Dec 2023, Siddhesh Poyarekar wrote:

> On 2023-12-21 13:59, Adhemerval Zanella wrote:
> > The fortify wrappers for varargs functions already add fallbacks to
> > builtins calls if __va_arg_pack is not supported.
> 
> ... and in fact helps test the #else part with a different compiler, like
> clang.  BTW, I'm not sure if you've seen this, but Serge Guelton used to
> maintain a _FORTIFY_SOURCE testsuite to do comparative testing between clang
> and gcc:
> 
> https://github.com/serge-sans-paille/fortify-test-suite
> 
> It would be nice to subsume all of that, if there's additional coverage there.

A related (hard) issue is how to run the glibc testsuite with a different 
compiler (whether in the glibc build tree, or, introducing a further 
issue, against an installed copy of glibc) - as one built copy of glibc 
ought to work with multiple compilers, including ones that aren't 
supported for building glibc itself.  (In principle such testing could 
detect ABI incompatibilities as well as header issues; consider e.g. the 
known issues where clang is incompatible with the x86-64 ABI's handling of 
narrower-than-32-bit function arguments and return values, though it's 
quite likely tests would fail to exercise such incompatibilities.)
Adhemerval Zanella Dec. 22, 2023, 12:29 p.m. UTC | #3 
On 21/12/23 17:02, Siddhesh Poyarekar wrote:
> On 2023-12-21 13:59, Adhemerval Zanella wrote:
>> The fortify wrappers for varargs functions already add fallbacks to
>> builtins calls if __va_arg_pack is not supported.
> 
> ... and in fact helps test the #else part with a different compiler, like clang.  BTW, I'm not sure if you've seen this, but Serge Guelton used to maintain a _FORTIFY_SOURCE testsuite to do comparative testing between clang and gcc:
> 
> https://github.com/serge-sans-paille/fortify-test-suite
> 
> It would be nice to subsume all of that, if there's additional coverage there.

Interesting, I will take a look.  Our tests are still lacking a way to
proper check wrong usages that might trigger compiler warnings/error
(either through the builtins and/or wrapper).

But as I put on cover-letter, I actually tested it by using using on top
of my WIP branch that aims to enable clang build glibc. 

> 
> LGTM.
> 
> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 
>>
>> Checked on aarch64, armhf, x86_64, and i686.
>> ---
>>   debug/tst-fortify.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c
>> index 20e926751a..5cd9d22feb 100644
>> --- a/debug/tst-fortify.c
>> +++ b/debug/tst-fortify.c
>> @@ -130,7 +130,7 @@ static int num2 = 987654;
>>         chk_fail_ok = 0;                \
>>         FAIL ();                    \
>>       }
>> -#if __USE_FORTIFY_LEVEL >= 2 && (!defined __cplusplus || defined __va_arg_pack)
>> +#if __USE_FORTIFY_LEVEL >= 2
>>   # define CHK_FAIL2_START CHK_FAIL_START
>>   # define CHK_FAIL2_END CHK_FAIL_END
>>   #else
>> @@ -419,7 +419,6 @@ do_test (void)
>>     stpncpy (buf + 6, "cd", l0 + 5);
>>     CHK_FAIL_END
>>   -# if !defined __cplusplus || defined __va_arg_pack
>>     CHK_FAIL_START
>>     sprintf (buf + 8, "%d", num1);
>>     CHK_FAIL_END
>> @@ -439,7 +438,6 @@ do_test (void)
>>     CHK_FAIL_START
>>     swprintf (wbuf + 8, l0 + 3, L"%d", num1);
>>     CHK_FAIL_END
>> -# endif
>>       memcpy (buf, str1 + 2, 9);
>>     CHK_FAIL_START
>> @@ -550,7 +548,6 @@ do_test (void)
>>         FAIL ();
>>     }
>>   -# if !defined __cplusplus || defined __va_arg_pack
>>     CHK_FAIL_START
>>     sprintf (a.buf1 + (O + 7), "%d", num1);
>>     CHK_FAIL_END
>> @@ -562,7 +559,6 @@ do_test (void)
>>     CHK_FAIL_START
>>     snprintf (a.buf1 + (O + 7), l0 + 3, "%d", num2);
>>     CHK_FAIL_END
>> -# endif
>>       memcpy (a.buf1, str1 + (3 - O), 8 + O);
>>     CHK_FAIL_START
diff mbox series

Patch

diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c
index 20e926751a..5cd9d22feb 100644
--- a/debug/tst-fortify.c
+++ b/debug/tst-fortify.c
@@ -130,7 +130,7 @@  static int num2 = 987654;
       chk_fail_ok = 0;				\
       FAIL ();					\
     }
-#if __USE_FORTIFY_LEVEL >= 2 && (!defined __cplusplus || defined __va_arg_pack)
+#if __USE_FORTIFY_LEVEL >= 2
 # define CHK_FAIL2_START CHK_FAIL_START
 # define CHK_FAIL2_END CHK_FAIL_END
 #else
@@ -419,7 +419,6 @@  do_test (void)
   stpncpy (buf + 6, "cd", l0 + 5);
   CHK_FAIL_END
 
-# if !defined __cplusplus || defined __va_arg_pack
   CHK_FAIL_START
   sprintf (buf + 8, "%d", num1);
   CHK_FAIL_END
@@ -439,7 +438,6 @@  do_test (void)
   CHK_FAIL_START
   swprintf (wbuf + 8, l0 + 3, L"%d", num1);
   CHK_FAIL_END
-# endif
 
   memcpy (buf, str1 + 2, 9);
   CHK_FAIL_START
@@ -550,7 +548,6 @@  do_test (void)
       FAIL ();
   }
 
-# if !defined __cplusplus || defined __va_arg_pack
   CHK_FAIL_START
   sprintf (a.buf1 + (O + 7), "%d", num1);
   CHK_FAIL_END
@@ -562,7 +559,6 @@  do_test (void)
   CHK_FAIL_START
   snprintf (a.buf1 + (O + 7), l0 + 3, "%d", num2);
   CHK_FAIL_END
-# endif
 
   memcpy (a.buf1, str1 + (3 - O), 8 + O);
   CHK_FAIL_START