On Mon, 2017-12-04 at 18:05 -0700, Martin Sebor wrote:
> 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.
> Martin
I've been poking at this file, and I think there's a cluster of bugs
related to how we handle and print chains of inlined callsites when
reporting middle-end diagnostics.
I've filed PR tree-optimization/83336 ("Issues with displaying inlining
chain for middle-end warnings")
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83336
as a catch-all to cover the various issues; I'm adding more information
there, and am testing some fixes.
Dave
