On Wed, Feb 12, 2025 at 08:27:37PM -0500, Lewis Hyatt wrote:
> Hello-
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118838
>
> This patch addresses the issue mentioned in the PR (another instance of
> _Pragma string location issues). bootstrap + regtest all languages on
> aarch64 looks good. Is it OK please for now or for stage 1? Note, it is not
> a regression, since this never worked in C or C++ frontends; but on the
> other hand, r15-4505 for GCC 15 fixed some related issues, so it could be
> nice if this one gets in along with it. Thanks!
>
> -Lewis
>
> -- >8 --
>
> The warning for -Wunknown-pragmas is issued at the location provided by
> libcpp to the def_pragma() callback. This location is
> cpp_reader::directive_line, which is a location for the start of the line
> only; it is also not a valid location in case the unknown pragma was lexed
> from a _Pragma string. These factors make it impossible to suppress
> -Wunknown-pragmas via _Pragma("GCC diagnostic...") directives on the same
> source line, as in the PR and the test case. Address that by issuing the
> warning at a better location returned by cpp_get_diagnostic_override_loc().
> libcpp already maintains this location to handle _Pragma-related diagnostics
> internally; it was needed also to make a publicly accessible version of it.
>
> gcc/c-family/ChangeLog:
>
> PR c/118838
> * c-lex.cc (cb_def_pragma): Call cpp_get_diagnostic_override_loc()
> to get a valid location at which to issue -Wunknown-pragmas, in case
> it was triggered from a _Pragma.
>
> libcpp/ChangeLog:
>
> PR c/118838
> * errors.cc (cpp_get_diagnostic_override_loc): New function.
> * include/cpplib.h (cpp_get_diagnostic_override_loc): Declare.
>
> gcc/testsuite/ChangeLog:
>
> PR c/118838
> * c-c++-common/cpp/pragma-diagnostic-loc-2.c: New test.
> * g++.dg/gomp/macro-4.C: Adjust expected output.
> * gcc.dg/gomp/macro-4.c: Likewise.
> * gcc.dg/cpp/Wunknown-pragmas-1.c: Likewise.
> ---
> libcpp/errors.cc | 10 +++++++++
> libcpp/include/cpplib.h | 5 +++++
> gcc/c-family/c-lex.cc | 7 +++++-
> .../cpp/pragma-diagnostic-loc-2.c | 15 +++++++++++++
> gcc/testsuite/g++.dg/gomp/macro-4.C | 8 +++----
> gcc/testsuite/gcc.dg/cpp/Wunknown-pragmas-1.c | 22 +++++++++++--------
> gcc/testsuite/gcc.dg/gomp/macro-4.c | 8 +++----
> 7 files changed, 57 insertions(+), 18 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/cpp/pragma-diagnostic-loc-2.c
>
> diff --git a/libcpp/errors.cc b/libcpp/errors.cc
> index 9621c4b66ea..d9efb6acd30 100644
> --- a/libcpp/errors.cc
> +++ b/libcpp/errors.cc
> @@ -52,6 +52,16 @@ cpp_diagnostic_get_current_location (cpp_reader *pfile)
> }
> }
>
> +/* Sometimes a diagnostic needs to be generated before libcpp has been able
> + to generate a valid location for the current token; in that case, the
> + non-zero location returned by this function is the preferred one to use.
> */
> +
> +location_t
> +cpp_get_diagnostic_override_loc (const cpp_reader *pfile)
> +{
> + return pfile->diagnostic_override_loc;
> +}
> +
> /* Print a diagnostic at the given location. */
>
> ATTRIBUTE_CPP_PPDIAG (5, 0)
> diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
> index 90aa3160ebf..04d4621da3c 100644
> --- a/libcpp/include/cpplib.h
> +++ b/libcpp/include/cpplib.h
> @@ -1168,6 +1168,11 @@ extern const char *cpp_probe_header_unit (cpp_reader
> *, const char *file,
> extern const char *cpp_get_narrow_charset_name (cpp_reader *) ATTRIBUTE_PURE;
> extern const char *cpp_get_wide_charset_name (cpp_reader *) ATTRIBUTE_PURE;
>
> +/* Sometimes a diagnostic needs to be generated before libcpp has been able
> + to generate a valid location for the current token; in that case, the
> + non-zero location returned by this function is the preferred one to use.
> */
I don't love duplicating the comment like this, it's going to get out of sync.
> +extern location_t cpp_get_diagnostic_override_loc (const cpp_reader *);
> +
> /* This function reads the file, but does not start preprocessing. It
> returns the name of the original file; this is the same as the
> input file, except for preprocessed input. This will generate at
> diff --git a/gcc/c-family/c-lex.cc b/gcc/c-family/c-lex.cc
> index e450c9a57f0..df84020de62 100644
> --- a/gcc/c-family/c-lex.cc
> +++ b/gcc/c-family/c-lex.cc
> @@ -248,7 +248,12 @@ cb_def_pragma (cpp_reader *pfile, location_t loc)
> {
> const unsigned char *space, *name;
> const cpp_token *s;
> - location_t fe_loc = loc;
> +
> + /* If we are processing a _Pragma, LOC is not a valid location, but
> libcpp
> + will provide a good location via this function instead. */
> + location_t fe_loc = cpp_get_diagnostic_override_loc (pfile);
> + if (!fe_loc)
I think checking == UNKNOWN_LOCATION is clearer.
> + fe_loc = loc;
>
> space = name = (const unsigned char *) "";
>
> diff --git a/gcc/testsuite/c-c++-common/cpp/pragma-diagnostic-loc-2.c
> b/gcc/testsuite/c-c++-common/cpp/pragma-diagnostic-loc-2.c
> new file mode 100644
> index 00000000000..e7e8cf23759
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/cpp/pragma-diagnostic-loc-2.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-Wunknown-pragmas" } */
> +
> +/* PR 118838 */
Usually this would be the first line and also mention "c/".
But those comments are details. I think the patch is OK for GCC 16.
Thanks,
Marek
