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

Ping: https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01834.html

On 10/25/17, Eric Gallager <egall@gwmail.gwu.edu> 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.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.

New update on hard drive status: All data is salvaged and transferred,
but installing the new hard drive into the internal hard drive slot
didn't actually fixing the disk errors I was experiencing. It works
perfectly fine when I boot from it as an external hard drive, though,
which is what I'm doing now. So, I'm pretty sure I can commit again
now. So, OK to commit this patch?

>
>>
>> Thanks,
>> Eric
>