| Message ID | A155DA90-29E8-4BA9-9C6D-ECDEEE7B55D5@adacore.com |
|---|---|
| State | New |
| Headers | show |
| Series | gcc.dg/analyzer tests: relax dependency on alloca.h | expand |
Hello, Ping for https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557548.html (copied below for convenience), please ? Thanks in advance! With Kind Regards, Olivier > On 29 Oct 2020, at 21:45, Olivier Hainque <hainque@adacore.com> wrote: > > Hello, > > Some of the tests in gcc.dg/analyzer exercize alloca > and #include <alloca.h>. > > Some systems, e.g. VxWorks, don't feature alloca.h and > the aforementioned tests fail. > > Other tests in the suite have been in this situation and > the usual way around consists in resorting to __builtin_alloca > directly. > > This patch is a proposal in this direction for gcc.dg/analyzer. > > It introduces a common "analyzer-alloca.h" where we can > stick a common comment and a macro to redirect "alloca" > directly to "__builtin_alloca". > > The intermediate macro in a non system header unfortunately > diverts some of the warning expectations, as the allocation > point for "x = alloca(128);" is shown on the macro definition > and not at the macro invocation point. > > The patch circumvents this by calling __builtin_alloca > directly from the points where the tests perform a warning > check. > > I have verified that all the tests adjusted by the change > now pass in a run for a powerpc-vxworks configuration. > > I'll gladly perform an extra regression test on a native > system if the patch is considered ok. > > Would this be ok to commit ? > > Thanks in advance, > > With Kind Regards, > > Olivier > > 2020-10-29 Olivier Hainque <hainque@adacore.com> > > gcc/testsuite/ > * gcc.dg/analyzer/analyzer-alloca.h: New file. > * gcc.dg/analyzer/alloca-leak.c: Use it. > * gcc.dg/analyzer/data-model-1.c: Use it. > * gcc.dg/analyzer/malloc-1.c: Use it and replace call to > be tracked by a direct call to __builtin_alloca. > * gcc.dg/analyzer/malloc-paths-8.c: Likewise. > > <0001-Relax-dependency-on-alloca.h-in-gcc.dg-analyzer-tests.txt> > >
Hello, Olivier, On Dec 18, 2020, Olivier Hainque <hainque@adacore.com> wrote: > Ping for https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557548.html > (copied below for convenience), please ? I think defining alloca as a macro in analyzer-alloca.h might conflict with system headers included before or after analyzer-alloca.h Say, if a system header #defines alloca(n), GCC might warn when encountering a different #define in analyzer-alloca.h, included later. OTOH, if analyzer-alloca.h is included first, then a later system header that attempts to declare alloca as a function could be macro-substituted into declaring __builtin_alloca, which probably wouldn't end well. While the alloca macro would render other changes from alloca to __builtin_alloca unnecessary, there's such an explicit change in the patch. If it were up to me, I'd rather use __builtin_alloca explicitly all over. I think we already do that in several other tests, for similar reasons (that some target systems don't have alloca.h or alloca) Would you mind if I submitted an alternate patch to do so?
Hi Alex, > On 14 Jan 2021, at 22:13, Alexandre Oliva <oliva@adacore.com> wrote: > > Hello, Olivier, > > On Dec 18, 2020, Olivier Hainque <hainque@adacore.com> wrote: > >> Ping for https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557548.html >> (copied below for convenience), please ? > > I think defining alloca as a macro in analyzer-alloca.h might conflict > with system headers included before or after analyzer-alloca.h > > Say, if a system header #defines alloca(n), GCC might warn when > encountering a different #define in analyzer-alloca.h, included later. > > OTOH, if analyzer-alloca.h is included first, then a later system header > that attempts to declare alloca as a function could be macro-substituted > into declaring __builtin_alloca, which probably wouldn't end well. > > While the alloca macro would render other changes from alloca to > __builtin_alloca unnecessary, there's such an explicit change in the > patch. > > If it were up to me, I'd rather use __builtin_alloca explicitly all > over. I think we already do that in several other tests, for similar > reasons (that some target systems don't have alloca.h or alloca) > > Would you mind if I submitted an alternate patch to do so? Not at all, thanks for your feedback and for proposing an alternative! Olivier
On Jan 15, 2021, Olivier Hainque <hainque@adacore.com> wrote: > On 14 Jan 2021, at 22:13, Alexandre Oliva <oliva@adacore.com> wrote: >> Would you mind if I submitted an alternate patch to do so? > Not at all, thanks for your feedback and for proposing > an alternative! Here's the modified patch. Regstrapped on x86_64-linux-gnu, also tested on x-arm-vx7r2. David, I'm leaning towards putting it in as "obvious", barring any objections. gcc.dg/analyzer tests: use __builtin_alloca, not alloca.h From: Alexandre Oliva <oliva@adacore.com> Use __builtin_alloca. Some systems don't have alloca.h or alloca. Co-Authored-By: Olivier Hainque <hainque@adacore.com> for gcc/testsuite/ChangeLog * gcc.dg/analyzer/alloca-leak.c: Drop alloca.h, use builtin. * gcc.dg/analyzer/data-model-1.c: Likewise. * gcc.dg/analyzer/malloc-1.c: Likewise. * gcc.dg/analyzer/malloc-paths-8.c: Likewise. --- gcc/testsuite/gcc.dg/analyzer/alloca-leak.c | 4 +--- gcc/testsuite/gcc.dg/analyzer/data-model-1.c | 5 ++--- gcc/testsuite/gcc.dg/analyzer/malloc-1.c | 3 +-- gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c | 7 +++---- 4 files changed, 7 insertions(+), 12 deletions(-) diff --git a/gcc/testsuite/gcc.dg/analyzer/alloca-leak.c b/gcc/testsuite/gcc.dg/analyzer/alloca-leak.c index 93319932d44ac..073f97e1ade32 100644 --- a/gcc/testsuite/gcc.dg/analyzer/alloca-leak.c +++ b/gcc/testsuite/gcc.dg/analyzer/alloca-leak.c @@ -1,10 +1,8 @@ /* { dg-require-effective-target alloca } */ -#include <alloca.h> - void *test (void) { - void *ptr = alloca (64); + void *ptr = __builtin_alloca (64); return ptr; } /* TODO: warn about escaping alloca. */ diff --git a/gcc/testsuite/gcc.dg/analyzer/data-model-1.c b/gcc/testsuite/gcc.dg/analyzer/data-model-1.c index 3f16a38ab14d4..f6681b678af61 100644 --- a/gcc/testsuite/gcc.dg/analyzer/data-model-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/data-model-1.c @@ -3,7 +3,6 @@ #include <stdlib.h> #include <string.h> #include <stdio.h> -#include <alloca.h> #include "analyzer-decls.h" struct foo @@ -140,8 +139,8 @@ void test_11 (void) void test_12 (void) { - void *p = alloca (256); - void *q = alloca (256); + void *p = __builtin_alloca (256); + void *q = __builtin_alloca (256); /* alloca results should be unique. */ __analyzer_eval (p == q); /* { dg-warning "FALSE" } */ diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c index 26d828848a259..448b8558ffe11 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c @@ -1,6 +1,5 @@ /* { dg-require-effective-target alloca } */ -#include <alloca.h> #include <stdlib.h> extern int foo (void); @@ -273,7 +272,7 @@ int *test_23a (int n) int test_24 (void) { - void *ptr = alloca (sizeof (int)); /* { dg-message "memory is allocated on the stack here" } */ + void *ptr = __builtin_alloca (sizeof (int)); /* { dg-message "memory is allocated on the stack here" } */ free (ptr); /* { dg-warning "'free' of memory allocated on the stack by 'alloca' \\('ptr'\\) will corrupt the heap \\\[CWE-590\\\]" } */ } diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c index 35c9385b20611..9a7c414920ce2 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c @@ -2,7 +2,6 @@ /* { dg-require-effective-target alloca } */ #include <stddef.h> -#include <alloca.h> #include <stdlib.h> extern void do_stuff (const void *); @@ -15,7 +14,7 @@ void test_1 (size_t sz) if (sz >= LIMIT) ptr = malloc (sz); else - ptr = alloca (sz); + ptr = __builtin_alloca (sz); do_stuff (ptr); @@ -27,7 +26,7 @@ void test_2 (size_t sz) { void *ptr; if (sz < LIMIT) - ptr = alloca (sz); + ptr = __builtin_alloca (sz); else ptr = malloc (sz); @@ -41,7 +40,7 @@ void test_3 (size_t sz) { void *ptr; if (sz <= LIMIT) - ptr = alloca (sz); /* { dg-message "memory is allocated on the stack here" } */ + ptr = __builtin_alloca (sz); /* { dg-message "memory is allocated on the stack here" } */ else ptr = malloc (sz);
On Fri, 2021-01-15 at 14:45 -0300, Alexandre Oliva wrote: > On Jan 15, 2021, Olivier Hainque <hainque@adacore.com> wrote: > > > On 14 Jan 2021, at 22:13, Alexandre Oliva <oliva@adacore.com> > > wrote: > > > Would you mind if I submitted an alternate patch to do so? > > Not at all, thanks for your feedback and for proposing > > an alternative! > > Here's the modified patch. Regstrapped on x86_64-linux-gnu, also > tested > on x-arm-vx7r2. David, I'm leaning towards putting it in as > "obvious", > barring any objections. I think an issue here was that I assumed check_effective_target_alloca checks that "alloca" is supported, whereas I now see that I was wrong; it actually checks for "__builtin_alloca". I have no objections to the patch. Thanks Dave
On Jan 15, 2021, David Malcolm <dmalcolm@redhat.com> wrote: > I think an issue here was that I assumed check_effective_target_alloca > checks that "alloca" is supported, whereas I now see that I was wrong; > it actually checks for "__builtin_alloca". Yeah, it's a little misleading. And it only tests __builtin_alloca on nvptx, it's assumed present on all other targets. Almost a no-op. > I have no objections to the patch. Thanks, I put it in.
diff --git a/gcc/testsuite/gcc.dg/analyzer/alloca-leak.c b/gcc/testsuite/gcc.dg/analyzer/alloca-leak.c index 93319932d44a..e990cbc642b9 100644 --- a/gcc/testsuite/gcc.dg/analyzer/alloca-leak.c +++ b/gcc/testsuite/gcc.dg/analyzer/alloca-leak.c @@ -1,6 +1,6 @@ /* { dg-require-effective-target alloca } */ -#include <alloca.h> +#include "analyzer-alloca.h" void *test (void) { diff --git a/gcc/testsuite/gcc.dg/analyzer/analyzer-alloca.h b/gcc/testsuite/gcc.dg/analyzer/analyzer-alloca.h new file mode 100644 index 000000000000..edb20b118db3 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/analyzer-alloca.h @@ -0,0 +1,8 @@ + +/* This header is provided to circumvent the absence of alloca.h on + some systems. Spots that rely on the ability to designate + allocation points within tests can use __builtin_alloca directly + to prevent the diagnostic redirection to the local macro definition + here, as this is not a system header. */ + +#define alloca(n) __builtin_alloca(n) diff --git a/gcc/testsuite/gcc.dg/analyzer/data-model-1.c b/gcc/testsuite/gcc.dg/analyzer/data-model-1.c index 3f16a38ab14d..f4ba96b1e997 100644 --- a/gcc/testsuite/gcc.dg/analyzer/data-model-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/data-model-1.c @@ -3,7 +3,7 @@ #include <stdlib.h> #include <string.h> #include <stdio.h> -#include <alloca.h> +#include "analyzer-alloca.h" #include "analyzer-decls.h" struct foo diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c index 38ce1a52987b..b0039597c5fb 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-1.c @@ -1,6 +1,6 @@ /* { dg-require-effective-target alloca } */ -#include <alloca.h> +#include "analyzer-alloca.h" #include <stdlib.h> extern int foo (void); @@ -273,7 +273,7 @@ int *test_23a (int n) int test_24 (void) { - void *ptr = alloca (sizeof (int)); /* { dg-message "memory is allocated on the stack here" } */ + void *ptr = __builtin_alloca (sizeof (int)); /* { dg-message "memory is allocated on the stack here" } */ free (ptr); /* { dg-warning "'free' of memory allocated on the stack by 'alloca' \\('ptr'\\) will corrupt the heap \\\[CWE-590\\\]" } */ } diff --git a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c index 35c9385b2061..417459edf8cc 100644 --- a/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c +++ b/gcc/testsuite/gcc.dg/analyzer/malloc-paths-8.c @@ -2,7 +2,7 @@ /* { dg-require-effective-target alloca } */ #include <stddef.h> -#include <alloca.h> +#include "analyzer-alloca.h" #include <stdlib.h> extern void do_stuff (const void *); @@ -41,7 +41,7 @@ void test_3 (size_t sz) { void *ptr; if (sz <= LIMIT) - ptr = alloca (sz); /* { dg-message "memory is allocated on the stack here" } */ + ptr = __builtin_alloca (sz); /* { dg-message "memory is allocated on the stack here" } */ else ptr = malloc (sz);
Hello, Some of the tests in gcc.dg/analyzer exercize alloca and #include <alloca.h>. Some systems, e.g. VxWorks, don't feature alloca.h and the aforementioned tests fail. Other tests in the suite have been in this situation and the usual way around consists in resorting to __builtin_alloca directly. This patch is a proposal in this direction for gcc.dg/analyzer. It introduces a common "analyzer-alloca.h" where we can stick a common comment and a macro to redirect "alloca" directly to "__builtin_alloca". The intermediate macro in a non system header unfortunately diverts some of the warning expectations, as the allocation point for "x = alloca(128);" is shown on the macro definition and not at the macro invocation point. The patch circumvents this by calling __builtin_alloca directly from the points where the tests perform a warning check. I have verified that all the tests adjusted by the change now pass in a run for a powerpc-vxworks configuration. I'll gladly perform an extra regression test on a native system if the patch is considered ok. Would this be ok to commit ? Thanks in advance, With Kind Regards, Olivier 2020-10-29 Olivier Hainque <hainque@adacore.com> gcc/testsuite/ * gcc.dg/analyzer/analyzer-alloca.h: New file. * gcc.dg/analyzer/alloca-leak.c: Use it. * gcc.dg/analyzer/data-model-1.c: Use it. * gcc.dg/analyzer/malloc-1.c: Use it and replace call to be tracked by a direct call to __builtin_alloca. * gcc.dg/analyzer/malloc-paths-8.c: Likewise.