Bug 80472 - cannot use push/pop with #pragma GCC diagnostic warning "-Wsystem-headers"
Summary: cannot use push/pop with #pragma GCC diagnostic warning "-Wsystem-headers"
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 6.3.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic, easyhack
Depends on:
Blocks: 58876 70692 82745 87614 69769
  Show dependency treegraph
 
Reported: 2017-04-20 12:15 UTC by Jonathan Wakely
Modified: 2019-08-21 04:19 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-04-20 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2017-04-20 12:15:56 UTC
Given the following header:

#pragma GCC system_header
#pragma GCC diagnostic push
#pragma GCC diagnostic warning "-Wsystem-headers"
// want warnings here despite being in a system header
#pragma GCC diagnostic pop
// But don't want warnings here
int missing_return() { }


A file including it will warn with -Wreturn-type (or -Wall):

In file included from dep.cc:1:0:
dep.h: In function ‘int missing_return()’:
dep.h:8:24: warning: no return statement in function returning non-void [-Wreturn-type]
 int missing_return() { }
                        ^

The warning should be disabled again by the "pop", but -Wsystem-headers stays active once enabled.

This makes it very difficult for libstdc++ to selectively enable important warnings, e.g. Bug 58876
Comment 1 Marek Polacek 2017-04-20 12:28:21 UTC
Confirmed :(.
Comment 2 Manuel López-Ibáñez 2017-08-19 12:10:59 UTC
Currently, the macro controlling this only looks at the state of the option:

/* Returns nonzero if warnings should be emitted.  */
#define diagnostic_report_warnings_p(DC, LOC)                           \
  (!(DC)->dc_inhibit_warnings                                           \
   && !(in_system_header_at (LOC) && !(DC)->dc_warn_system_headers))

It needs to look at the classification_history like diagnostic_report_diagnostic() does.
Comment 3 Manuel López-Ibáñez 2017-08-19 12:20:05 UTC
It seems pretty straight-forward:

1. Abstract out the check for classification_history.

2. Create a function diagnostic_report_warnings_p that checks the classification history of Wsystem-headers in addition to the option value.

Not that -Wsystem-headers is not a typical warning option, so more work would be needed to have

#pragma GCC diagnostic error "-Wsystem-headers"

give errors instead  of warnings

or

-Werror
#pragma GCC diagnostic warning "-Wsystem-headers"

give warnings instead of errors. But that seems less important to fix.
Comment 4 Jonathan Wakely 2019-03-22 15:15:21 UTC
Another place where I'd like to selectively enable warnings is for this code (from PR 78830):

#include <forward_list>
#include <algorithm>

int main()
{
    std::forward_list<int> il = {1, 2, 3, 4, 5, 6, 7}; 
    auto iter = std::prev(il.end());
}


With -Wall -O1 -Wsystem-headers GCC detects the undefined behaviour:

In file included from /home/jwakely/gcc/9/include/c++/9.0.1/bits/stl_algobase.h:66,
                 from /home/jwakely/gcc/9/include/c++/9.0.1/bits/forward_list.h:38,
                 from /home/jwakely/gcc/9/include/c++/9.0.1/forward_list:38,
                 from prev.cc:1:
/home/jwakely/gcc/9/include/c++/9.0.1/bits/stl_iterator_base_funcs.h:153:7: warning: iteration 9223372036854775807 invokes undefined behavior [-Waggressive-loop-optimizations]
  153 |       while (__n--)
      |       ^~~~~
/home/jwakely/gcc/9/include/c++/9.0.1/bits/stl_iterator_base_funcs.h:153:7: note: within this loop

but that very useful warning is suppressed by default. This bug prevents me from locally enabling -Wsystem-headers around the relevant functions in <bits/stl_iterator_base_funcs.h>.
Comment 5 Manuel López-Ibáñez 2019-03-22 20:25:32 UTC
This warning will be incomprehensible to users because the warning never
mentions any code that the user can modify. What should the user do
according to the warning?
Comment 6 Jonathan Wakely 2019-03-22 21:16:26 UTC
Is it better to silently generate code with undefined behaviour, or issue a flawed warning about that undefined behaviour?

If the warning doesn't show the template instantiation context that's a separate issue.
Comment 7 Jonathan Wakely 2019-03-22 21:21:50 UTC
A comment added to the code would make the caret diagnostic self-explanatory:

--- a/libstdc++-v3/include/bits/stl_iterator_base_funcs.h
+++ b/libstdc++-v3/include/bits/stl_iterator_base_funcs.h
@@ -149,7 +149,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
       // concept requirements
       __glibcxx_function_requires(_InputIteratorConcept<_InputIterator>)
       __glibcxx_assert(__n >= 0);
-      while (__n--)
+      while (__n--) // if n is negative this has undefined behaviour
        ++__i;
     }

That would make the diagnostic look like:

/home/jwakely/gcc/9/include/c++/9.0.1/bits/stl_iterator_base_funcs.h:152:7: warning: iteration 9223372036854775807 invokes undefined behavior [-Waggressive-loop-optimizations]
  152 |       while (__n--) // if n is negative this has undefined behaviour
      |     


