Re: Avoid -Werror=format-overflow error in dbxout.c (dbxout_block) on Solaris/SPARC

[David Malcolm <dmalcolm at redhat dot com>]

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'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));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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?

(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).

etc.

Dave