Bug 91890 - [10 Regression] -Warray-bounds warning testing glibc not suppressed by pragma
Summary: [10 Regression] -Warray-bounds warning testing glibc not suppressed by pragma
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 10.0
: P1 normal
Target Milestone: 10.0
Assignee: Martin Sebor
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2019-09-24 23:12 UTC by Joseph S. Myers
Modified: 2020-03-05 21:06 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-09-25 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph S. Myers 2019-09-24 23:12:56 UTC
r275981 ("PR middle-end/91631 - buffer overflow into an array member of a declared object not detected") introduced an error when the following code is built with -O2 -Wall -Werror (the problem isn't that there is a -Warray-bounds diagnostic for the code, the problem is that the pragma is no longer suppressing it for some reason). This is reduced from a glibc testcase (which has had the pragma to suppress the warning for some time, but the pragma is no longer working).

char one[50];
char two[50];

void
test_strncat (void)
{
  (void) __builtin_strcpy (one, "gh");
  (void) __builtin_strcpy (two, "ef");
 
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-overflow="
#pragma GCC diagnostic ignored "-Warray-bounds"
  (void) __builtin_strncat (one, two, 99); 
#pragma GCC diagnostic pop
}


aarch64-glibc-linux-gnu-gcc -S -O2 -Wall -Werror tester.c
tester.c: In function 'test_strncat':
tester.c:13:10: error: '__builtin_strncat' forming offset [50, 98] is out of the bounds [0, 50] of object 'one' with type 'char[50]' [-Werror=array-bounds]
   13 |   (void) __builtin_strncat (one, two, 99);
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tester.c:1:6: note: 'one' declared here
    1 | char one[50];
      |      ^~~
cc1: all warnings being treated as errors
Comment 1 Martin Sebor 2019-09-25 01:12:24 UTC
Confirmed.  The diagnostic pragmas don't work very well for the middle-end warnings (e.g., with inlining, as discussed in bug 55881).  In this test case, moving the #pragma GCC diagnostic ignored above the strcpy calls does the trick.  I haven't looked into why.  Something similar was reported in bug 66099.  Manu had some ideas for how to make it work more reliably but I haven't had the time to work on it and don't expect to for GCC 10.
Comment 2 Martin Sebor 2019-09-25 14:07:00 UTC
To clarify what I said in comment #1: with the growing coverage of middle-end warnings, making the diagnostic pragmas work more reliably is increasingly important.  I don't think I'll have the time to work on it in stage 1 but since this is a bug (and could even be considered a regression) I'll try to find some in stage 3.
Comment 3 Manuel López-Ibáñez 2019-11-02 12:20:24 UTC
(In reply to Martin Sebor from comment #1)
> Confirmed.  The diagnostic pragmas don't work very well for the middle-end
> warnings (e.g., with inlining, as discussed in bug 55881).  In this test
> case, moving the #pragma GCC diagnostic ignored above the strcpy calls does
> the trick.  I haven't looked into why.  Something similar was reported in
> bug 66099.  Manu had some ideas for how to make it work more reliably but I
> haven't had the time to work on it and don't expect to for GCC 10.

I don't think this is the same issue. In this case, the diagnostic is emitted within the range affected by the #pragma. 

In those other cases, the location at which the warning is emitted is not within the range silenced by the #pragma (due to middle-end transformations or inlining) so there is no way the #pragma can silence it.
Comment 4 Manuel López-Ibáñez 2019-11-02 12:38:38 UTC
I'm 100% convinced this has nothing to do with locations and all to do with how -Warray-bounds and -Wstringop-overflow= interact.

Change the ignored for error, 

char one[50];
char two[50];
void
test_strncat (void)
{
  (void) __builtin_strcpy (one, "gh");
  (void) __builtin_strcpy (two, "ef");
#pragma GCC diagnostic ignored "-Wstringop-overflow"
#pragma GCC diagnostic error "-Warray-bounds"
  (void) __builtin_strncat (one, two, 99); 
}

and run with -Wall  -O2 -Wno-array-bounds and we get:

<source>:10:28: error: 'char* __builtin_strncat(char*, const char*, long unsigned int)' forming offset [50, 98] is out of the bounds [0, 50] of object 'one' with type 'char [50]' [-Werror=array-bounds]

so the #pragma has an effect, but there is something weird with ignored and -Wstringop-overflow interaction.
Comment 5 Manuel López-Ibáñez 2019-11-02 13:08:11 UTC
 333 Warray-bounds
 334 LangEnabledBy(C ObjC C++ LTO ObjC++)
 335 ; in common.opt

This seems wrong, the second argument ", Wall" is missing. Moreover, this probably should be an Alias for some -Warray-bounds= option.

Nevertheless, there is indeed something weird going on with the locations of the #pragma:

char one[50];
char two[50];

void
test_strncat (void)
{
#pragma GCC diagnostic error "-Warray-bounds"
  (void) __builtin_strncat (one, two, 99); 
  (void) __builtin_strcpy (one, "gh");
#pragma GCC diagnostic warning "-Warray-bounds"
  (void) __builtin_strcpy (two, "ef");
  (void) __builtin_strncat (one, two, 99); 
}
//  -O2 -Warray-bounds

gives 2 errors. It seems that if the #pragma is at the start of the function, it has effect, otherwise it doesn't.
Comment 6 Jeffrey A. Law 2020-02-28 22:56:50 UTC
This really looks like a line mapping issue to me. I think Manu is totally offbase.

At the time of the diagnostic we have 3 items in context->classification_history. One for each of the ignored diagnostics and one for the pop.

(gdb) p context->classification_history[2]
$32 = {location = 295618, option = 0, kind = DK_POP}
(gdb) p context->classification_history[1]
$33 = {location = 287426, option = 419, kind = DK_IGNORED}
(gdb) p context->classification_history[0]
$34 = {location = 283330, option = 669, kind = DK_IGNORED}

Now the diagnostic location looks like:

(gdb) p location
$35 = 2147483652

Now I can actually map that to a normal location by putting a breakpoint in linemap_compare_locations :-)  It happens to correspond to location 267552.

So as far as the compiler is concerned we're emitting a diagnostic for a location which is *before* all the pragmas AFAICT.  So it's no great surprise that the pragmas have no effect on the diagnostic.

We initially set the location of the strcat call to 291583 which would work beautifully.  We then change the location via lower_stmt->gimple_set_block to a rather inconvenient value that's outside the scope of the pragmas and thus the pragmas have no effect.
Comment 7 Jeffrey A. Law 2020-02-28 23:14:52 UTC
Another tidbit.  It looks like the sprintf warning will at times ignore the passed in location.  I'm not suggesting this is necessarily the right fix, but if we make gimple-ssa-warn-restrict honor the passed in location per this change, then the pragmas work perfectly in this case.

diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
index 2c582a670eb..1449492798a 100644
--- a/gcc/gimple-ssa-warn-restrict.c
+++ b/gcc/gimple-ssa-warn-restrict.c
@@ -1754,7 +1754,7 @@ maybe_diag_access_bounds (location_t loc, gimple *call, tree func, int strict,
       || (ref.ref && TREE_NO_WARNING (ref.ref)))
     return false;
 
-  if (EXPR_HAS_LOCATION (ref.ptr))
+  if (loc == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (ref.ptr))
     loc = EXPR_LOCATION (ref.ptr);
 
   loc = expansion_point_location_if_in_system_header (loc);


Martin, do you recall the rationale behind extracting the location out of ref.ptr when we were already passed in a location?
Comment 8 CVS Commits 2020-03-05 21:02:34 UTC
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:55ace4d14637866466498ed43e02d6f95af98f10

commit r10-7049-g55ace4d14637866466498ed43e02d6f95af98f10
Author: Jeff Law <law@redhat.com>
Date:   Thu Mar 5 14:01:30 2020 -0700

    Fix location maybe_diag_overlap passes to diagnostics so that diagnostic pragmas work better.
    
    	PR tree-optimization/91890
    	* gimple-ssa-warn-restrict.c (maybe_diag_overlap): Remove LOC argument.
    	Use gimple_or_expr_nonartificial_location.
    	(check_bounds_overlap): Drop LOC argument to maybe_diag_access_bounds.
    	Use gimple_or_expr_nonartificial_location.
    	* gimple.c (gimple_or_expr_nonartificial_location): New function.
    	* gimple.h (gimple_or_expr_nonartificial_location): Declare it.
    	* tree-ssa-strlen.c (maybe_warn_overflow): Use
    	gimple_or_expr_nonartificial_location.
    	(maybe_diag_stxncpy_trunc, handle_builtin_stxncpy_strncat): Likewise.
    	(maybe_warn_pointless_strcmp): Likewise.
    
    	* gcc.dg/pragma-diag-8.c: New test.
Comment 9 Jeffrey A. Law 2020-03-05 21:04:50 UTC
Should be fixed on the trunk now.  I'm going to be opening a distinct bug for cleanup/removal of the code which extracts a (likely incorrect) location from the expression -- I wasn't comfortable moving on that during stage4.
Comment 10 Jeffrey A. Law 2020-03-05 21:06:55 UTC
Per commit on the trunk.