It's still a bug that there's no "In instantiation of ... required from ... required from here" context shown though.
Comment 8 Manuel López-Ibáñez 2019-03-22 21:57:56 UTC
There is no negative n__ in user code.

On Fri, 22 Mar 2019, 21:21 redi at gcc dot gnu.org, <
gcc-bugzilla@gcc.gnu.org> wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80472
>
> --- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> A comment added to the code would make the caret diagnostic
> self-explanatory:
>
> --- a/libstdc++-v3/include/bits/stl_iterator_base_funcs.h
> +++ b/libstdc++-v3/include/bits/stl_iterator_base_funcs.h
> @@ -149,7 +149,7 @@ _GLIBCXX_END_NAMESPACE_CONTAINER
>        // concept requirements
>        __glibcxx_function_requires(_InputIteratorConcept<_InputIterator>)
>        __glibcxx_assert(__n >= 0);
> -      while (__n--)
> +      while (__n--) // if n is negative this has undefined behaviour
>         ++__i;
>      }
>
> That would make the diagnostic look like:
>
> /home/jwakely/gcc/9/include/c++/9.0.1/bits/stl_iterator_base_funcs.h:152:7:
> warning: iteration 9223372036854775807 invokes undefined behavior
> [-Waggressive-loop-optimizations]
>   152 |       while (__n--) // if n is negative this has undefined
> behaviour
>       |
>
>
> It's still a bug that there's no "In instantiation of ... required from ...
> required from here" context shown though.
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 9 Jonathan Wakely 2019-03-22 22:16:12 UTC
There is though. std::prev(it, n) is specified as std::advance(it, -n). Calling prev means advancing a negative amount.

But I'm not sure what your point is. Currently there's no warning by default even though the compiler has detected UB. Are you saying it is better to not warn at all, and just have silent UB?

If you want the warning to be improved, please open a separate bug report. This one is about the inability to enable warnings in system headers, not the quality of individual warnings.
Comment 10 Jonathan Wakely 2019-03-22 22:18:23 UTC
(In reply to Manuel López-Ibáñez from comment #8)
> There is no negative n__ in user code.

If you want to be pedantic, there's no __n at all in user code. Because it's a function parameter of std::advance. But clearly if the compiler says "undefined behaviour detected at this line" and the line has a comment saying "undefined if n is negative" then the user can figure out that the function got called with negative n.

Is that perfect? No. Is it better than printing nothing when UB is detected? To me the answer is obviously yes, so it seems like you're just objecting to making improvements.
Comment 11 Manuel López-Ibáñez 2019-03-22 23:02:06 UTC
I'm not being pedantic for the sake of being pedantic. It is trivial to fix
the #pragma as I explained above. However, that won't give the user any
idea about which user code is triggering the warning. For that, we need to
have at the point of warning a token that comes from user code or we need
to propagate a user location to the tokens that made up this particular
instantation of the template, so that the diagnostic code can windup the
location stack and find the user code that triggered the warning.


On Fri, 22 Mar 2019, 22:18 redi at gcc dot gnu.org, <
gcc-bugzilla@gcc.gnu.org> wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80472
>
> --- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> (In reply to Manuel López-Ibáñez from comment #8)
> > There is no negative n__ in user code.
>
> If you want to be pedantic, there's no __n at all in user code. Because
> it's a
> function parameter of std::advance. But clearly if the compiler says
> "undefined
> behaviour detected at this line" and the line has a comment saying
> "undefined
> if n is negative" then the user can figure out that the function got called
> with negative n.
>
> Is that perfect? No. Is it better than printing nothing when UB is
> detected? To
> me the answer is obviously yes, so it seems like you're just objecting to
> making improvements.
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 12 Jonathan Wakely 2019-03-22 23:22:28 UTC
And as I've already said, the quality of the particular -Waggressive-loop-optimizations warning is a separate issue, and should be dealt with in a separate PR.

PR 58876 (mentioned in comment 0 as the reason for creating this bug) shows a proper instantiation trace when -Wsystem-headers is used:

In file included from /home/jwakely/gcc/9/include/c++/9.0.1/memory:80,
                 from up.cc:1:
/home/jwakely/gcc/9/include/c++/9.0.1/bits/unique_ptr.h: In instantiation of 'void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = A]':
/home/jwakely/gcc/9/include/c++/9.0.1/bits/unique_ptr.h:289:17:   required from 'std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = A; _Dp = std::default_delete<A>]'
up.cc:12:30:   required from here
/home/jwakely/gcc/9/include/c++/9.0.1/bits/unique_ptr.h:81:2: warning: deleting object of abstract class type 'A' which has non-virtual destructor will cause undefined behavior [-Wdelete-non-virtual-dtor]
   81 |  delete __ptr;
      |  ^~~~~~

That shows the up.cc:12 location from the user code that triggers the warning.

But I can't make libstdc++ show that warning because of this bug with the diagnostic pragmas.

Improving the warning in comment 4 is irrelevant to this bug.
Comment 13 Jonathan Wakely 2019-03-22 23:52:03 UTC
(In reply to Jonathan Wakely from comment #12)
> Improving the warning in comment 4 is irrelevant to this bug.

I've created Bug 89800 for improving that warning, please move that discussion there.