Re: [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
On Thu, 2021-01-21 at 16:46 -0700, Martin Sebor via Gcc-patches wrote: Martin and I had a chat about this patch, but it's best to discuss code on the mailing list rather than in a silo, so here goes... > The initial patch I posted is missing initialization for a couple > of locals. I'd noticed it in testing but forgot to add the fix to > the patch before posting it. I have corrected that in the updated > revision and also added the test case from pr98512, and retested > the whole thing on x86_64-linux. > > On 1/19/21 11:58 AM, Martin Sebor wrote: > > std::string tends to trigger a class of false positive out of bounds > > access warnings for code GCC cannot prove is unreachable because of > > missing aliasing constrains, and that ends up expanded inline into > > user code. Simply inserting the contents of a constant char array > > does that. In GCC 10 these false positives are suppressed due to > > -Wno-system-headers, but in GCC 11, to help detect calls rendered > > invalid by user code passing in either incorrect or insufficiently > > constrained arguments, -Wno-system-header no longer has this effect > > on invalid access warnings. > > > > To solve the problem without at least partially reverting the change > > and going back to the GCC 10 way of things for the affected subset > > of calls (just memcpy and memmove), the attached patch enhances > > the #pragma GCC diagnostic machinery to consider not just a single > > location for inlined code but all locations at which an expression > > and its callers are inlined all the way up the stack. This gives > > each author of a function involved in inlining the ability to > > control a warning issued for the code, not just the user into whose > > code all the calls end up inlined. To resolve PR 98465, it lets us > > suppress the false positives selectively in std::string rather > > than across the board in GCC. I like the idea of checking the whole of the inlining stack for pragmas, but I don't like the way the patch implements it. The patch provides a hook for getting a vec of locations for a diagnostic for use when checking for pragmas, and uses it the hook on a few specific diagnostics. Why wouldn’t we always do this? It seems to me like something we should always do when there's inlining information associated with a location, rather than being a per-diagnostic thing - but maybe there's something I'm missing here. The patch adds diag_inlining_context instances on the stack in various places, and doing that feels to me like a special-case hack, when it should be fixed more generally in diagnostics.c I don't like attaching the "override the location" hook to the diagnostic_metadata; I intended the latter to be about diagnostic taxonomies (CWE, coding standards, etc), rather than a place to stash location overrides. One issue is that the core of the diagnostics subsystem doesn't have knowledge of "tree", but the inlining information requires tree-ish knowledge. It's still possible to get at the inlining information from the diagnostics.c, but only as a void *: input.h's has: #define LOCATION_BLOCK(LOC) \ ((tree) ((IS_ADHOC_LOC (LOC)) ? get_data_from_adhoc_loc (line_table, (LOC)) \ : NULL)) Without knowing about "tree", diagnostic.c could still query a location_t to get at the data as a void *: if (IS_ADHOC_LOC (loc) return get_data_from_adhoc_loc (line_table, loc); else return NULL; If we make the "get all the pertinent locations" hook a part of the diagnostic_context, we could have diagnostic_report_diagnostic check to see if there's ad-hoc data associated with the location and a non-NULL hook on the context, and if so, call it. This avoids adding an indirect call for the common case where there isn't any inlining information, and lets us stash the implementation of the hook in the tree-diagnostic.c, keeping the separation of trees from diagnostic.c One design question here is: what if there are multiple pragmas on the inlining stack, e.g. explicitly enabling a warning at one place, and explicitly ignoring that warning in another? I don't think it would happen in the cases you're interested in, but it seems worth considering. Perhaps the closest place to the user's code "wins". > > > > The solution is to provide a new pair of overloads for warning > > functions that, instead of taking a single location argument, take > > a tree node from which the location(s) are determined. The tree > > argument is indirect because the diagnostic machinery doesn't (and > > cannot without more intrusive changes) at the moment depend on > > the various tree definitions. A nice feature of these overloads > > is that they do away with the need for the %K directive (and in > > the future also %G, with another enhancement to accept a gimple* > > argument). We were chatting about this, and I believe we don't need the new overloads or the %K and %G directives: all of the information about inlining can be got from the location_t
Re: [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
* Jeff Law via Gcc-patches: > I'd lean towards deferring to gcc12 stage1 given the libstdc++ hack is > in place. That does mean that glibc will need to work around the > instance they've stumbled over in ppc's rawmemchr. We'll need to work around this in the glibc build, too. I'll check if the suggested alternative (have the alias covered by the pragma) works there as well. Thanks, Florian
Re: [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
On 1/19/21 11:58 AM, Martin Sebor via Gcc-patches wrote: > std::string tends to trigger a class of false positive out of bounds > access warnings for code GCC cannot prove is unreachable because of > missing aliasing constrains, and that ends up expanded inline into > user code. Simply inserting the contents of a constant char array > does that. In GCC 10 these false positives are suppressed due to > -Wno-system-headers, but in GCC 11, to help detect calls rendered > invalid by user code passing in either incorrect or insufficiently > constrained arguments, -Wno-system-header no longer has this effect > on invalid access warnings. > > To solve the problem without at least partially reverting the change > and going back to the GCC 10 way of things for the affected subset > of calls (just memcpy and memmove), the attached patch enhances > the #pragma GCC diagnostic machinery to consider not just a single > location for inlined code but all locations at which an expression > and its callers are inlined all the way up the stack. This gives > each author of a function involved in inlining the ability to > control a warning issued for the code, not just the user into whose > code all the calls end up inlined. To resolve PR 98465, it lets us > suppress the false positives selectively in std::string rather > than across the board in GCC. > > The solution is to provide a new pair of overloads for warning > functions that, instead of taking a single location argument, take > a tree node from which the location(s) are determined. The tree > argument is indirect because the diagnostic machinery doesn't (and > cannot without more intrusive changes) at the moment depend on > the various tree definitions. A nice feature of these overloads > is that they do away with the need for the %K directive (and in > the future also %G, with another enhancement to accept a gimple* > argument). > > This patch depends on the fix for PR 98664 (already approved but > not yet checked in). I've tested it on x86_64-linux. > > To avoid fallout I tried to keep the changes to a minimum, and > so the design isn't as robust as I'd like it ultimately to be. > I plan to enhance it in stage 1. I'd lean towards deferring to gcc12 stage1 given the libstdc++ hack is in place. That does mean that glibc will need to work around the instance they've stumbled over in ppc's rawmemchr. If there are other BZs where this would help our ability to "cleanly" suppress diagnosics with our pragmas, then we probably should link them together with 98512/98465 and defer them all to gcc-12. Jeff
PING 3 [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
Ping 3: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564060.html I submitted this as a fix for a fair number of false positives reported by Fedora package maintainers. Last week Jakub committed r11-7146, which is an alternate workaround for the same problem, but one isolated to libstdc++. That might make this patch less pressing but not less relevant since it also fixes pr98512 (a bug impacting Glibc) and adds the infrastructure for resolving pr98871 and other bugs about the #pragma diagnostic's inability to suppress warnings in inlined code. Jeff (or anyone else who cares about this) if you consider this patch too intrusive at this point let me know and I'll stop pinging it and resubmit it for GCC 12. On 2/6/21 10:12 AM, Martin Sebor wrote: Ping 2: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564060.html On 1/29/21 7:56 PM, Martin Sebor wrote: Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564060.html On 1/21/21 4:46 PM, Martin Sebor wrote: The initial patch I posted is missing initialization for a couple of locals. I'd noticed it in testing but forgot to add the fix to the patch before posting it. I have corrected that in the updated revision and also added the test case from pr98512, and retested the whole thing on x86_64-linux. On 1/19/21 11:58 AM, Martin Sebor wrote: std::string tends to trigger a class of false positive out of bounds access warnings for code GCC cannot prove is unreachable because of missing aliasing constrains, and that ends up expanded inline into user code. Simply inserting the contents of a constant char array does that. In GCC 10 these false positives are suppressed due to -Wno-system-headers, but in GCC 11, to help detect calls rendered invalid by user code passing in either incorrect or insufficiently constrained arguments, -Wno-system-header no longer has this effect on invalid access warnings. To solve the problem without at least partially reverting the change and going back to the GCC 10 way of things for the affected subset of calls (just memcpy and memmove), the attached patch enhances the #pragma GCC diagnostic machinery to consider not just a single location for inlined code but all locations at which an expression and its callers are inlined all the way up the stack. This gives each author of a function involved in inlining the ability to control a warning issued for the code, not just the user into whose code all the calls end up inlined. To resolve PR 98465, it lets us suppress the false positives selectively in std::string rather than across the board in GCC. The solution is to provide a new pair of overloads for warning functions that, instead of taking a single location argument, take a tree node from which the location(s) are determined. The tree argument is indirect because the diagnostic machinery doesn't (and cannot without more intrusive changes) at the moment depend on the various tree definitions. A nice feature of these overloads is that they do away with the need for the %K directive (and in the future also %G, with another enhancement to accept a gimple* argument). This patch depends on the fix for PR 98664 (already approved but not yet checked in). I've tested it on x86_64-linux. To avoid fallout I tried to keep the changes to a minimum, and so the design isn't as robust as I'd like it ultimately to be. I plan to enhance it in stage 1. Martin
PING 2 [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
Ping 2: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564060.html On 1/29/21 7:56 PM, Martin Sebor wrote: Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564060.html On 1/21/21 4:46 PM, Martin Sebor wrote: The initial patch I posted is missing initialization for a couple of locals. I'd noticed it in testing but forgot to add the fix to the patch before posting it. I have corrected that in the updated revision and also added the test case from pr98512, and retested the whole thing on x86_64-linux. On 1/19/21 11:58 AM, Martin Sebor wrote: std::string tends to trigger a class of false positive out of bounds access warnings for code GCC cannot prove is unreachable because of missing aliasing constrains, and that ends up expanded inline into user code. Simply inserting the contents of a constant char array does that. In GCC 10 these false positives are suppressed due to -Wno-system-headers, but in GCC 11, to help detect calls rendered invalid by user code passing in either incorrect or insufficiently constrained arguments, -Wno-system-header no longer has this effect on invalid access warnings. To solve the problem without at least partially reverting the change and going back to the GCC 10 way of things for the affected subset of calls (just memcpy and memmove), the attached patch enhances the #pragma GCC diagnostic machinery to consider not just a single location for inlined code but all locations at which an expression and its callers are inlined all the way up the stack. This gives each author of a function involved in inlining the ability to control a warning issued for the code, not just the user into whose code all the calls end up inlined. To resolve PR 98465, it lets us suppress the false positives selectively in std::string rather than across the board in GCC. The solution is to provide a new pair of overloads for warning functions that, instead of taking a single location argument, take a tree node from which the location(s) are determined. The tree argument is indirect because the diagnostic machinery doesn't (and cannot without more intrusive changes) at the moment depend on the various tree definitions. A nice feature of these overloads is that they do away with the need for the %K directive (and in the future also %G, with another enhancement to accept a gimple* argument). This patch depends on the fix for PR 98664 (already approved but not yet checked in). I've tested it on x86_64-linux. To avoid fallout I tried to keep the changes to a minimum, and so the design isn't as robust as I'd like it ultimately to be. I plan to enhance it in stage 1. Martin
PING [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-January/564060.html On 1/21/21 4:46 PM, Martin Sebor wrote: The initial patch I posted is missing initialization for a couple of locals. I'd noticed it in testing but forgot to add the fix to the patch before posting it. I have corrected that in the updated revision and also added the test case from pr98512, and retested the whole thing on x86_64-linux. On 1/19/21 11:58 AM, Martin Sebor wrote: std::string tends to trigger a class of false positive out of bounds access warnings for code GCC cannot prove is unreachable because of missing aliasing constrains, and that ends up expanded inline into user code. Simply inserting the contents of a constant char array does that. In GCC 10 these false positives are suppressed due to -Wno-system-headers, but in GCC 11, to help detect calls rendered invalid by user code passing in either incorrect or insufficiently constrained arguments, -Wno-system-header no longer has this effect on invalid access warnings. To solve the problem without at least partially reverting the change and going back to the GCC 10 way of things for the affected subset of calls (just memcpy and memmove), the attached patch enhances the #pragma GCC diagnostic machinery to consider not just a single location for inlined code but all locations at which an expression and its callers are inlined all the way up the stack. This gives each author of a function involved in inlining the ability to control a warning issued for the code, not just the user into whose code all the calls end up inlined. To resolve PR 98465, it lets us suppress the false positives selectively in std::string rather than across the board in GCC. The solution is to provide a new pair of overloads for warning functions that, instead of taking a single location argument, take a tree node from which the location(s) are determined. The tree argument is indirect because the diagnostic machinery doesn't (and cannot without more intrusive changes) at the moment depend on the various tree definitions. A nice feature of these overloads is that they do away with the need for the %K directive (and in the future also %G, with another enhancement to accept a gimple* argument). This patch depends on the fix for PR 98664 (already approved but not yet checked in). I've tested it on x86_64-linux. To avoid fallout I tried to keep the changes to a minimum, and so the design isn't as robust as I'd like it ultimately to be. I plan to enhance it in stage 1. Martin
Re: [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
The initial patch I posted is missing initialization for a couple
of locals. I'd noticed it in testing but forgot to add the fix to
the patch before posting it. I have corrected that in the updated
revision and also added the test case from pr98512, and retested
the whole thing on x86_64-linux.
On 1/19/21 11:58 AM, Martin Sebor wrote:
std::string tends to trigger a class of false positive out of bounds
access warnings for code GCC cannot prove is unreachable because of
missing aliasing constrains, and that ends up expanded inline into
user code. Simply inserting the contents of a constant char array
does that. In GCC 10 these false positives are suppressed due to
-Wno-system-headers, but in GCC 11, to help detect calls rendered
invalid by user code passing in either incorrect or insufficiently
constrained arguments, -Wno-system-header no longer has this effect
on invalid access warnings.
To solve the problem without at least partially reverting the change
and going back to the GCC 10 way of things for the affected subset
of calls (just memcpy and memmove), the attached patch enhances
the #pragma GCC diagnostic machinery to consider not just a single
location for inlined code but all locations at which an expression
and its callers are inlined all the way up the stack. This gives
each author of a function involved in inlining the ability to
control a warning issued for the code, not just the user into whose
code all the calls end up inlined. To resolve PR 98465, it lets us
suppress the false positives selectively in std::string rather
than across the board in GCC.
The solution is to provide a new pair of overloads for warning
functions that, instead of taking a single location argument, take
a tree node from which the location(s) are determined. The tree
argument is indirect because the diagnostic machinery doesn't (and
cannot without more intrusive changes) at the moment depend on
the various tree definitions. A nice feature of these overloads
is that they do away with the need for the %K directive (and in
the future also %G, with another enhancement to accept a gimple*
argument).
This patch depends on the fix for PR 98664 (already approved but
not yet checked in). I've tested it on x86_64-linux.
To avoid fallout I tried to keep the changes to a minimum, and
so the design isn't as robust as I'd like it ultimately to be.
I plan to enhance it in stage 1.
Martin
PR middle-end/98465 - Bogus -Wstringop-overread in std::string
PR middle-end/98512 - “#pragma GCC diagnostic ignored” ineffective in conjunction with alias attribute
gcc/ChangeLog:
PR middle-end/98465
PR middle-end/98512
* builtins.c (class diag_inlining_context): New class.
(maybe_warn_for_bound): Adjust signature. Use diag_inlining_context.
(warn_for_access): Same.
(check_access): Remove calls to tree_inlined_location.
(expand_builtin_strncmp): Remove argument from calls to
maybe_warn_for_bound.
(warn_dealloc_offset): Adjust signature. Use diag_inlining_context.
(maybe_emit_free_warning): Remove calls to tree_inlined_location.
* diagnostic-core.h (warning, warning_n): New overloads.
* diagnostic-metadata.h (class diagnostic_metadata::location_context):
New.
(struct diagnostic_info): Declare.
* diagnostic.c (location_context::locations): Define.
(update_effective_level_from_pragmas): Use location_context to test
inlinined locations.
(diagnostic_report_diagnostic): Set location context.
(warning, warning_n): Define new overloads.
* diagnostic.h (diagnostic_inhibit_notes):
gcc/cp/ChangeLog:
* mapper-client.cc: Include headers needed by others.
libstdc++-v3/ChangeLog:
PR middle-end/98465
* include/bits/basic_string.tcc (_M_replace): Suppress false positive
warnings.
* testsuite/18_support/new_delete_placement.cc: Suppress valid warnings.
* testsuite/20_util/monotonic_buffer_resource/allocate.cc: Same.
* testsuite/20_util/unsynchronized_pool_resource/allocate.cc: Same.
gcc/testsuite/ChangeLog:
PR middle-end/98512
* gcc.dg/pragma-diag-9.c: New test.
* gcc.dg/pragma-diag-10.c: New test.
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 0aed008687c..39fe1d0a6e0 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -39,7 +39,7 @@ along with GCC; see the file COPYING3. If not see
#include "optabs.h"
#include "emit-rtl.h"
#include "recog.h"
-#include "diagnostic-core.h"
+#include "diagnostic.h"
#include "alias.h"
#include "fold-const.h"
#include "fold-const-call.h"
@@ -79,6 +79,7 @@ along with GCC; see the file COPYING3. If not see
#include "tree-outof-ssa.h"
#include "attr-fnspec.h"
#include "demangle.h"
+#include "tree-pretty-print.h"
struct target_builtins default_target_builtins;
#if SWITCHABLE_TARGET
@@ -749,6 +750,93 @@ is_builtin_name (const char *name)
return false;
}
+/* Class to override the base location context for an expression EXPR. */
+
+class diag_inlining_context: public diagnostic_metadata::location_context
+{
+ public:
+ diag_inlining_context (tree expr):
Re: [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
On 1/21/21 12:01 PM, Florian Weimer wrote:
* Martin Sebor:
On 1/21/21 10:34 AM, Florian Weimer wrote:
* Martin Sebor via Gcc-patches:
This patch depends on the fix for PR 98664 (already approved but
not yet checked in). I've tested it on x86_64-linux.
To avoid fallout I tried to keep the changes to a minimum, and
so the design isn't as robust as I'd like it ultimately to be.
I plan to enhance it in stage 1.
I've tested this patch on top of 43705f3fa343e08b2fb030460f (so with
the
PR98664 fix, I think), and the reproducer from PR98512 now ICEs:
Thanks for giving it a try! I saw a similar ICE during my testing
-- it's caused by a couple of uninitialized variables. I fixed
it in my tree (see below) but the fix didn't make it into the patch.
Please give this a try and let me know if it doesn't help:
index abcd991b829..d82a7eb67e5 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -1426,7 +1426,7 @@ diagnostic_impl (rich_location *richloc, const
diagnostic_metadata *metadata,
int opt, const char *gmsgid,
va_list *ap, diagnostic_t kind)
{
- diagnostic_info diagnostic;
+ diagnostic_info diagnostic{ };
if (kind == DK_PERMERROR)
{
diagnostic_set_info (, gmsgid, ap, richloc,
@@ -1452,7 +1452,7 @@ diagnostic_n_impl (rich_location *richloc, const
diagnostic_metadata *metadata,
const char *plural_gmsgid,
va_list *ap, diagnostic_t kind)
{
- diagnostic_info diagnostic;
+ diagnostic_info diagnostic{ };
unsigned long gtn;
if (sizeof n <= sizeof gtn)
This fixes the crash for me, and the warnings is gone as well.
Great, thanks for the confirmation! I'll post the updated patch
shortly.
Martin
Thanks,
Florian
Re: [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
* Martin Sebor:
> On 1/21/21 10:34 AM, Florian Weimer wrote:
>> * Martin Sebor via Gcc-patches:
>>
>>> This patch depends on the fix for PR 98664 (already approved but
>>> not yet checked in). I've tested it on x86_64-linux.
>>>
>>> To avoid fallout I tried to keep the changes to a minimum, and
>>> so the design isn't as robust as I'd like it ultimately to be.
>>> I plan to enhance it in stage 1.
>> I've tested this patch on top of 43705f3fa343e08b2fb030460f (so with
>> the
>> PR98664 fix, I think), and the reproducer from PR98512 now ICEs:
>
> Thanks for giving it a try! I saw a similar ICE during my testing
> -- it's caused by a couple of uninitialized variables. I fixed
> it in my tree (see below) but the fix didn't make it into the patch.
>
> Please give this a try and let me know if it doesn't help:
>
> index abcd991b829..d82a7eb67e5 100644
> --- a/gcc/diagnostic.c
> +++ b/gcc/diagnostic.c
> @@ -1426,7 +1426,7 @@ diagnostic_impl (rich_location *richloc, const
> diagnostic_metadata *metadata,
> int opt, const char *gmsgid,
> va_list *ap, diagnostic_t kind)
> {
> - diagnostic_info diagnostic;
> + diagnostic_info diagnostic{ };
>if (kind == DK_PERMERROR)
> {
>diagnostic_set_info (, gmsgid, ap, richloc,
> @@ -1452,7 +1452,7 @@ diagnostic_n_impl (rich_location *richloc, const
> diagnostic_metadata *metadata,
>const char *plural_gmsgid,
>va_list *ap, diagnostic_t kind)
> {
> - diagnostic_info diagnostic;
> + diagnostic_info diagnostic{ };
>unsigned long gtn;
>
>if (sizeof n <= sizeof gtn)
This fixes the crash for me, and the warnings is gone as well.
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
Re: [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
On 1/21/21 10:34 AM, Florian Weimer wrote:
* Martin Sebor via Gcc-patches:
This patch depends on the fix for PR 98664 (already approved but
not yet checked in). I've tested it on x86_64-linux.
To avoid fallout I tried to keep the changes to a minimum, and
so the design isn't as robust as I'd like it ultimately to be.
I plan to enhance it in stage 1.
I've tested this patch on top of 43705f3fa343e08b2fb030460f (so with the
PR98664 fix, I think), and the reproducer from PR98512 now ICEs:
Thanks for giving it a try! I saw a similar ICE during my testing
-- it's caused by a couple of uninitialized variables. I fixed
it in my tree (see below) but the fix didn't make it into the patch.
Please give this a try and let me know if it doesn't help:
index abcd991b829..d82a7eb67e5 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -1426,7 +1426,7 @@ diagnostic_impl (rich_location *richloc, const
diagnostic_metadata *metadata,
int opt, const char *gmsgid,
va_list *ap, diagnostic_t kind)
{
- diagnostic_info diagnostic;
+ diagnostic_info diagnostic{ };
if (kind == DK_PERMERROR)
{
diagnostic_set_info (, gmsgid, ap, richloc,
@@ -1452,7 +1452,7 @@ diagnostic_n_impl (rich_location *richloc, const
diagnostic_metadata *metadata,
const char *plural_gmsgid,
va_list *ap, diagnostic_t kind)
{
- diagnostic_info diagnostic;
+ diagnostic_info diagnostic{ };
unsigned long gtn;
if (sizeof n <= sizeof gtn)
Martin
void *
__rawmemchr_ppc (const void *s, int c)
{
#pragma GCC diagnostics push
#pragma GCC diagnostic ignored "-Wstringop-overflow="
#pragma GCC diagnostic ignored "-Wstringop-overread"
if (c != 0)
return __builtin_memchr (s, c, (unsigned long)-1);
#pragma GCC diagnostics pop
return (char *)s + __builtin_strlen (s);
}
extern __typeof (__rawmemchr_ppc) __EI___rawmemchr_ppc
__attribute__((alias ("__rawmemchr_ppc")));
during RTL pass: expand
t.c: In function ‘__rawmemchr_ppc’:
t.c:8:12: internal compiler error: Segmentation fault
8 | return __builtin_memchr (s, c, (unsigned long)-1);
|^~
0xde134f crash_signal
/home/bmg/src/gcc/gcc/toplev.c:327
0x9181bd diag_inlining_context::set_locations(vec*, diagnostic_info*)
/home/bmg/src/gcc/gcc/builtins.c:835
0x17ce2da update_effective_level_from_pragmas
/home/bmg/src/gcc/gcc/diagnostic.c:1028
0x17ce2da diagnostic_report_diagnostic(diagnostic_context*, diagnostic_info*)
/home/bmg/src/gcc/gcc/diagnostic.c:1218
0x17ceb1e diagnostic_impl
/home/bmg/src/gcc/gcc/diagnostic.c:1443
0x17cf144 warning(diagnostic_metadata::location_context&, int, char const*, ...)
/home/bmg/src/gcc/gcc/diagnostic.c:1669
0x917ab0 maybe_warn_for_bound
/home/bmg/src/gcc/gcc/builtins.c:4077
0x927eee maybe_warn_for_bound
/home/bmg/src/gcc/gcc/builtins.c:4920
0x927eee check_access(tree_node*, tree_node*, tree_node*, tree_node*,
tree_node*, access_mode, access_data const*)
/home/bmg/src/gcc/gcc/builtins.c:4918
0x928b52 check_read_access
/home/bmg/src/gcc/gcc/builtins.c:4996
0x92e19b check_read_access
/home/bmg/src/gcc/gcc/builtins.c:9992
0x92e19b expand_builtin_memchr
/home/bmg/src/gcc/gcc/builtins.c:5926
0x92e19b expand_builtin(tree_node*, rtx_def*, rtx_def*, machine_mode, int)
/home/bmg/src/gcc/gcc/builtins.c:9992
0xa6d714 expand_expr_real_1(tree_node*, rtx_def*, machine_mode,
expand_modifier, rtx_def**, bool)
/home/bmg/src/gcc/gcc/expr.c:11275
0xa796fb store_expr(tree_node*, rtx_def*, int, bool, bool)
/home/bmg/src/gcc/gcc/expr.c:5885
0xa7ab71 expand_assignment(tree_node*, tree_node*, bool)
/home/bmg/src/gcc/gcc/expr.c:5621
0x9569bb expand_call_stmt
/home/bmg/src/gcc/gcc/cfgexpand.c:2837
0x9569bb expand_gimple_stmt_1
/home/bmg/src/gcc/gcc/cfgexpand.c:3843
0x9569bb expand_gimple_stmt
/home/bmg/src/gcc/gcc/cfgexpand.c:4007
0x95c60f expand_gimple_basic_block
/home/bmg/src/gcc/gcc/cfgexpand.c:6044
Thanks,
Florian
Re: [PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
* Martin Sebor via Gcc-patches:
> This patch depends on the fix for PR 98664 (already approved but
> not yet checked in). I've tested it on x86_64-linux.
>
> To avoid fallout I tried to keep the changes to a minimum, and
> so the design isn't as robust as I'd like it ultimately to be.
> I plan to enhance it in stage 1.
I've tested this patch on top of 43705f3fa343e08b2fb030460f (so with the
PR98664 fix, I think), and the reproducer from PR98512 now ICEs:
void *
__rawmemchr_ppc (const void *s, int c)
{
#pragma GCC diagnostics push
#pragma GCC diagnostic ignored "-Wstringop-overflow="
#pragma GCC diagnostic ignored "-Wstringop-overread"
if (c != 0)
return __builtin_memchr (s, c, (unsigned long)-1);
#pragma GCC diagnostics pop
return (char *)s + __builtin_strlen (s);
}
extern __typeof (__rawmemchr_ppc) __EI___rawmemchr_ppc
__attribute__((alias ("__rawmemchr_ppc")));
during RTL pass: expand
t.c: In function ‘__rawmemchr_ppc’:
t.c:8:12: internal compiler error: Segmentation fault
8 | return __builtin_memchr (s, c, (unsigned long)-1);
|^~
0xde134f crash_signal
/home/bmg/src/gcc/gcc/toplev.c:327
0x9181bd diag_inlining_context::set_locations(vec*, diagnostic_info*)
/home/bmg/src/gcc/gcc/builtins.c:835
0x17ce2da update_effective_level_from_pragmas
/home/bmg/src/gcc/gcc/diagnostic.c:1028
0x17ce2da diagnostic_report_diagnostic(diagnostic_context*, diagnostic_info*)
/home/bmg/src/gcc/gcc/diagnostic.c:1218
0x17ceb1e diagnostic_impl
/home/bmg/src/gcc/gcc/diagnostic.c:1443
0x17cf144 warning(diagnostic_metadata::location_context&, int, char const*, ...)
/home/bmg/src/gcc/gcc/diagnostic.c:1669
0x917ab0 maybe_warn_for_bound
/home/bmg/src/gcc/gcc/builtins.c:4077
0x927eee maybe_warn_for_bound
/home/bmg/src/gcc/gcc/builtins.c:4920
0x927eee check_access(tree_node*, tree_node*, tree_node*, tree_node*,
tree_node*, access_mode, access_data const*)
/home/bmg/src/gcc/gcc/builtins.c:4918
0x928b52 check_read_access
/home/bmg/src/gcc/gcc/builtins.c:4996
0x92e19b check_read_access
/home/bmg/src/gcc/gcc/builtins.c:9992
0x92e19b expand_builtin_memchr
/home/bmg/src/gcc/gcc/builtins.c:5926
0x92e19b expand_builtin(tree_node*, rtx_def*, rtx_def*, machine_mode, int)
/home/bmg/src/gcc/gcc/builtins.c:9992
0xa6d714 expand_expr_real_1(tree_node*, rtx_def*, machine_mode,
expand_modifier, rtx_def**, bool)
/home/bmg/src/gcc/gcc/expr.c:11275
0xa796fb store_expr(tree_node*, rtx_def*, int, bool, bool)
/home/bmg/src/gcc/gcc/expr.c:5885
0xa7ab71 expand_assignment(tree_node*, tree_node*, bool)
/home/bmg/src/gcc/gcc/expr.c:5621
0x9569bb expand_call_stmt
/home/bmg/src/gcc/gcc/cfgexpand.c:2837
0x9569bb expand_gimple_stmt_1
/home/bmg/src/gcc/gcc/cfgexpand.c:3843
0x9569bb expand_gimple_stmt
/home/bmg/src/gcc/gcc/cfgexpand.c:4007
0x95c60f expand_gimple_basic_block
/home/bmg/src/gcc/gcc/cfgexpand.c:6044
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
[PATCH] improve warning suppression for inlined functions (PR 98465, 98512)
std::string tends to trigger a class of false positive out of bounds
access warnings for code GCC cannot prove is unreachable because of
missing aliasing constrains, and that ends up expanded inline into
user code. Simply inserting the contents of a constant char array
does that. In GCC 10 these false positives are suppressed due to
-Wno-system-headers, but in GCC 11, to help detect calls rendered
invalid by user code passing in either incorrect or insufficiently
constrained arguments, -Wno-system-header no longer has this effect
on invalid access warnings.
To solve the problem without at least partially reverting the change
and going back to the GCC 10 way of things for the affected subset
of calls (just memcpy and memmove), the attached patch enhances
the #pragma GCC diagnostic machinery to consider not just a single
location for inlined code but all locations at which an expression
and its callers are inlined all the way up the stack. This gives
each author of a function involved in inlining the ability to
control a warning issued for the code, not just the user into whose
code all the calls end up inlined. To resolve PR 98465, it lets us
suppress the false positives selectively in std::string rather
than across the board in GCC.
The solution is to provide a new pair of overloads for warning
functions that, instead of taking a single location argument, take
a tree node from which the location(s) are determined. The tree
argument is indirect because the diagnostic machinery doesn't (and
cannot without more intrusive changes) at the moment depend on
the various tree definitions. A nice feature of these overloads
is that they do away with the need for the %K directive (and in
the future also %G, with another enhancement to accept a gimple*
argument).
This patch depends on the fix for PR 98664 (already approved but
not yet checked in). I've tested it on x86_64-linux.
To avoid fallout I tried to keep the changes to a minimum, and
so the design isn't as robust as I'd like it ultimately to be.
I plan to enhance it in stage 1.
Martin
PR middle-end/98465 - Bogus -Wstringop-overread in std::string
PR middle-end/98512 - “#pragma GCC diagnostic ignored” ineffective in conjunction with alias attribute
gcc/ChangeLog:
PR middle-end/98465
PR middle-end/98512
* builtins.c (class diag_inlining_context): New class.
(maybe_warn_for_bound): Adjust signature. Use diag_inlining_context.
(warn_for_access): Same.
(check_access): Remove calls to tree_inlined_location.
(expand_builtin_strncmp): Remove argument from calls to
maybe_warn_for_bound.
(warn_dealloc_offset): Adjust signature. Use diag_inlining_context.
(maybe_emit_free_warning): Remove calls to tree_inlined_location.
* diagnostic-core.h (warning, warning_n): New overloads.
* diagnostic-metadata.h (class diagnostic_metadata::location_context):
New.
(struct diagnostic_info): Declare.
* diagnostic.c (location_context::locations): Define.
(update_effective_level_from_pragmas): Use location_context to test
inlinined locations.
(diagnostic_report_diagnostic): Set location context.
(warning, warning_n): Define new overloads.
* diagnostic.h (diagnostic_inhibit_notes):
gcc/cp/ChangeLog:
* mapper-client.cc: Include headers needed by others.
libstdc++-v3/ChangeLog:
PR middle-end/98465
* include/bits/basic_string.tcc (_M_replace): Suppress false positive
warnings.
* testsuite/18_support/new_delete_placement.cc: Suppress valid warnings.
* testsuite/20_util/monotonic_buffer_resource/allocate.cc: Same.
* testsuite/20_util/unsynchronized_pool_resource/allocate.cc: Same.
gcc/testsuite/ChangeLog:
PR middle-end/98512
* gcc.dg/pragma-diag-9.c: New test.
diff --git a/gcc/builtins.c b/gcc/builtins.c
index c1115a32d91..68f1ae042d8 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -39,7 +39,7 @@ along with GCC; see the file COPYING3. If not see
#include "optabs.h"
#include "emit-rtl.h"
#include "recog.h"
-#include "diagnostic-core.h"
+#include "diagnostic.h"
#include "alias.h"
#include "fold-const.h"
#include "fold-const-call.h"
@@ -79,6 +79,7 @@ along with GCC; see the file COPYING3. If not see
#include "tree-outof-ssa.h"
#include "attr-fnspec.h"
#include "demangle.h"
+#include "tree-pretty-print.h"
struct target_builtins default_target_builtins;
#if SWITCHABLE_TARGET
@@ -749,6 +750,93 @@ is_builtin_name (const char *name)
return false;
}
+/* Class to override the base location context for an expression EXPR. */
+
+class diag_inlining_context: public diagnostic_metadata::location_context
+{
+ public:
+ diag_inlining_context (tree expr): m_expr (expr), m_ao (), m_loc () { }
+
+ virtual void locations (vec , diagnostic_info *di)
+ {
+set_locations (, di);
+ }
+
+ virtual void set_location (diagnostic_info *);
+
+ private:
+ void set_locations (vec *, diagnostic_info *);
+
+ /* The expression for which a diagnostic is being issued. */
+ tree m_expr;
+ /* The "abstract origin" of the diagnosed expression,
