GCC produces slightly different output for the two equivalent calls to sprintf below. The underlining for the second call looks wrong, as does its color: it's green. $ cat t.c && gcc -O2 -S -Wall t.c const char a[] = { 'a' }; char *d, *e; void f (void) { __builtin_sprintf (d, "%s", a); __builtin_sprintf (e, "%s", &a[0]); } t.c: In function ‘f’: t.c:7:26: warning: ‘%s’ directive argument is not a nul-terminated string [-Wformat-overflow=] 7 | __builtin_sprintf (d, "%s", a); | ^~ ~ t.c:1:12: note: referenced argument declared here 1 | const char a[] = { 'a' }; | ^ t.c:8:26: warning: ‘%s’ directive argument is not a nul-terminated string [-Wformat-overflow=] 8 | __builtin_sprintf (e, "%s", &a[0]); | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ t.c:1:12: note: referenced argument declared here 1 | const char a[] = { 'a' }; | ^
In the first case the argument seen by the sprintf pass is: <addr_expr 0x7fffefc0a0a0 type <pointer_type 0x7fffefbe8e70 type <array_type 0x7fffefbe8690 type <integer_type 0x7fffefae51f8 char> QI size <integer_cst 0x7fffefac0d68 constant 8> unit-size <integer_cst 0x7fffefac0d80 constant 1> align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffefbe8690 domain <integer_type 0x7fffefae4d20> pointer_to_this <pointer_type 0x7fffefbe8e70>> unsigned DI size <integer_cst 0x7fffefac0c78 constant 64> unit-size <integer_cst 0x7fffefac0c90 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffefbe8e70> readonly constant arg:0 <var_decl 0x7ffff7ff5ab0 a type <array_type 0x7fffefbe8690> readonly addressable used public static read QI /build/tmp/t.c:1:12 size <integer_cst 0x7fffefac0d68 8> unit-size <integer_cst 0x7fffefac0d80 1> align:8 warn_if_not_align:0 context <translation_unit_decl 0x7fffefacdc30 /build/tmp/t.c> initial <string_cst 0x7fffefbf1cd8> chain <var_decl 0x7ffff7ff5b40 d type <pointer_type 0x7fffefae4e70> used public static unsigned common read DI /build/tmp/t.c:3:7 size <integer_cst 0x7fffefac0c78 64> unit-size <integer_cst 0x7fffefac0c90 8> align:64 warn_if_not_align:0 context <translation_unit_decl 0x7fffefacdc30 /build/tmp/t.c> chain <var_decl 0x7ffff7ff5bd0 e>>> /build/tmp/t.c:7:31 start: /build/tmp/t.c:7:31 finish: /build/tmp/t.c:7:31> while in the second case it is: <addr_expr 0x7fffefc0a160 type <pointer_type 0x7fffefae52a0 type <integer_type 0x7fffefae51f8 char readonly string-flag QI size <integer_cst 0x7fffefac0d68 constant 8> unit-size <integer_cst 0x7fffefac0d80 constant 1> align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffefae51f8 precision:8 min <integer_cst 0x7fffefac0db0 -128> max <integer_cst 0x7fffefac0de0 127> pointer_to_this <pointer_type 0x7fffefae52a0>> unsigned DI size <integer_cst 0x7fffefac0c78 constant 64> unit-size <integer_cst 0x7fffefac0c90 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7fffefae52a0> readonly constant arg:0 <array_ref 0x7fffefacc0a8 type <integer_type 0x7fffefae51f8 char> readonly arg:0 <var_decl 0x7ffff7ff5ab0 a type <array_type 0x7fffefbe8690> readonly addressable used public static read QI /build/tmp/t.c:1:12 size <integer_cst 0x7fffefac0d68 8> unit-size <integer_cst 0x7fffefac0d80 1> align:8 warn_if_not_align:0 context <translation_unit_decl 0x7fffefacdc30 /build/tmp/t.c> initial <string_cst 0x7fffefbf1cd8> chain <var_decl 0x7ffff7ff5b40 d>> arg:1 <integer_cst 0x7fffefadf018 constant 0> /build/tmp/t.c:8:33 start: /build/tmp/t.c:8:32 finish: /build/tmp/t.c:8:35> /build/tmp/t.c:8:3 start: /build/tmp/t.c:8:3 finish: /build/tmp/t.c:8:36> The (presumably) relevant difference in the locations of the two is: /build/tmp/t.c:7:31 start: /build/tmp/t.c:7:31 finish: /build/tmp/t.c:7:31 vs /build/tmp/t.c:8:3 start: /build/tmp/t.c:8:3 finish: /build/tmp/t.c:8:36 Not sure where that comes from, but the ADDR_EXPR operand does have the right location.
The second ADDR_EXPR's location is wrong; it's the location of the whole of the function call: 8 | __builtin_sprintf (e, "%s", &a[0]); | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~ rather than just of the 2nd argument, and this leads to the strange colorization. The bogus location is coming from here in gimplify.c: 3130 /* FIXME diagnostics: This will mess up gcc.dg/Warray-bounds.c. */ 3131 /* Make sure arguments have the same location as the function call 3132 itself. */ 3133 protected_set_expr_location (*arg_p, call_location); which came from commits by Aldy in 2008 and 2009: r140917 (aka 489c40889c8be89bd5bed4b166974f8c1e01e4ee) and r148442 (aka e60a6f7b6c12183ca32dfe2876f09f4b4f4f69c1)
prior to that gimplify.c code, the location of the argument is correctly set, as: t.c:8:31: note: 8 | __builtin_sprintf (e, "%s", &a[0]); | ^~~~~
Aldy: do you remember why the arguments need to have the same location as the call itself? The first case works because during gimplify_arg it's wrapped in a NOP_EXPR which is casting from char[] to char *, and it's this NOP_EXPR which gets its location modified. The second case doesn't have the NOP_EXPR.
(In reply to David Malcolm from comment #4) > Aldy: do you remember why the arguments need to have the same location as > the call itself? > > The first case works because during gimplify_arg it's wrapped in a NOP_EXPR > which is casting from char[] to char *, and it's this NOP_EXPR which gets > its location modified. > > The second case doesn't have the NOP_EXPR. This looks like an oversight on my part. In the original patch you mention, there was a now defunct bit that seems suspicious and my shed light on the problem: + /* FIXME diagnostics: This should be the location of the argument, + not the FNDECL. E.g., for an old-style declaration + + int f10(v) { blah; } + + We should use the location of the V, not the F10. + Unfortunately, the V is an IDENTIFIER_NODE which has no + location. In the future we need locations for c_arg_info + entries. + + See gcc.dg/Wshadow-3.c for an example of this problem. */ So maybe I was assuming that the problematic line was also dealing with an IDENTIFIER_NODE which had no place to put a location? Either way, it looks like some sort of oversight. Confirmed, BTW.