[PING][patch] PR81794: have "would be stringified in traditional C" warning in libcpp/macro.c be controlled by -Wtraditional
David Malcolm
dmalcolm@redhat.com
Tue Nov 21 02:58:00 GMT 2017
On Wed, 2017-10-25 at 14:45 -0400, Eric Gallager wrote:
> On Sat, Sep 30, 2017 at 8:05 PM, Eric Gallager <egall@gwmail.gwu.edu>
> wrote:
> > On Fri, Sep 29, 2017 at 11:15 AM, David Malcolm <dmalcolm@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 <egall@gwmail.gwu.edu>
> > > >
> > > > * macro.c (check_trad_stringification): Have warning be
> > > > controlled by
> > > > -Wtraditional.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > 2017-09-17 Eric Gallager <egall@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 <egall@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 <egall@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 <egall@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.
More information about the Gcc-patches
mailing list