[Bug c++/66290] New: wrong location for -Wunused-macros

[Bug c++/66290] New: wrong location for -Wunused-macros

[manu at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

            Bug ID: 66290
           Summary: wrong location for -Wunused-macros
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: manu at gcc dot gnu.org
  Target Milestone: ---

/* { dg-do compile } */
/* { dg-options "-E -Wunused-macros" } */
#define BAR
#define FOO

manuel@gcc14:~/test2$ ./223651/build/gcc/cc1 -Wunused-macros pragma-diag-4.c 

pragma-diag-4.c:4:0: warning: macro "FOO" is not used [-Wunused-macros]
 #define FOO
^
pragma-diag-4.c:3:0: warning: macro "BAR" is not used [-Wunused-macros]
 #define BAR
^

Not perfect, but ok.

manuel@gcc14:~/test2$ ./223651/build/gcc/cc1plus -Wunused-macros
pragma-diag-4.c 
pragma-diag-4.c:1:0: warning: macro "FOO" is not used [-Wunused-macros]
 /* { dg-do compile } */
^
pragma-diag-4.c:1:0: warning: macro "BAR" is not used [-Wunused-macros]


Terrible!

[Bug c++/66290] wrong location for -Wunused-macros

[lhyatt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

Lewis Hyatt <lhyatt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |lhyatt at gcc dot gnu.org

--- Comment #3 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
(https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598188.html)
This testcase, hopefully soon to be committed as c-c++-common/pragma-diag-15.c:

-------
/* { dg-do compile } */
/* { dg-additional-options "-Wunused-macros" } */
#define X /* { dg-warning "-:-Wunused-macros" {} { xfail c++ } } */
#pragma GCC diagnostic ignored "-Wunused-macros"
-------

fails for C++, I believe it's the same thing pointed out in this PR. The reason
is because of this in c_cpp_diagnostic() in c-common.cc:

-------
  if (done_lexing)
    richloc->set_range (0, input_location, SHOW_RANGE_WITH_CARET);
-------

This warning is issued at the end of compilation, when done_lexing is true.
However, input_location is not the right thing to use here, the location
provided was remembered from the time the macro was lexed, and that's the one
that needs to be used. The way it is now, input_location is at the end of the
file, and so the macro is deemed to have been defined after the #pragma.

I think this done_lexing concept was added to solve PR17964 many years ago.
Does anyone know if it's still needed, are there still cases where libcpp can
generate a diagnostic while C++ is parsing the already-lexed tokens? I have
checked anyway, that the particular testcase on PR17964 passes if done_lexing
is kept always false. I am trying the full testsuite now as well. If it can't
be removed, then I think we need some way to tell c_cpp_diagnostic not to
override a location for instances like this. Or paradoxically, we could set
done_lexing=false in c_common_finish(), which would fix it as well. It fixes
both my testcase, and the one in this original PR. Could be a reasonable fix,
perhaps combined with renaming done_lexing to something more like
override_libcpp_locations, but would be nice if it could just be removed as
well. I can look into that but perhaps Joseph knows the details here already?

diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
index b9f01a65ed7..25a3c50de8e 100644
--- a/gcc/c-family/c-opts.cc
+++ b/gcc/c-family/c-opts.cc
@@ -1283,6 +1283,7 @@ c_common_finish (void)

   /* For performance, avoid tearing down cpplib's internal structures
      with cpp_destroy ().  */
+  done_lexing = false;
   cpp_finish (parse_in, deps_stream);

   if (deps_stream && deps_stream != out_stream && deps_stream != stdout

[Bug c++/66290] wrong location for -Wunused-macros

[lhyatt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

--- Comment #4 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
OK, I understand now why done_lexing is necessary, plenty of places call back
into libcpp after lexing, e.g. to interpret strings, and this may generate
warnings.
I think that one line patch is the way to go then.

diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
index b9f01a65ed7..25a3c50de8e 100644
--- a/gcc/c-family/c-opts.cc
+++ b/gcc/c-family/c-opts.cc
@@ -1283,6 +1283,7 @@ c_common_finish (void)

   /* For performance, avoid tearing down cpplib's internal structures
      with cpp_destroy ().  */
+  done_lexing = false;
   cpp_finish (parse_in, deps_stream);

   if (deps_stream && deps_stream != out_stream && deps_stream != stdout

[Bug c++/66290] wrong location for -Wunused-macros

[lhyatt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

--- Comment #5 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
Patch submitted for review:
https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598989.html

[Bug c++/66290] wrong location for -Wunused-macros

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Lewis Hyatt <lhyatt@gcc.gnu.org>:

https://gcc.gnu.org/g:b04c399e258e686dddad879bf7e27d9e28fd6fde

commit r13-1903-gb04c399e258e686dddad879bf7e27d9e28fd6fde
Author: Lewis Hyatt <lhyatt@gmail.com>
Date:   Tue Jul 12 09:47:47 2022 -0400

    c++: Fix location for -Wunused-macros [PR66290]

    In C++, since all tokens are lexed from libcpp up front, diagnostics
generated
    by libcpp after lexing has completed do not get a valid location from
libcpp
    (rather, libcpp thinks they all pertain to the end of the file.) This has
long
    been addressed using the global variable "done_lexing", which the C++
frontend
    sets at the appropriate time; when done_lexing is true, then
c_cpp_diagnostic(),
    which outputs libcpp's diagnostics, uses input_location instead of the
wrong
    libcpp location. The C++ frontend arranges that input_location will point
to the
    token it is currently processing, so this generally works fine. However,
there
    is one exception currently, which is -Wunused-macros. This gets generated
at the
    end of processing in cpp_finish (), since we need to wait until then to
    determine whether a macro was eventually used or not. But the locations it
    passes to c_cpp_diagnostic () were remembered from the original lexing and
hence
    they should not be overridden with input_location, which is now the one
    incorrectly pointing to the end of the file.

    Fixed by setting done_lexing=false again just prior to calling cpp_finish
(). I
    also renamed the variable from done_lexing to "override_libcpp_locations",
since
    it's now not strictly about lexing anymore.

    There is no new testcase with this patch, since we already had an xfailed
    testcase which is now fixed.

    gcc/c-family/ChangeLog:

            PR c++/66290
            * c-common.h: Rename global done_lexing to
            override_libcpp_locations.
            * c-common.cc (c_cpp_diagnostic): Likewise.
            * c-opts.cc (c_common_finish): Set override_libcpp_locations
            (formerly done_lexing) immediately prior to calling cpp_finish ().

    gcc/cp/ChangeLog:

            PR c++/66290
            * parser.cc (cp_lexer_new_main): Rename global done_lexing to
            override_libcpp_locations.

    gcc/testsuite/ChangeLog:

            PR c++/66290
            * c-c++-common/pragma-diag-15.c: Remove xfail for C++.

[Bug c++/66290] wrong location for -Wunused-macros

[lhyatt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

--- Comment #7 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
The wrong location is fixed for GCC 13. Shall I leave the PR open for now,
since there was also the issue of getting a caret pointing to the name of
macro, rather than just a diagnostic for the whole line? I can look into that
too.

[Bug c++/66290] wrong location for -Wunused-macros

[lhyatt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

--- Comment #8 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
(In reply to Lewis Hyatt from comment #7)
> The wrong location is fixed for GCC 13. Shall I leave the PR open for now,
> since there was also the issue of getting a caret pointing to the name of
> macro, rather than just a diagnostic for the whole line? I can look into
> that too.

Patch to improve the range information on the diagnostic submitted for review
here: https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599397.html
I think if that gets applied then we could close this one.

[Bug c++/66290] wrong location for -Wunused-macros

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Lewis Hyatt <lhyatt@gcc.gnu.org>:

https://gcc.gnu.org/g:4f3be7cbebce8ec9e0c5d9340b2772581454b862

commit r14-2004-g4f3be7cbebce8ec9e0c5d9340b2772581454b862
Author: Lewis Hyatt <lhyatt@gmail.com>
Date:   Wed Aug 3 10:46:23 2022 -0400

    libcpp: Improve location for macro names [PR66290]

    When libcpp reports diagnostics whose locus is a macro name (such as for
    -Wunused-macros), it uses the location in the cpp_macro object that was
    stored by _cpp_new_macro. This is currently set to pfile->directive_line,
    which contains the line number only and no column information. This patch
    changes the stored location to the src_loc for the token defining the macro
    name, which includes the location and range information.

    libcpp/ChangeLog:

            PR c++/66290
            * macro.cc (_cpp_create_definition): Add location argument.
            * internal.h (_cpp_create_definition): Adjust prototype.
            * directives.cc (do_define): Pass new location argument to
            _cpp_create_definition.
            (do_undef): Stop passing inferior location to
cpp_warning_with_line;
            the default from cpp_warning is better.
            (cpp_pop_definition): Pass new location argument to
            _cpp_create_definition.
            * pch.cc (cpp_read_state): Likewise.

    gcc/testsuite/ChangeLog:

            PR c++/66290
            * c-c++-common/cpp/macro-ranges.c: New test.
            * c-c++-common/cpp/line-2.c: Adapt to check for column information
            on macro-related libcpp warnings.
            * c-c++-common/cpp/line-3.c: Likewise.
            * c-c++-common/cpp/macro-arg-count-1.c: Likewise.
            * c-c++-common/cpp/pr58844-1.c: Likewise.
            * c-c++-common/cpp/pr58844-2.c: Likewise.
            * c-c++-common/cpp/warning-zero-location.c: Likewise.
            * c-c++-common/pragma-diag-14.c: Likewise.
            * c-c++-common/pragma-diag-15.c: Likewise.
            * g++.dg/modules/macro-2_d.C: Likewise.
            * g++.dg/modules/macro-4_d.C: Likewise.
            * g++.dg/modules/macro-4_e.C: Likewise.
            * g++.dg/spellcheck-macro-ordering.C: Likewise.
            * gcc.dg/builtin-redefine.c: Likewise.
            * gcc.dg/cpp/Wunused.c: Likewise.
            * gcc.dg/cpp/redef2.c: Likewise.
            * gcc.dg/cpp/redef3.c: Likewise.
            * gcc.dg/cpp/redef4.c: Likewise.
            * gcc.dg/cpp/ucnid-11-utf8.c: Likewise.
            * gcc.dg/cpp/ucnid-11.c: Likewise.
            * gcc.dg/cpp/undef2.c: Likewise.
            * gcc.dg/cpp/warn-redefined-2.c: Likewise.
            * gcc.dg/cpp/warn-redefined.c: Likewise.
            * gcc.dg/cpp/warn-unused-macros-2.c: Likewise.
            * gcc.dg/cpp/warn-unused-macros.c: Likewise.

[Bug c++/66290] wrong location for -Wunused-macros

[lhyatt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66290

Lewis Hyatt <lhyatt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #10 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
Closing since the aforementioned improvement to the macro location is now in
place as well.