On Fri, Sep 29, 2017 at 11:15 AM, David Malcolm <dmalc...@redhat.com> 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. > > 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.gwu.edu> >> wrote: >> > Pinging this: https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01325.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). > > 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. Thanks, Eric
