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
Confirmed :(.
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.
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.
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>.
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?
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.
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.
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.
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.
(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.
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.
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.
(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.