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 again: https://gcc.gnu.org/ml/gcc-patches/2017-11/msg00123.html

On 11/2/17, Eric Gallager <egall@gwmail.gwu.edu> wrote:
> 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
>>
>