Re: [PATCH] avoid -Wnonnull for printf format in dead code (PR 87041)
On 11/15/18 12:06 PM, Martin Sebor wrote:
> On 11/15/2018 02:39 AM, Matthew Malcomson wrote:
>> On 02/11/18 09:54, Christophe Lyon wrote:
>>> Hi,
>>>
>>> I've noticed failure on targets using newlib (aarch64-elf and arm-eabi):
>>> FAIL: gcc.c-torture/execute/printf-2.c
>>> FAIL: gcc.c-torture/execute/user-printf.c
>>>
>>> my gcc.log contains:
>>> gcc.c-torture/execute/user-printf.c -O0 execution test (reason: TCL
>>> LOOKUP CHANNEL exp5)
>>> which is not very helpful
>>>
>>
>> @Christophe
>> Can I ask if your DejaGNU board setup has "needs_status_wrapper 1" for
>> both of these boards?
>>
>> I believe this problem is caused by an interaction with the DejaGNU
>> status wrapper.
>>
>> When the status wrapper is needed, DejaGNU looks at stdout for a line
>> saying "*** EXIT code " indicating what the status code was.
>> When it doesn't find that line it assumes an exit code of 2.
>> Without the status wrapper DejaGNU takes the return code from the
>> program executed.
>>
>> Because these tests use "freopen ()" on stdout, the status wrapper fails
>> to print to the IO channel DejaGNU is listening on, hence DejaGNU fails
>> to find it's line, uses an exit code of 2, and fails the test.
>>
>>
>> @Martin
>> Would these tests still be valid having run freopen on stderr and using
>> fprintf instead of printf?
>> That makes the testcases pass for me.
>
> The printf-2.c test specifically exercises the printf built-in,
> i.e., not fprintf, so changing it to use fprintf would defeat
> its purpose.
>
> user-printf.c does something similar but for a user-defined
> function with attribute format (printf).
>
> The purpose of the tests is to verify that what they read from
> the temporary file matches what they wrote (i.e,, that none of
> the printf() or user_print() calls was incorrectly eliminated).
>
>> If not we could add an
>> { dg-require-effective-target unwrapped }
>> directive in the testcases to stop the failure complaints.
>
> I'm not familiar with this directive or really know what
> a status wrapper is but as long as it doesn't change the I/O
> the test does I think it should be fine.
Wrapping in this context refers to the dejagnu harness wrapping to
facilitate testing of remote and embedded targets where getting the real
exit status of an execution test is painful.
We wrap main, exit and abort. The wrappers print info to stdout to
indicate exit status which can reliably be read by the harness.
At least that's my recollection of the wrapper bits.
jeff
Re: [PATCH] avoid -Wnonnull for printf format in dead code (PR 87041)
On 11/15/2018 02:39 AM, Matthew Malcomson wrote:
On 02/11/18 09:54, Christophe Lyon wrote:
Hi,
I've noticed failure on targets using newlib (aarch64-elf and arm-eabi):
FAIL: gcc.c-torture/execute/printf-2.c
FAIL: gcc.c-torture/execute/user-printf.c
my gcc.log contains:
gcc.c-torture/execute/user-printf.c -O0 execution test (reason: TCL
LOOKUP CHANNEL exp5)
which is not very helpful
@Christophe
Can I ask if your DejaGNU board setup has "needs_status_wrapper 1" for
both of these boards?
I believe this problem is caused by an interaction with the DejaGNU
status wrapper.
When the status wrapper is needed, DejaGNU looks at stdout for a line
saying "*** EXIT code " indicating what the status code was.
When it doesn't find that line it assumes an exit code of 2.
Without the status wrapper DejaGNU takes the return code from the
program executed.
Because these tests use "freopen ()" on stdout, the status wrapper fails
to print to the IO channel DejaGNU is listening on, hence DejaGNU fails
to find it's line, uses an exit code of 2, and fails the test.
@Martin
Would these tests still be valid having run freopen on stderr and using
fprintf instead of printf?
That makes the testcases pass for me.
The printf-2.c test specifically exercises the printf built-in,
i.e., not fprintf, so changing it to use fprintf would defeat
its purpose.
user-printf.c does something similar but for a user-defined
function with attribute format (printf).
The purpose of the tests is to verify that what they read from
the temporary file matches what they wrote (i.e,, that none of
the printf() or user_print() calls was incorrectly eliminated).
If not we could add an
{ dg-require-effective-target unwrapped }
directive in the testcases to stop the failure complaints.
I'm not familiar with this directive or really know what
a status wrapper is but as long as it doesn't change the I/O
the test does I think it should be fine.
Martin
Re: [PATCH] avoid -Wnonnull for printf format in dead code (PR 87041)
On Thu, 15 Nov 2018 at 10:39, Matthew Malcomson
wrote:
>
> On 02/11/18 09:54, Christophe Lyon wrote:
> > Hi,
> >
> > I've noticed failure on targets using newlib (aarch64-elf and arm-eabi):
> > FAIL: gcc.c-torture/execute/printf-2.c
> > FAIL: gcc.c-torture/execute/user-printf.c
> >
> > my gcc.log contains:
> > gcc.c-torture/execute/user-printf.c -O0 execution test (reason: TCL
> > LOOKUP CHANNEL exp5)
> > which is not very helpful
> >
>
> @Christophe
> Can I ask if your DejaGNU board setup has "needs_status_wrapper 1" for
> both of these boards?
>
Yes, I do use this.
> I believe this problem is caused by an interaction with the DejaGNU
> status wrapper.
>
> When the status wrapper is needed, DejaGNU looks at stdout for a line
> saying "*** EXIT code " indicating what the status code was.
> When it doesn't find that line it assumes an exit code of 2.
> Without the status wrapper DejaGNU takes the return code from the
> program executed.
>
> Because these tests use "freopen ()" on stdout, the status wrapper fails
> to print to the IO channel DejaGNU is listening on, hence DejaGNU fails
> to find it's line, uses an exit code of 2, and fails the test.
>
>
> @Martin
> Would these tests still be valid having run freopen on stderr and using
> fprintf instead of printf?
> That makes the testcases pass for me.
>
> If not we could add an
> { dg-require-effective-target unwrapped }
> directive in the testcases to stop the failure complaints.
Re: [PATCH] avoid -Wnonnull for printf format in dead code (PR 87041)
On 02/11/18 09:54, Christophe Lyon wrote:
> Hi,
>
> I've noticed failure on targets using newlib (aarch64-elf and arm-eabi):
> FAIL: gcc.c-torture/execute/printf-2.c
> FAIL: gcc.c-torture/execute/user-printf.c
>
> my gcc.log contains:
> gcc.c-torture/execute/user-printf.c -O0 execution test (reason: TCL
> LOOKUP CHANNEL exp5)
> which is not very helpful
>
@Christophe
Can I ask if your DejaGNU board setup has "needs_status_wrapper 1" for
both of these boards?
I believe this problem is caused by an interaction with the DejaGNU
status wrapper.
When the status wrapper is needed, DejaGNU looks at stdout for a line
saying "*** EXIT code " indicating what the status code was.
When it doesn't find that line it assumes an exit code of 2.
Without the status wrapper DejaGNU takes the return code from the
program executed.
Because these tests use "freopen ()" on stdout, the status wrapper fails
to print to the IO channel DejaGNU is listening on, hence DejaGNU fails
to find it's line, uses an exit code of 2, and fails the test.
@Martin
Would these tests still be valid having run freopen on stderr and using
fprintf instead of printf?
That makes the testcases pass for me.
If not we could add an
{ dg-require-effective-target unwrapped }
directive in the testcases to stop the failure complaints.
Re: [PATCH] avoid -Wnonnull for printf format in dead code (PR 87041)
On Tue, 30 Oct 2018 at 20:07, Jeff Law wrote: > > On 10/29/18 3:59 PM, Martin Sebor wrote: > > PR 87041 - -Wformat "reading through null pointer" on unreachable > > code is a complaint about -Wformat false positives due to null > > arguments to %s directives in unreachable printf calls. The warning > > is issued by the front end, too early to know whether or not the call > > is ever made. > > > > The -Wformat-overflow has had the ability to detect null pointers > > in %s and similar directives to sprintf calls since GCC 7 without > > these false positives, but the warning doesn't consider stream or > > file I/O functions like printf/fprintf. To resolve the bug report > > I have enhanced -Wformat-overflow to consider all printf-like > > functions, including user-defined ones declared attribute format > > (printf). > > > > Besides null pointers the enhancement also makes it possible to > > detect other problems (like out-of-range arguments and output in > > excess of INT_MAX bytes). It also lays the groundwork for > > checking user-defined printf-like functions for buffer overflow > > (once a suitable attribute is added to indicate which arguments > > are the destination buffer pointer and the buffer size). > > > > With that, I have removed the null checking from -Wformat (again, > > only for printf-like functions). > > > > Martin > > > > gcc-87041.diff > > > > PR middle-end/87041 - -Wformat reading through null pointer on unreachable > > code > > > > gcc/ChangeLog: > > > > PR middle-end/87041 > > * gimple-ssa-sprintf.c (format_directive): Use %G to include > > inlining context. > > (sprintf_dom_walker::compute_format_length): > > Avoid setting POSUNDER4K here. > > (get_destination_size): Handle null argument values. > > (get_user_idx_format): New function. > > (sprintf_dom_walker::handle_gimple_call): Handle all printf-like > > functions, including user-defined with attribute format printf. > > Use %G to include inlining context. > > Set POSUNDER4K here. > > > > gcc/c-family/ChangeLog: > > > > PR middle-end/87041 > > * c-format.c (check_format_types): Avoid diagnosing null pointer > > arguments to printf-family of functions. > > > > gcc/testsuite/ChangeLog: > > > > PR middle-end/87041 > > * gcc.c-torture/execute/fprintf-2.c: New test. > > * gcc.c-torture/execute/printf-2.c: Same. > > * gcc.c-torture/execute/user-printf.c: Same. > > * gcc.dg/tree-ssa/builtin-fprintf-warn-1.c: Same. > > * gcc.dg/tree-ssa/builtin-printf-2.c: Same. > > * gcc.dg/tree-ssa/builtin-printf-warn-1.c: Same. > > * gcc.dg/tree-ssa/user-printf-warn-1.c: Same. > OK. > Hi, I've noticed failure on targets using newlib (aarch64-elf and arm-eabi): FAIL: gcc.c-torture/execute/printf-2.c FAIL: gcc.c-torture/execute/user-printf.c my gcc.log contains: gcc.c-torture/execute/user-printf.c -O0 execution test (reason: TCL LOOKUP CHANNEL exp5) which is not very helpful > Note some folks might complain about dropping the warning from the > front-end. Their (largely reasonable) argument is that warning out of > the front-end is stable across releases and doesn't depend on > optimizations. Of course the downside of warning out of the front-end > is false positives like we see in this PR. > > jeff
Re: [PATCH] avoid -Wnonnull for printf format in dead code (PR 87041)
On 10/29/18 3:59 PM, Martin Sebor wrote: > PR 87041 - -Wformat "reading through null pointer" on unreachable > code is a complaint about -Wformat false positives due to null > arguments to %s directives in unreachable printf calls. The warning > is issued by the front end, too early to know whether or not the call > is ever made. > > The -Wformat-overflow has had the ability to detect null pointers > in %s and similar directives to sprintf calls since GCC 7 without > these false positives, but the warning doesn't consider stream or > file I/O functions like printf/fprintf. To resolve the bug report > I have enhanced -Wformat-overflow to consider all printf-like > functions, including user-defined ones declared attribute format > (printf). > > Besides null pointers the enhancement also makes it possible to > detect other problems (like out-of-range arguments and output in > excess of INT_MAX bytes). It also lays the groundwork for > checking user-defined printf-like functions for buffer overflow > (once a suitable attribute is added to indicate which arguments > are the destination buffer pointer and the buffer size). > > With that, I have removed the null checking from -Wformat (again, > only for printf-like functions). > > Martin > > gcc-87041.diff > > PR middle-end/87041 - -Wformat reading through null pointer on unreachable > code > > gcc/ChangeLog: > > PR middle-end/87041 > * gimple-ssa-sprintf.c (format_directive): Use %G to include > inlining context. > (sprintf_dom_walker::compute_format_length): > Avoid setting POSUNDER4K here. > (get_destination_size): Handle null argument values. > (get_user_idx_format): New function. > (sprintf_dom_walker::handle_gimple_call): Handle all printf-like > functions, including user-defined with attribute format printf. > Use %G to include inlining context. > Set POSUNDER4K here. > > gcc/c-family/ChangeLog: > > PR middle-end/87041 > * c-format.c (check_format_types): Avoid diagnosing null pointer > arguments to printf-family of functions. > > gcc/testsuite/ChangeLog: > > PR middle-end/87041 > * gcc.c-torture/execute/fprintf-2.c: New test. > * gcc.c-torture/execute/printf-2.c: Same. > * gcc.c-torture/execute/user-printf.c: Same. > * gcc.dg/tree-ssa/builtin-fprintf-warn-1.c: Same. > * gcc.dg/tree-ssa/builtin-printf-2.c: Same. > * gcc.dg/tree-ssa/builtin-printf-warn-1.c: Same. > * gcc.dg/tree-ssa/user-printf-warn-1.c: Same. OK. Note some folks might complain about dropping the warning from the front-end. Their (largely reasonable) argument is that warning out of the front-end is stable across releases and doesn't depend on optimizations. Of course the downside of warning out of the front-end is false positives like we see in this PR. jeff
[PATCH] avoid -Wnonnull for printf format in dead code (PR 87041)
PR 87041 - -Wformat "reading through null pointer" on unreachable
code is a complaint about -Wformat false positives due to null
arguments to %s directives in unreachable printf calls. The warning
is issued by the front end, too early to know whether or not the call
is ever made.
The -Wformat-overflow has had the ability to detect null pointers
in %s and similar directives to sprintf calls since GCC 7 without
these false positives, but the warning doesn't consider stream or
file I/O functions like printf/fprintf. To resolve the bug report
I have enhanced -Wformat-overflow to consider all printf-like
functions, including user-defined ones declared attribute format
(printf).
Besides null pointers the enhancement also makes it possible to
detect other problems (like out-of-range arguments and output in
excess of INT_MAX bytes). It also lays the groundwork for
checking user-defined printf-like functions for buffer overflow
(once a suitable attribute is added to indicate which arguments
are the destination buffer pointer and the buffer size).
With that, I have removed the null checking from -Wformat (again,
only for printf-like functions).
Martin
PR middle-end/87041 - -Wformat reading through null pointer on unreachable code
gcc/ChangeLog:
PR middle-end/87041
* gimple-ssa-sprintf.c (format_directive): Use %G to include
inlining context.
(sprintf_dom_walker::compute_format_length):
Avoid setting POSUNDER4K here.
(get_destination_size): Handle null argument values.
(get_user_idx_format): New function.
(sprintf_dom_walker::handle_gimple_call): Handle all printf-like
functions, including user-defined with attribute format printf.
Use %G to include inlining context.
Set POSUNDER4K here.
gcc/c-family/ChangeLog:
PR middle-end/87041
* c-format.c (check_format_types): Avoid diagnosing null pointer
arguments to printf-family of functions.
gcc/testsuite/ChangeLog:
PR middle-end/87041
* gcc.c-torture/execute/fprintf-2.c: New test.
* gcc.c-torture/execute/printf-2.c: Same.
* gcc.c-torture/execute/user-printf.c: Same.
* gcc.dg/tree-ssa/builtin-fprintf-warn-1.c: Same.
* gcc.dg/tree-ssa/builtin-printf-2.c: Same.
* gcc.dg/tree-ssa/builtin-printf-warn-1.c: Same.
* gcc.dg/tree-ssa/user-printf-warn-1.c: Same.
Index: gcc/c-family/c-format.c
===
--- gcc/c-family/c-format.c (revision 265496)
+++ gcc/c-family/c-format.c (working copy)
@@ -3123,8 +3123,11 @@ check_format_types (const substring_loc _loc,
warning (OPT_Wformat_, "writing through null pointer "
"(argument %d)", arg_num);
- /* Check for reading through a NULL pointer. */
- if (types->reading_from_flag
+ /* Check for reading through a NULL pointer. Ignore
+ printf-family of functions as they are checked for
+ null arguments by the middle-end. */
+ if (fki->conversion_specs != print_char_table
+ && types->reading_from_flag
&& i == 0
&& cur_param != 0
&& integer_zerop (cur_param))
Index: gcc/gimple-ssa-sprintf.c
===
--- gcc/gimple-ssa-sprintf.c (revision 265496)
+++ gcc/gimple-ssa-sprintf.c (working copy)
@@ -68,6 +68,7 @@ along with GCC; see the file COPYING3. If not see
#include "intl.h"
#include "langhooks.h"
+#include "attribs.h"
#include "builtins.h"
#include "stor-layout.h"
@@ -2796,8 +2797,9 @@ format_directive (const sprintf_dom_walker::call_i
if (fmtres.nullp)
{
fmtwarn (dirloc, argloc, NULL, info.warnopt (),
- "%<%.*s%> directive argument is null",
- dirlen, target_to_host (hostdir, sizeof hostdir, dir.beg));
+ "%G%<%.*s%> directive argument is null",
+ info.callstmt, dirlen,
+ target_to_host (hostdir, sizeof hostdir, dir.beg));
/* Don't bother processing the rest of the format string. */
res->warned = true;
@@ -3475,7 +3477,6 @@ sprintf_dom_walker::compute_format_length (call_in
by the known range [0, 0] (with no conversion resulting in a failure
or producing more than 4K bytes) until determined otherwise. */
res->knownrange = true;
- res->posunder4k = true;
res->floating = false;
res->warned = false;
@@ -3518,6 +3519,10 @@ sprintf_dom_walker::compute_format_length (call_in
static unsigned HOST_WIDE_INT
get_destination_size (tree dest)
{
+ /* When there is no destination return -1. */
+ if (!dest)
+return HOST_WIDE_INT_M1U;
+
/* Initialize object size info before trying to compute it. */
init_object_sizes ();
@@ -3738,6 +3743,37 @@ try_simplify_call (gimple_stmt_iterator *gsi,
return false;
}
+/* Return the zero-based index of the format string argument of a printf
+ like function and set *IDX_ARGS to the first format argument. When
+ no such index exists return UINT_MAX. */
+
+static unsigned
+get_user_idx_format (tree fndecl, unsigned *idx_args)
+{
+ tree attrs = lookup_attribute ("format", DECL_ATTRIBUTES
