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

[Eric Gallager <egall at gwmail dot gwu dot edu>]

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.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.

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.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.
>
> 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