On 3/4/20 8:54 AM, Jeff Law wrote:
Martin, I'd like your thoughts here.
As noted in the BZ our #pragmas aren't working to suppress a warning.
I did some debugging and ultimately found that the location passed down to the
diagnostic code is indeed outside the scope of the pragmas.
Further digging uncovered that we had a reasonable location passed to
maybe_diag_access_bounds, but rather than using it, we extracted a location
attached to the builtin_memref structure.
So if we look at the test:
char one[50];
char two[50];
void
test_strncat (void)
{
(void) __builtin_strcpy (one, "gh");
(void) __builtin_strcpy (two, "ef");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-overflow="
#pragma GCC diagnostic ignored "-Warray-bounds"
(void) __builtin_strncat (one, two, 99);
#pragma GCC diagnostic pop
}
The location we end up passing to the diagnostic code comes from the destination
of the call (via DSTREF which is passed to maybe_diag_access_bounds and becomes
REF in that context).
(gdb) p debug_tree (ref.ptr)
<addr_expr 0x7fffea93d0a0
type <pointer_type 0x7fffea937540
type <array_type 0x7fffea9372a0 type <integer_type 0x7fffea8173f0 char>
BLK
size <integer_cst 0x7fffea81c3a8 constant 400>
unit-size <integer_cst 0x7fffea933f90 constant 50>
align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea9372a0 domain <integer_type 0x7fffea9371f8>
pointer_to_this <pointer_type 0x7fffea937540>>
unsigned DI
size <integer_cst 0x7fffea7fecd8 constant 64>
unit-size <integer_cst 0x7fffea7fecf0 constant 8>
align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7fffea937540>
constant
arg:0 <var_decl 0x7ffff7ffbb40 one type <array_type 0x7fffea9372a0>
addressable used public static read BLK j.c:2:6 size <integer_cst
0x7fffea81c3a8 400> unit-size <integer_cst 0x7fffea933f90 50>
align:256 warn_if_not_align:0 context <translation_unit_decl
0x7fffea80bac8 j.c>
chain <var_decl 0x7ffff7ffbbd0 two type <array_type 0x7fffea9372a0>
addressable used public static read BLK j.c:3:6 size <integer_cst
0x7fffea81c3a8 400> unit-size <integer_cst 0x7fffea933f90 50>
align:256 warn_if_not_align:0 context <translation_unit_decl
0x7fffea80bac8 j.c> chain <function_decl 0x7fffea92f500 test_strncat>>>
j.c:8:28 start: j.c:8:28 finish: j.c:8:30>
Note the location. j.c line 8 column 28-30. THat location makes absolutely no
sense (it's the first call to strcpy) given we're trying to diagnose the
strncat.
I have no idea why we're using the location from ref.ptr rather than the
location
of the statement we're analyzing (which is passed in as LOC). It's no surprise
the pragma didn't work.
Anyway, this patch seems to fix the problem and doesn't regress anything. But
I'm hesitant to go forward with it given I don't know the motivation behind
extracting the location from ref.ptr.
Another thought would be to just remove the code that pulls the location from
ref.ptr. I haven't tested that, but certainly could easily.
Martin, thoughts?
I don't remember why the code in -Wrestrict unconditionally overwrites
the statement location rather than only when it's not available, but
I do remember adding conditional code like in your patch in r277076
to deal with missing location on the statement. So either your fix
or something like the hunk below might be the right solution (if we
go with the code below, abstracting it into a utility function might
be nice).
Thanks for looking into it!
Martin
@@ -3642,7 +3670,11 @@ maybe_warn_pointless_strcmp (gimple *stmt,
HOST_WIDE_INT bound,
/* FIXME: Include a note pointing to the declaration of the smaller
array. */
- location_t stmt_loc = gimple_location (stmt);
+ location_t stmt_loc = gimple_nonartificial_location (stmt);
+ if (stmt_loc == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (lhs))
+ stmt_loc = tree_nonartificial_location (lhs);
+ stmt_loc = expansion_point_location_if_in_system_header (stmt_loc);
+
tree callee = gimple_call_fndecl (stmt);
bool warned = false;
if (siz <= minlen && bound == -1)
