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

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

[Rainer Orth]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

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.

Fixed as follows, which allowed sparcv9-sun-solaris2.11 bootstrap to
finish and passed regtest on sparc-sun-solaris2.11.

Ok for mainline?

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2017-12-04  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	* dbxout.c (dbxout_block): Grow buf to 30 bytes.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sol2-dbxout-format-overflow.patch --]
[-- Type: text/x-patch, Size: 410 bytes --]

diff --git a/gcc/dbxout.c b/gcc/dbxout.c
--- a/gcc/dbxout.c
+++ b/gcc/dbxout.c
@@ -3844,7 +3844,7 @@ dbxout_block (tree block, int depth, tre
 	      /* If we emitted any vars and didn't output any LBRAC/RBRAC,
 		 either at this level or any lower level, we need to emit
 		 an empty LBRAC/RBRAC pair now.  */
-	      char buf[20];
+	      char buf[30];
 	      const char *scope_start;
 
 	      ret = true;

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

[Martin Sebor]

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.

Martin

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

[Jeff Law]

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.
> 
> Fixed as follows, which allowed sparcv9-sun-solaris2.11 bootstrap to
> finish and passed regtest on sparc-sun-solaris2.11.
> 
> Ok for mainline?
OK.
jeff

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

[David Malcolm]

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

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

[Martin Sebor]

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

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

[David Malcolm]

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

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

[Martin Sebor]

On 12/08/2017 02:34 PM, David Malcolm wrote:
> 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.

Thanks for spending time on this!

Martin