[PING][patch] PR81794: have "would be stringified in traditional C" warning in libcpp/macro.c be controlled by -Wtraditional

[David Malcolm]

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.

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.gwu.edu>
> wrote:
> > Pinging this: https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01325.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.

> +#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)

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