On 12/04/2017 01:58 PM, David Malcolm wrote:
On Mon, 2017-12-04 at 12:18 -0700, Martin Sebor wrote:
On 12/04/2017 05:41 AM, Rainer Orth wrote:
Within test last week, 64-bit Solaris/SPARC bootstrap began to
fail:
/vol/gcc/src/hg/trunk/local/gcc/dbxout.c: In function 'bool
dbxout_block(tree, int, tree, int)':
/vol/gcc/src/hg/trunk/local/gcc/dbxout.c:3767:1: error: '%lu'
directive writing between 1 and 20 bytes into a region of size
14
[-Werror=format-overflow=]
dbxout_block (tree block, int depth, tree args, int
parent_blocknum)
^~~~~~~~~~~~
/vol/gcc/src/hg/trunk/local/gcc/dbxout.c:3767:1: note:
directive
argument in the range [0, 18446744073709551614]
In file included from ./tm.h:26,
from
/vol/gcc/src/hg/trunk/local/gcc/target.h:52,
from
/vol/gcc/src/hg/trunk/local/gcc/dbxout.c:72:
/vol/gcc/src/hg/trunk/local/gcc/config/sparc/sol2.h:353:11:
note:
'std::sprintf' output between 8 and 27 bytes into a destination
of
size 20
sprintf ((LABEL), "*.L%s%lu", (PREFIX), (unsigned
long)(NUM))
~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~
/vol/gcc/src/hg/trunk/local/gcc/dbxout.c:3855:5: note: in
expansion
of macro 'ASM_GENERATE_INTERNAL_LABEL'
ASM_GENERATE_INTERNAL_LABEL (buf, "LBB", parent_blocknum);
^~~~~~~~~~~~~~~~~~~~~~~~~~~
The line numbers are extremely confusing, to say the least,
though:
the
one in the error and the first note refer to the begin of the
function
definition and only the third note refers to the line of the
actual
error.
I agree it looks confusing. It's the result of the interaction
between macro tracking and inlining.
I think it's a general problem that affects many (though not all)
warnings emitted out of the middle-end. For instance, the
example
below results in similar output. The output changes depending on
whether or not _FORTIFY_SOURCE is defined.
#include <string.h>
#define FOO(d, s) strcpy (d, s)
void foo (char *d, const char *s) { FOO (d, s); }
void sink (char*);
void bar (void)
{
char a[3];
foo (a, "1234567"); // bug here
sink (a);
}
Without _FORTIFY_SOURCE:
d.c: In function ‘void bar()’:
d.c:3:26: warning: ‘void* __builtin_memcpy(void*, const void*,
long
unsigned int)’ writing 8 bytes into a region of size 3 overflows
the
destination [-Wstringop-overflow=]
#define FOO(d, s) strcpy (d, s)
~~~~~~~^~~~~~
d.c:5:37: note: in expansion of macro ‘FOO’
void foo (char *d, const char *s) { FOO (d, s); }
^~~
If bar() were a bigger function with multiple calls to foo() it
could be tricky to tell which of them caused the warning.
With _FORTIFY_SOURCE:
In file included from /usr/include/string.h:635,
from d.c:1:
In function ‘char* strcpy(char*, const char*)’,
inlined from ‘void bar()’ at d.c:5:37:
/usr/include/bits/string3.h:110:33: warning: ‘void*
__builtin___memcpy_chk(void*, const void*, long unsigned int,
long
unsigned int)’ writing 8 bytes into a region of size 3 overflows
the
destination [-Wstringop-overflow=]
return __builtin___strcpy_chk (__dest, __src, __bos
(__dest));
~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This suffers from the same problem as the first case: the line
number of the offending call in bar() is missing.
In both cases the problem is compounded by the diagnostic, as
a result of folding, referring to __builtin___memcpy_chk while
the source code contains a call to strcpy.
I don't know nearly enough about the internals of the diagnostic
subsystem to tell how difficult it might be to change the output
to make it more readable. David Malcolm is the expert on this
area so he might have an idea what it would take.
[Did you mean to CC me on this, rather than dje?]
I meant you but I'm interested in all expert opinions/suggestions.
I'm not as familiar as I could be on the existing inlining-tracking
implementation - so much so that, in ignorance, I wrote my own
version
of it earlier this year (doh!).
The warning implementation reads:
3151 warning_at (loc, opt,
3152 (integer_onep (range[0])
3153 ? G_("%K%qD writing %E byte
into a region "
3154 "of size %E overflows the
destination")
3155 : G_("%K%qD writing %E bytes
into a region "
3156 "of size %E overflows the
destination")),
3157 exp, get_callee_fndecl (exp),
range[0], objsize);
The inlining information is coming from that "%K" in the string,
which
leads to tree-pretty-print.c's percent_K_format being called for
"exp".
This overwrites the "loc" provided for the warning_at with
EXPR_LOCATION of exp, and looks at TREE_BLOCK (exp), finding the
outermost containing block within its function, and writing it into
the
diagnostic_info's x_data.
This block is used by lhd_print_error_function (and C++'s
cp_print_error_function) to print the "inlined from" information.
This has several limitations:
(a) It would be better for the user to identify a specific
callsite,
rather than a function (or block), and print the source of the
callsite; we would then print something like:
(without _FORTIFY_SOURCE)
d.c: In function ‘void foo(char *d, const char *s)’,
inlined from ‘bar’ at d.c:13:5:
foo (a, "1234567"); // bug here
~~~~^~~~~~~~~~~~~~
d.c:3:26: warning: ‘void* __builtin_memcpy(void*, const void*, long
unsigned int)’ writing 8 bytes into a region of size 3 overflows
the destination [-Wstringop-overflow=]
#define FOO(d, s) strcpy (d, s)
~~~~~~~^~~~~~
d.c:5:37: note: in expansion of macro ‘FOO’
void foo (char *d, const char *s) { FOO (d, s); }
^~~
(with _FORTIFY_SOURCE)
In file included from /usr/include/string.h:636,
from d.c:1:
In function ‘strcpy’,
inlined from ‘foo’ at d.c:5:37,
void foo (char *d, const char *s) { FOO (d, s); }
^~~
inlined from ‘bar’ at d.c:13:5:
foo (a, "1234567"); // bug here
~~~~^~~~~~~~~~~~~~
/usr/include/bits/string3.h:104:10: warning:
‘__builtin___memcpy_chk’ writing 8 bytes into a region of size 3
overflows the destination [-Wstringop-overflow=]
return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Yes, that would be much better. AFAICS, it actually already seems
to work this way when foo() is declared both inline and artificial.
How ought we to store the location of the callsite that's inlined?
(e.g. Would it be reasonable to add a TREE_BLOCK around a callsite
that's inlined, for that purpose?
It sounds reasonable to me.
(b) That "%K" seems like something of a wart. Would a family of
overloads like:
warning_at (tree exp, int option, const char *etc, ...);
be more appropriate? (though that requires locations for every
expression, I guess).
Given that the only position in the format string where %K makes
sense the above would make sense. %K is documented to take
a CALL_EXPR argument. There's now also a %G for a GIMPLE call
argument, and presumably they already have a location. A potential
complication is that there are two locations: the location argument
that's computed like so:
location_t loc = tree_nonartificial_location (exp);
loc = expansion_point_location_if_in_system_header (loc);
and the CALL_EXPR argument to the %K directive. If there are
clients that need to suppress the warning from system headers
and others that don't then a single API would be tricky to use
in both kinds. But I don't know of any such clients and
disabling warnings coming out of system headers may not be
necessary in the middle end.
