[Bug c++/111918] New: #pragma GCC diagnostic pop does not restore permerror status of -Wnarrowing

[Bug c++/111918] New: #pragma GCC diagnostic pop does not restore permerror status of -Wnarrowing

[fw at gcc dot gnu.org]

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

            Bug ID: 111918
           Summary: #pragma GCC diagnostic pop does not restore permerror
                    status of -Wnarrowing
           Product: gcc
           Version: 13.2.1
            Status: UNCONFIRMED
          Keywords: diagnostic
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: fw at gcc dot gnu.org
                CC: dmalcolm at gcc dot gnu.org, jason at gcc dot gnu.org
  Target Milestone: ---

Consider this test case:

float f1{123456789};
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wnarrowing"
float f2{123456789};
#pragma GCC diagnostic pop
float f3{123456789};

It reports (with GCC 13.2 and trunk):

t.cc:1:10: error: narrowing conversion of ‘123456789’ from ‘int’ to ‘float’
[-Wnarrowing]
    1 | float f1{123456789};
      |          ^~~~~~~~~
t.cc:6:10: warning: narrowing conversion of ‘123456789’ from ‘int’ to ‘float’
[-Wnarrowing]
    6 | float f3{123456789};
      |          ^~~~~~~~~

I would have expected an error at line 6 because the original diagnostics state
should have been restored by the pop pragma.

This might be a more generic issue, not specific to -Wnarrowing.  I see it with
newly added permerrors in the C front end, too.

[Bug c++/111918] #pragma GCC diagnostic pop does not restore permerror status of -Wnarrowing

[fw at gcc dot gnu.org]

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

--- Comment #1 from Florian Weimer <fw at gcc dot gnu.org> ---
diagnostic_classify_diagnostic overwrites DK_UNSPECIFIED in
context->classify_diagnostic[OPT_Wnarrowing] with DK_WARNING here:

      /* Record the command-line status, so we can reset it back on DK_POP. */
      if (old_kind == DK_UNSPECIFIED)
        {
          old_kind = !context->option_enabled (option_index,
                                               context->lang_mask,
                                               context->option_state)
            ? DK_IGNORED : (context->warning_as_error_requested
                            ? DK_ERROR : DK_WARNING);
          context->classify_diagnostic[option_index] = old_kind;
        }

In diagnostic_enabled, we use that to switch the diagnostic kind to DK_WARNING
because update_effective_level_from_pragmas return DK_UNSPECIFIED:

  /* This tests for #pragma diagnostic changes.  */
  diagnostic_t diag_class
    = update_effective_level_from_pragmas (context, diagnostic);

  /* This tests if the user provided the appropriate -Werror=foo
     option.  */
  if (diag_class == DK_UNSPECIFIED
      && (context->classify_diagnostic[diagnostic->option_index]
          != DK_UNSPECIFIED))
    diagnostic->kind
      = context->classify_diagnostic[diagnostic->option_index];

We cannot treat permerrors differently in either function because they are an
emerging property, not associated with the warning option itself. This fixes
the test case, though:

  /* This tests if the user provided the appropriate -Werror=foo
     option.  */
  if (diag_class == DK_UNSPECIFIED
      && (context->classify_diagnostic[diagnostic->option_index] == DK_ERROR))
    diagnostic->kind = DK_ERROR;

I see what it does to the test suite.

[Bug c++/111918] #pragma GCC diagnostic pop does not restore error status of -Wnarrowing

[fw at gcc dot gnu.org]

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

--- Comment #2 from Florian Weimer <fw at gcc dot gnu.org> ---
It does not work.

I think the problem is the quoted reset code with old_kind.  It was introduced
in r5-2858 to fix PR59304. It's necessary because global warning options serve
a dual purpose: recording the command line state, and activating warning
processing code (or put differently, to skip over analysis for a warning if the
warning is not active).  The diagnostics pragma code has to set global warnings
to disable the shortcuts, but this clobbers the command line state.

[Bug c++/111918] #pragma GCC diagnostic pop does not restore error status of -Wnarrowing

