| Message ID | ee9a8a6b11438f158933a21d7965b486987be5a8.1721454759.git.sam@gentoo.org |
|---|---|
| State | New |
| Headers | show |
| Series | testsuite: fix pr115929-1.c with -Wformat-security | expand |
On Sat, 2024-07-20 at 06:52 +0100, Sam James wrote: > Some distributions like Gentoo make -Wformat and -Wformat-security > enabled by default. Pass -Wno-format to the test to avoid a spurious > fail in such environments. > > gcc/testsuite/ > PR rtl-optimization/115929 > * gcc.dg/torture/pr115929-1.c: Pass -Wno-format. > --- IMO if you are patching GCC downstream to enable some options, you can patch the test case in the same .patch file anyway instead of pushing it upstream. If we take the responsibility to make the test suite anticipate random downstream changes, the test suite will ended up filled with different workarounds for 42 distros. If we have to anticipate downstream changes we should make a policy about which changes we must anticipate (hmm and if we'll anticipate - Wformat by default why not add a configuration option for it by the way?), or do it in a more generic way (using a .spec file to explicitly give the "baseline" options for testing?) > Richard, is this OK? I could adjust the testcase if you prefer. > > Please commit on my behalf if it's fine. Thanks. > > gcc/testsuite/gcc.dg/torture/pr115929-1.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/testsuite/gcc.dg/torture/pr115929-1.c b/gcc/testsuite/gcc.dg/torture/pr115929-1.c > index 19b831ab99ef..d81081e2f2e9 100644 > --- a/gcc/testsuite/gcc.dg/torture/pr115929-1.c > +++ b/gcc/testsuite/gcc.dg/torture/pr115929-1.c > @@ -1,5 +1,5 @@ > /* { dg-require-effective-target lp64 } */ > -/* { dg-options "-fno-gcse -fschedule-insns -fno-guess-branch-probability -fno-tree-fre -fno-tree-ch" } */ > +/* { dg-options "-fno-gcse -fschedule-insns -fno-guess-branch-probability -fno-tree-fre -fno-tree-ch -Wno-format" } */ > > int printf(const char *, ...); > int a[6], b, c; >
Xi Ruoyao <xry111@xry111.site> writes: > On Sat, 2024-07-20 at 06:52 +0100, Sam James wrote: >> Some distributions like Gentoo make -Wformat and -Wformat-security >> enabled by default. Pass -Wno-format to the test to avoid a spurious >> fail in such environments. >> >> gcc/testsuite/ >> PR rtl-optimization/115929 >> * gcc.dg/torture/pr115929-1.c: Pass -Wno-format. >> --- > > IMO if you are patching GCC downstream to enable some options, you can > patch the test case in the same .patch file anyway instead of pushing it > upstream. It's a fair argument. That said, you did say the same thing reflexively about -fhardened, even if this is a different situation ;) One might argue that especially for torture/, supporting "not unreasonable" random flags is not a bad thing. > > If we take the responsibility to make the test suite anticipate random > downstream changes, the test suite will ended up filled with different > workarounds for 42 distros. > My counterpoint would be that there are certain warnings we know various distros enable by default, and various warnings where it's not inconceivable we might do them upstream at some point. Being loose about conformance in test cases is part of why the C99 enforcement took a while. > If we have to anticipate downstream changes we should make a policy > about which changes we must anticipate (hmm and if we'll anticipate - > Wformat by default why not add a configuration option for it by the > way?), or do it in a more generic way (using a .spec file to explicitly > give the "baseline" options for testing?) > Anyway, I take the point, but it was cheaper to ask with a patch attached than have it in my head and not know what the position was. I like the .spec idea. In the past, we used custom .spec extensively downstream, and we stopped before my tenure. My intention long-term with Arsen is to bring it back as it feels like a better fit. I'm also happy to adjust the testcase given I reproduced the original issue fine. Or to leave it if the consensus is to. Whatever works for me. I dare say we're spending time talking about it than the occasional -Wno- costs though. >> Richard, is this OK? I could adjust the testcase if you prefer. >> >> [...] thanks, sam
On Sat, 2024-07-20 at 07:16 +0100, Sam James wrote: > Xi Ruoyao <xry111@xry111.site> writes: > > > On Sat, 2024-07-20 at 06:52 +0100, Sam James wrote: > > > Some distributions like Gentoo make -Wformat and -Wformat-security > > > enabled by default. Pass -Wno-format to the test to avoid a spurious > > > fail in such environments. > > > > > > gcc/testsuite/ > > > PR rtl-optimization/115929 > > > * gcc.dg/torture/pr115929-1.c: Pass -Wno-format. > > > --- > > > > IMO if you are patching GCC downstream to enable some options, you can > > patch the test case in the same .patch file anyway instead of pushing it > > upstream. > > It's a fair argument. > > That said, you did say the same thing reflexively about -fhardened, even > if this is a different situation ;) > > One might argue that especially for torture/, supporting "not > unreasonable" random flags is not a bad thing. > > > > > If we take the responsibility to make the test suite anticipate random > > downstream changes, the test suite will ended up filled with different > > workarounds for 42 distros. > > > > My counterpoint would be that there are certain warnings we know various > distros enable by default, and various warnings where it's not > inconceivable we might do them upstream at some point. Then I'll prefer the change log not to say "Gentoo makes GCC enable - Wformat by default", but something like "-Wformat is a commonly used flag and people may run test suite with it enabled" and maybe also "we may turn it on by default in the future." I.e. not to mention a specific distro (so distro maintainers cannot use the changelog as a reason to push random messes), but consider the situation someone running the test suite with some not so strange CFLAGS. Just my 2 cents. I'm just a "write after approval" guy anyway and I've no permission to Ack or Nack a patch. And I'm somewhat frustrated after my test suite fixes for default PIE (well we have --enable-default-pie and AFAIK every distro is using it, so IMO it should have a higher priority than -Wformat) gets no response after Ping^5.
Xi Ruoyao <xry111@xry111.site> writes: > On Sat, 2024-07-20 at 06:52 +0100, Sam James wrote: >> Some distributions like Gentoo make -Wformat and -Wformat-security >> enabled by default. Pass -Wno-format to the test to avoid a spurious >> fail in such environments. >> >> gcc/testsuite/ >> PR rtl-optimization/115929 >> * gcc.dg/torture/pr115929-1.c: Pass -Wno-format. >> --- > > IMO if you are patching GCC downstream to enable some options, you can > patch the test case in the same .patch file anyway instead of pushing it > upstream. > > If we take the responsibility to make the test suite anticipate random > downstream changes, the test suite will ended up filled with different > workarounds for 42 distros. Yeah, I'm worried about that too. > If we have to anticipate downstream changes we should make a policy > about which changes we must anticipate (hmm and if we'll anticipate - > Wformat by default why not add a configuration option for it by the > way?), or do it in a more generic way (using a .spec file to explicitly > give the "baseline" options for testing?) Two systematic ways of dealing with this under the current testsuite framework would be: (1) Make dg-torture.exp add -w by default. This is what gcc.c-torture already does. Then, tests that want to test for warnings can enable them explicitly. Some of the existing dg-warnings are already due to lack of -w, rather than something that the test was originally designed for. E.g. pr26565.c. (2) Make dg-torture.exp add -Wall -Wextra by default, so that tests have to suppress any warnings they don't want. Personally, I'd prefer one of those two rather than patching upstream tests for downstream changes. Thanks, Richard
Richard Sandiford <richard.sandiford@arm.com> writes: > Xi Ruoyao <xry111@xry111.site> writes: >> On Sat, 2024-07-20 at 06:52 +0100, Sam James wrote: >>> Some distributions like Gentoo make -Wformat and -Wformat-security >>> enabled by default. Pass -Wno-format to the test to avoid a spurious >>> fail in such environments. >>> >>> gcc/testsuite/ >>> PR rtl-optimization/115929 >>> * gcc.dg/torture/pr115929-1.c: Pass -Wno-format. >>> --- >> >> IMO if you are patching GCC downstream to enable some options, you can >> patch the test case in the same .patch file anyway instead of pushing it >> upstream. >> >> If we take the responsibility to make the test suite anticipate random >> downstream changes, the test suite will ended up filled with different >> workarounds for 42 distros. > > Yeah, I'm worried about that too. > >> If we have to anticipate downstream changes we should make a policy >> about which changes we must anticipate (hmm and if we'll anticipate - >> Wformat by default why not add a configuration option for it by the >> way?), or do it in a more generic way (using a .spec file to explicitly >> give the "baseline" options for testing?) > > Two systematic ways of dealing with this under the current testsuite > framework would be: > > (1) Make dg-torture.exp add -w by default. This is what gcc.c-torture > already does. Then, tests that want to test for warnings can > enable them explicitly. > > Some of the existing dg-warnings are already due to lack of -w, > rather than something that the test was originally designed for. > E.g. pr26565.c. > > (2) Make dg-torture.exp add -Wall -Wextra by default, so that tests > have to suppress any warnings they don't want. > > Personally, I'd prefer one of those two rather than patching upstream > tests for downstream changes. I don't mind doing the work once we have consensus. (1) feels more pure but (2) is more progressive and lets us make things error out by default in future upstream with a bit more freedom. In the meantime, I'll return to other testsuite bits I have in mind. thanks, sam
Richard Sandiford <richard.sandiford@arm.com> writes: > Xi Ruoyao <xry111@xry111.site> writes: >> On Sat, 2024-07-20 at 06:52 +0100, Sam James wrote: >>> Some distributions like Gentoo make -Wformat and -Wformat-security >>> enabled by default. Pass -Wno-format to the test to avoid a spurious >>> fail in such environments. >>> >>> gcc/testsuite/ >>> PR rtl-optimization/115929 >>> * gcc.dg/torture/pr115929-1.c: Pass -Wno-format. >>> --- >> >> IMO if you are patching GCC downstream to enable some options, you can >> patch the test case in the same .patch file anyway instead of pushing it >> upstream. >> >> If we take the responsibility to make the test suite anticipate random >> downstream changes, the test suite will ended up filled with different >> workarounds for 42 distros. > > Yeah, I'm worried about that too. > >> If we have to anticipate downstream changes we should make a policy >> about which changes we must anticipate (hmm and if we'll anticipate - >> Wformat by default why not add a configuration option for it by the >> way?), or do it in a more generic way (using a .spec file to explicitly >> give the "baseline" options for testing?) > > Two systematic ways of dealing with this under the current testsuite > framework would be: > > (1) Make dg-torture.exp add -w by default. This is what gcc.c-torture > already does. Then, tests that want to test for warnings can > enable them explicitly. > > Some of the existing dg-warnings are already due to lack of -w, > rather than something that the test was originally designed for. > E.g. pr26565.c. > > (2) Make dg-torture.exp add -Wall -Wextra by default, so that tests > have to suppress any warnings they don't want. > > Personally, I'd prefer one of those two rather than patching upstream > tests for downstream changes. Another question: so, for now, I'm testing with RUNTESTFLAGS=... to disable -Wformat. This works OK, but some tests for format behaviour are doing: /* { dg-options "-Wall" } */ instead of /* { dg-options "-Wformat" } */ so they end up failing, because GCC is clever wrt options, so gcc -Wno-format -Wall # doesn't enable -Wformat but gcc -Wno-format -Wall -Wformat # does enable -Wformat How do you feel about those being changed to either -Wall -Wformat (and so on), or just -Wformat, as appropriate? thanks, sam
diff --git a/gcc/testsuite/gcc.dg/torture/pr115929-1.c b/gcc/testsuite/gcc.dg/torture/pr115929-1.c index 19b831ab99ef..d81081e2f2e9 100644 --- a/gcc/testsuite/gcc.dg/torture/pr115929-1.c +++ b/gcc/testsuite/gcc.dg/torture/pr115929-1.c @@ -1,5 +1,5 @@ /* { dg-require-effective-target lp64 } */ -/* { dg-options "-fno-gcse -fschedule-insns -fno-guess-branch-probability -fno-tree-fre -fno-tree-ch" } */ +/* { dg-options "-fno-gcse -fschedule-insns -fno-guess-branch-probability -fno-tree-fre -fno-tree-ch -Wno-format" } */ int printf(const char *, ...); int a[6], b, c;