Re: [PATCH] Fix _Pragma GCC diagnostic in macro expansions

[David Malcolm <dmalcolm at redhat dot com>]

On Wed, 2018-07-04 at 10:53 +0000, Bernd Edlinger wrote:

Sorry for the delay in reviewing this.

> Hi,
> 
> currently _Pragma("GCC diagnostic ...") does not properly
> work in macro expansions.
> 
> Consider the following code:
> 
> #define B _Pragma("GCC diagnostic push") \
> 	  _Pragma("GCC diagnostic ignored \"-Wattributes\"")
> #define E _Pragma("GCC diagnostic pop")
> 
> #define X() B int __attribute((unknown_attr)) x; E /* { dg-bogus
> "attribute directive ignored" } */
> 
> void test1(void)
> {
>     X()  /* { dg-bogus "in expansion of macro" } */
> }
> 
> 
> Warnings happen in C++ despite the _Pragma, while C happens to
> suppress the warnings
> more or less by accident.
> 
> This is connected to the fact that GCC uses the location of the
> closing parenthesis of the
> function-like macro expansion in the _Pragma, while the rest of the
> locations are relative
> to the macro expansion point, which is the letter X in this case.
> 
> This patch changes the location of builtin macros and _Pragma to use
> the macro expansion
> point instead of the closing parenthesis.

While reviewing the various PRs for the testcases this touches, I
noticed:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61817#c3
which has a link to:
  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1911.htm
which has:

  Suggested Technical Corrigendum

  Add to 6.10.8.1, paragraph 1, item __LINE__:

  The line number of a pp token is implementation defined to
  be the (physical) line number of either the first character
  or the last character of the pp token. The line number of a
  __LINE__ that spans multiple physical lines is implementation
  defined to be either the first line or the last line of that
  __LINE__. The line number of a __LINE__ in a macro body is the
  line number of the macro invocation. The line number of a macro
  invocation that spans multiple (physical or logical) lines is
  implementation defined to be either the line number of the
  first character of the macro name, the last character of the
  macro name or the closing ')' (if there is one).

I don't know the status of that suggested corrigendum, but if I'm
reading it right, changing the location from that of the closing ')' to
that of the macro name would keep us in compliance with that final
sentence (after extracting the location's line number).

> A few test cases had to be adjusted, most changes were necessary
> because the __LINE__
> location moved to the macro expansion point, which looks like a
> straight improvement.
> 
> In pr61817-2.c the location of __LINE__ depends on -ftrack-macro-
> expansion,
> when enabled the location of the macro argument is the spelling
> location, while all other
> locations change to the macro expansion point.
> 
> The C++ pagma plugin.c is also affected by the change, because the
> input_location is now
> the spelling location of _Pragma in DO_PRAGMA and has to be converted
> to the expansion
> point of the macro to get the expected result.
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

Some nits:

> libcpp:
> 2018-07-04  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
>         * macro.c (enter_macro_context): Change the location info for builtin
>         macros from location of the closing parenthesis to location of the macro
>         expansion point.

Please update to say "builtin macros and _Pragma" here, rather than just
"builtin macros" here.

> testsuite:
> 2018-07-04  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
>         * c-c++-common/cpp/diagnostic-pragma-2.c: New test.
>         * c-c++-common/pr69558.c: Remove xfail.
>         * gcc.dg/cpp/builtin-macro-1.c: Adjust test expectations.
>         * gcc.dg/pr61817-1.c: Likewise.
>         * gcc.dg/pr61817-2.c: Likewise.
>         * g++.dg/plugin/pragma_plugin.c: Warn at expansion_point_location.

Please reference PR 69558 in both ChangeLog entries (given that this
fixes an XFAIL).

OK for trunk with the above changes; thanks.

It looks like with this change we can remove Jakub's r233058 hack.  I
briefly tested with it, and it seems to work.  But that can wait for a
followup.

Dave