[lhyatt at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |lhyatt at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-11-09
     Ever confirmed|0                           |1

--- Comment #3 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
The below patch should fix it for all such options, I am testing it now.

The old_kind shouldn't demand that the diagnostic must be a warning or an error
specifically, it just needs to note that the diagnostic is enabled or disabled,
and then let the frontend determine what type it is like it normally does.

diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc
index addd6606eaa..99921a10b7b 100644
--- a/gcc/diagnostic.cc
+++ b/gcc/diagnostic.cc
@@ -1126,8 +1126,7 @@ classify_diagnostic (const diagnostic_context *context,
          old_kind = !context->m_option_enabled (option_index,
                                                 context->m_lang_mask,
                                                 context->m_option_state)
-           ? DK_IGNORED : (context->warning_as_error_requested_p ()
-                           ? DK_ERROR : DK_WARNING);
+           ? DK_IGNORED : DK_ANY;
          m_classify_diagnostic[option_index] = old_kind;
        }

@@ -1469,7 +1468,15 @@ diagnostic_context::diagnostic_enabled (diagnostic_info
*diagnostic)
      option.  */
   if (diag_class == DK_UNSPECIFIED
       && !option_unspecified_p (diagnostic->option_index))
-    diagnostic->kind = m_option_classifier.get_current_override
(diagnostic->option_index);
+    {
+      const diagnostic_t new_kind
+       = m_option_classifier.get_current_override (diagnostic->option_index);
+      if (new_kind != DK_ANY)
+       /* DK_ANY means the diagnostic is not to be ignored, but we don't want
+          to change it specifically to DK_ERROR or DK_WARNING; we want to
+          preserve whatever the caller has specified.  */
+       diagnostic->kind = new_kind;
+    }

   /* This allows for future extensions, like temporarily disabling
      warnings for ranges of source code.  */
diff --git a/gcc/diagnostic.def b/gcc/diagnostic.def
index 813b8daa4cc..e889eca7757 100644
--- a/gcc/diagnostic.def
+++ b/gcc/diagnostic.def
@@ -53,3 +53,8 @@ DEFINE_DIAGNOSTIC_KIND (DK_WERROR, "error: ", NULL)
 /* This is like DK_ICE, but backtrace is not printed.  Used in the driver
    when reporting fatal signal in the compiler.  */
 DEFINE_DIAGNOSTIC_KIND (DK_ICE_NOBT, "internal compiler error: ", "error")
+
+/* This is used internally to indicate that a diagnostic is not to be ignored,
+   without mandating it be a specific type, so that it can be an error or
+   warning or otherwise, as the current context requires.  */
+DEFINE_DIAGNOSTIC_KIND (DK_ANY, "", NULL)

[Bug c++/111918] #pragma GCC diagnostic pop does not restore error status of -Wnarrowing

[lhyatt at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch
                URL|                            |https://gcc.gnu.org/piperma
                   |                            |il/gcc-patches/2023-Novembe
                   |                            |r/635871.html

--- Comment #4 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
Patch was submitted for review:
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635871.html

[Bug c++/111918] #pragma GCC diagnostic pop does not restore error status of -Wnarrowing

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dustbingtb at verizon dot net

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 113028 has been marked as a duplicate of this bug. ***

[Bug c++/111918] #pragma GCC diagnostic pop does not restore error status of -Wnarrowing

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |lhyatt at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #6 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Lewis posted a patch.

[Bug c++/111918] #pragma GCC diagnostic pop does not restore error status of -Wnarrowing

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from GCC 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:44ba7bcb752a40ec7490dea53d3a472ce633371d

commit r14-9561-g44ba7bcb752a40ec7490dea53d3a472ce633371d
Author: Lewis Hyatt <lhyatt@gmail.com>
Date:   Wed Nov 8 16:13:14 2023 -0500

    diagnostics: Fix behavior of permerror options after diagnostic pop
[PR111918]

    When a diagnostic pragma changes the classification of a given diagnostic,
    the global options flags (such as warn_narrowing, etc.) may get changed
too.
    Specifically, if a warning was not enabled initially and was later enabled
    by a pragma, then the corresponding global flag will change from false to
    true when the pragma is processed. That change is permanent and is not
    undone by a subsequent `#pragma GCC diagnostic pop'; the warning flag needs
    to remain enabled since a diagnostic could be generated later on for a
    source location prior to the pop.

    So in order to support popping to the initial classification, given that
the
    global options flags no longer reflect that state, the diagnostic_context
    object itself remembers the way things were before it changed anything. The
    current implementation works fine for diagnostics that are always errors or
    always warnings, but it doesn't do the right thing for diagnostics that
    could be either, such as -Wnarrowing. The classification of that diagnostic
    (or any permerror diagnostic) depends on the state of -fpermissive; for the
    particular case of -Wnarrowing it also matters whether a compile-time or
    run-time narrowing is being diagnosed.

    The problem is that the current implementation insists on recording whether
    an enabled diagnostic should be a DK_WARNING or a DK_ERROR, and then, after
    popping to the initial state, it overrides it always to that type only. Fix
    that up by adding a new internal diagnostic type DK_ANY. This just
indicates
    that the diagnostic is enabled without mandating exactly what type of
    diagnostic it should be. Then the diagnostic can be emitted with whatever
    type the frontend asks for.

    Incidentally, while making this change, I noticed that
classify_diagnostic()
    spends some time computing a return value (the old classification kind)
that
    is not used anywhere. The computed value seems to have some problems,
mainly
    that it does not take into account `#pragma GCC diagnostic pop' at all, and
    so the returned value doesn't seem like it could make sense in many
    contexts. Given it would also not be desirable to leak the new
internal-only
    DK_ANY type to outside callers, I think it would make sense in a subsequent
    cleanup patch to remove the return value altogether.

    gcc/ChangeLog:

            PR c++/111918
            * diagnostic-core.h (enum diagnostic_t): Add DK_ANY special flag.
            * diagnostic.cc
(diagnostic_option_classifier::classify_diagnostic):
            Make use of DK_ANY to indicate a diagnostic was initially enabled.
            (diagnostic_context::diagnostic_enabled): Do not change the type of
            a diagnostic if the saved classification is type DK_ANY.

    gcc/testsuite/ChangeLog:

            PR c++/111918
            * g++.dg/cpp0x/Wnarrowing21a.C: New test.
            * g++.dg/cpp0x/Wnarrowing21b.C: New test.
            * g++.dg/cpp0x/Wnarrowing21c.C: New test.
            * g++.dg/cpp0x/Wnarrowing21d.C: New test.

[Bug c++/111918] #pragma GCC diagnostic pop does not restore error status of -Wnarrowing

[lhyatt at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |14.0
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #8 from Lewis Hyatt <lhyatt at gcc dot gnu.org> ---
Fixed for GCC 14.