On Wed, 2017-10-25 at 14:45 -0400, Eric Gallager wrote: > On Sat, Sep 30, 2017 at 8:05 PM, Eric Gallager <eg...@gwmail.gwu.edu> > wrote: > > On Fri, Sep 29, 2017 at 11:15 AM, David Malcolm <dmalc...@redhat.co > > m> wrote: > > > On Sun, 2017-09-17 at 20:00 -0400, Eric Gallager wrote: > > > > Attached is a version of > > > > https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00481.html that > > > > contains > > > > a combination of both the fix and the testcase update, as > > > > requested > > > > in > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81794#c2 > > > > > > > > I had to use a different computer than I usually use to send > > > > this > > > > email, as the hard drive that originally had this patch is > > > > currently > > > > unresponsive. Since it's also the one with my ssh keys on it, I > > > > can't > > > > commit with it. Sorry if the ChangeLogs get mangled. > > > > > > Thanks for putting this together; sorry about the delay in > > > reviewing > > > it. > > > > > > The patch mostly looks good. > > > > > > Did you perform a full bootstrap and run of the testsuite with > > > this > > > patch? If so, it's best to state this in the email, so that we > > > know > > > that the patch has survived this level of testing. > > > > Yes, I bootstrapped with it, but I haven't done a full run of the > > testsuite with it yet; just the one testcase I updated. > > Update: I've now run the testsuite with it; test results are here: > https://gcc.gnu.org/ml/gcc-testresults/2017-10/msg01751.html > I'm pretty sure all the FAILs are unrelated to this patch. > > > > > > > > > Some nits below: > > > > > > > libcpp/ChangeLog: > > > > > > > > 2017-03-24 Eric Gallager <eg...@gwmail.gwu.edu> > > > > > > > > * macro.c (check_trad_stringification): Have warning be > > > > controlled by > > > > -Wtraditional. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > 2017-09-17 Eric Gallager <eg...@gwmail.gwu.edu> > > > > > > > > PR preprocessor/81794 > > > > * gcc.dg/pragma-diag-7.c: Update to include check for > > > > stringification. > > > > > > > > On Sat, May 6, 2017 at 11:33 AM, Eric Gallager <eg...@gwmail.gw > > > > u.edu> > > > > wrote: > > > > > Pinging this: https://gcc.gnu.org/ml/gcc-patches/2017-03/msg0 > > > > > 1325.h > > > > > tml > > > > > > > > > > On 3/24/17, Eric Gallager <eg...@gwmail.gwu.edu> wrote: > > > > > > It seemed odd to me that gcc was issuing a warning about > > > > > > compatibility > > > > > > with traditional C that I couldn't turn off by > > > > > > pushing/popping > > > > > > -Wtraditional over the problem area, so I made the attached > > > > > > (minor) > > > > > > patch to fix it. Survives bootstrap, but the only testing > > > > > > I've > > > > > > done > > > > > > with it has been compiling the one file that was giving me > > > > > > issues > > > > > > previously, which I'd need to reduce further to turn it > > > > > > into a > > > > > > proper > > > > > > test case. > > > > > > > > > > > > Thanks, > > > > > > Eric Gallager > > > > > > > > > > > > libcpp/ChangeLog: > > > > > > > > > > > > 2017-03-24 Eric Gallager <eg...@gwmail.gwu.edu> > > > > > > > > > > > > * macro.c (check_trad_stringification): Have warning > > > > > > be > > > > > > controlled by > > > > > > -Wtraditional. > > > > > > > > > > > > > > > > So I did the reducing I mentioned above and now have a > > > > > testcase for > > > > > it; it was pretty similar to the one from here: > > > > > https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01319.html > > > > > so I combined them into a single testcase and have attached > > > > > the > > > > > combined version. I can confirm that the testcase passes with > > > > > my > > > > > patch > > > > > applied. > > > > > > [...] > > > > > > > diff --git a/gcc/testsuite/gcc.dg/pragma-diag-7.c > > > > b/gcc/testsuite/gcc.dg/pragma-diag-7.c > > > > index 402ee56..e06c410 100644 > > > > --- a/gcc/testsuite/gcc.dg/pragma-diag-7.c > > > > +++ b/gcc/testsuite/gcc.dg/pragma-diag-7.c > > > > @@ -7,3 +7,16 @@ unsigned long bad = 1UL; /* { dg-warning > > > > "suffix" } */ > > > > /* Note the extra space before the pragma on this next line: > > > > */ > > > > #pragma GCC diagnostic pop > > > > unsigned long ok_again = 2UL; /* { dg-bogus "suffix" } */ > > > > + > > > > +/* Redundant with the previous pop, but just shows that it > > > > fails to stop the > > > > + * following warning with an unpatched GCC: */ > > > > +#pragma GCC diagnostic ignored "-Wtraditional" > > > > + > > > > +/* { dg-bogus "would be stringified" .+1 } */ > > > > > > As far as I can tell, this dg-bogus line doesn't actually get > > > matched; > > > when I run the testsuite without the libcpp fix, I get: > > > > > > FAIL: gcc.dg/pragma-diag-7.c (test for excess errors) > > > > > > If I update the dg-bogus line to read: > > > > > > /* { dg-bogus "would be stringified" "" { target *-*-* } .+1 } > > > */ > > > > > > then it's matched, and I get: > > > > > > FAIL: gcc.dg/pragma-diag-7.c (test for bogus messages, line > > > 16) > > > > > > I believe that as written the ".+1" 2nd argument is interpreted > > > as a > > > human-readable description of the problem, rather than as a line > > > offset; I believe you would need to add positional args for the > > > description and filter so that the line offset is argument 4. > > > > > > That said, I think the dg-bogus here is unnecessary: if the > > > warning is > > > erroneously emitted, we get: > > > > > > FAIL: gcc.dg/pragma-diag-7.c (test for excess errors) > > > > > > (where "errors" really means "excess errors, warnings and > > > extraneous > > > gunk that isn't a note"). > > > > > > So this testcase hunk is good without the dg-bogus line. > > > > OK, the line can be removed when committing. > > > > > > > > > +#define UNW_DEC_PROLOGUE(fmt, body, rlen, arg) \ > > > > + do > > > > { > > > > \ > > > > + unw_rlen = > > > > rlen; \ > > > > + *(int *)arg = > > > > body; \ > > > > + printf(" %s:%s(rlen=%lu)\n", > > > > \ > > > > + fmt, (body ? "body" : "prologue"), (unsigned > > > > long)rlen); \ > > > > + } while (0) > > > > diff --git a/libcpp/macro.c b/libcpp/macro.c > > > > index de18c22..71363b5 100644 > > > > --- a/libcpp/macro.c > > > > +++ b/libcpp/macro.c > > > > @@ -3316,7 +3316,7 @@ check_trad_stringification (cpp_reader > > > > *pfile, const cpp_macro *macro, > > > > if (NODE_LEN (node) == len > > > > && !memcmp (p, NODE_NAME (node), len)) > > > > { > > > > - cpp_error (pfile, CPP_DL_WARNING, > > > > + cpp_warning (pfile, CPP_W_TRADITIONAL, > > > > "macro argument \"%s\" would be stringified in > > > > traditional C", > > > > NODE_NAME (node)); > > > > break; > > > > > > This hunk looks good. > > > > > > So the patch is good if you drop the bogus "dg-bogus" line, > > > provided > > > that it's bootstrapped and regression-tested. > > > > > > Did you complete the FSF copyright assignment paperwork, and do > > > you > > > have access to the GCC compile farm? (that could help with > > > testing) > > > > Yes, I have a copyright assignment on file (it was a prerequisite > > for > > putting my name in the MAINTAINERS file), but no, I don't think I > > have > > access to the GCC compile farm. I agree, it'd be much better to > > test > > on the compile farm than on my own computer, since running the > > testsuite on my own computer usually takes an entire day (it's > > really > > old and slow). > > Update: I now have access to the GCC compile farm; that's where I > produced the test results linked above. > > > > > > > > > Thanks again for working on this (and for the work you've been > > > doing in > > > Bugzilla); I hope you get your hard drive back... > > > > > > Dave > > > > And thank you for all your work on improving gcc's diagnostics! If > > you'd like to send me a new hard drive, feel free to contact me > > off-list for my mailing address. > > Update on hard drive status: I've ordered and received a new one, I > just need to salvage data off the old one and then install the new > one > and then I should be good for committing again. I'd still appreciate > if someone else could commit for me while I'm still working on fixing > it though. > > > > > Thanks, > > Eric
Sorry about the delay in dealing with this. I verified the patch (bootstrap and regression testing), and there were no regressions against a "control" build (on x86_64-pc-linux-gnu) [1]. I decided to add the fixed version of the "dg-bogus", as I felt it helped document the intent of the test case. I've committed it to trunk on your behalf as r254981. Dave [1] FWIW the script I use for this is here: https://github.com/davidmalcolm/gcc-build which uses this comparison script: https://github.com/davidmalcolm/jamais-vu Takes a few hours and a lot of disk space.
