| Message ID | 0a6beda3-143e-3a12-5a0b-741dd00a07b9@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | suppress -Wstringop-overflow when no-warning is set (PR 83508) | expand |
On 01/10/2018 01:26 PM, Martin Sebor wrote: > To avoid issuing duplicate warnings for the same function call > in the source code the -Wrestrict warning code makes sure > the no-warning bit is propagated between trees and GIMPLE and > tested before issuing a warning. But the warning also detects > some of the same problems as -Wstringop-overflow, and that > warning was not updated to pay attention to the no-warning bit. > This can in turn lead to two warnings for what boils down to > the same bug. The warnings can be confusing when the first > one references the function as it appears in the source code > and the second one the one it was transformed to by GCC after > the first warning was issued. > > The attached patch corrects this oversight by having > the buffer overflow checker test the no-warning bit and skip > issuing a diagnostic. (The function that does the overflow > checking still runs so that it can continue to indicate to its > callers whether an overflow has been detected.) > > Bootstrap on x86_64-linux in progress. > > Martin > > gcc-83508.diff > > > PR other/83508 - c-c++-common/Wrestrict.c fails since r255836 > > gcc/testsuite/ChangeLog: > > PR other/83508 > * gcc.dg/Wstringop-overflow-2.c: New test. > > gcc/ChangeLog: > > PR other/83508 > * builtins.c (check_access): Avoid warning when the no-warning bit > is set. Would it be better to check for TREE_NO_WARNING at the very start of check_access rather than sprinkling it at various sites later in the code? Jeff
On 01/11/2018 04:24 PM, Jeff Law wrote: > On 01/10/2018 01:26 PM, Martin Sebor wrote: >> To avoid issuing duplicate warnings for the same function call >> in the source code the -Wrestrict warning code makes sure >> the no-warning bit is propagated between trees and GIMPLE and >> tested before issuing a warning. But the warning also detects >> some of the same problems as -Wstringop-overflow, and that >> warning was not updated to pay attention to the no-warning bit. >> This can in turn lead to two warnings for what boils down to >> the same bug. The warnings can be confusing when the first >> one references the function as it appears in the source code >> and the second one the one it was transformed to by GCC after >> the first warning was issued. >> >> The attached patch corrects this oversight by having >> the buffer overflow checker test the no-warning bit and skip >> issuing a diagnostic. (The function that does the overflow >> checking still runs so that it can continue to indicate to its >> callers whether an overflow has been detected.) >> >> Bootstrap on x86_64-linux in progress. >> >> Martin >> >> gcc-83508.diff >> >> >> PR other/83508 - c-c++-common/Wrestrict.c fails since r255836 >> >> gcc/testsuite/ChangeLog: >> >> PR other/83508 >> * gcc.dg/Wstringop-overflow-2.c: New test. >> >> gcc/ChangeLog: >> >> PR other/83508 >> * builtins.c (check_access): Avoid warning when the no-warning bit >> is set. > Would it be better to check for TREE_NO_WARNING at the very start of > check_access rather than sprinkling it at various sites later in the code? The function is called from a couple of places to check that there is no overflow and avoid expansion (we discussed this not too long ago when I enhanced compute_objsize() to handle ranges). I didn't want to change that (I mention it in the last sentence above.) Looking forward, I would like to see these middle-end checkers used to avoid making certain kinds of transformations that we know are dangerous (we discussed emitting __builtin_trap or __builtin_unreachable) for some such cases. I realize they aren't suitable for it quite it yet and will need work to make them so (assuming we can agree on that approach), but I mention it to explain what I was thinking when I sprinkled the tests in check_access() the way I did. As an aside, even though I think GCC should issue only one warning per function, I'm not sure that -Wrestrict should trump -Wstringop-overflow. It seems like it should be the other way around because the latter is more severe. That's also how it worked before -Wrestrict was moved into its own pass. To make it work like that again, -Wstringop-overflow would need to run either before -Wrestrict or at the same time. Maybe it should be moved in GCC 9. Martin
On 01/11/2018 04:49 PM, Martin Sebor wrote: > On 01/11/2018 04:24 PM, Jeff Law wrote: >> On 01/10/2018 01:26 PM, Martin Sebor wrote: >>> To avoid issuing duplicate warnings for the same function call >>> in the source code the -Wrestrict warning code makes sure >>> the no-warning bit is propagated between trees and GIMPLE and >>> tested before issuing a warning. But the warning also detects >>> some of the same problems as -Wstringop-overflow, and that >>> warning was not updated to pay attention to the no-warning bit. >>> This can in turn lead to two warnings for what boils down to >>> the same bug. The warnings can be confusing when the first >>> one references the function as it appears in the source code >>> and the second one the one it was transformed to by GCC after >>> the first warning was issued. >>> >>> The attached patch corrects this oversight by having >>> the buffer overflow checker test the no-warning bit and skip >>> issuing a diagnostic. (The function that does the overflow >>> checking still runs so that it can continue to indicate to its >>> callers whether an overflow has been detected.) >>> >>> Bootstrap on x86_64-linux in progress. >>> >>> Martin >>> >>> gcc-83508.diff >>> >>> >>> PR other/83508 - c-c++-common/Wrestrict.c fails since r255836 >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR other/83508 >>> * gcc.dg/Wstringop-overflow-2.c: New test. >>> >>> gcc/ChangeLog: >>> >>> PR other/83508 >>> * builtins.c (check_access): Avoid warning when the no-warning bit >>> is set. >> Would it be better to check for TREE_NO_WARNING at the very start of >> check_access rather than sprinkling it at various sites later in the >> code? > > The function is called from a couple of places to check that > there is no overflow and avoid expansion (we discussed this > not too long ago when I enhanced compute_objsize() to handle > ranges). I didn't want to change that (I mention it in the > last sentence above.) OK. Just wanted to raise it as a possibility. > > Looking forward, I would like to see these middle-end checkers > used to avoid making certain kinds of transformations that we > know are dangerous (we discussed emitting __builtin_trap or > __builtin_unreachable) for some such cases. I realize they > aren't suitable for it quite it yet and will need work to make > them so (assuming we can agree on that approach), but I mention > it to explain what I was thinking when I sprinkled the tests > in check_access() the way I did. Understood. > > As an aside, even though I think GCC should issue only one > warning per function, I'm not sure that -Wrestrict should > trump -Wstringop-overflow. It seems like it should be the > other way around because the latter is more severe. That's > also how it worked before -Wrestrict was moved into its own > pass. To make it work like that again, -Wstringop-overflow > would need to run either before -Wrestrict or at the same > time. Maybe it should be moved in GCC 9. I think we're OK for gcc-8. We can revisit ordering and such for gcc-9. FWIW, I went ahead and committed your patch to the trunk. jeff
PR other/83508 - c-c++-common/Wrestrict.c fails since r255836 gcc/testsuite/ChangeLog: PR other/83508 * gcc.dg/Wstringop-overflow-2.c: New test. gcc/ChangeLog: PR other/83508 * builtins.c (check_access): Avoid warning when the no-warning bit is set. diff --git a/gcc/builtins.c b/gcc/builtins.c index 1d6e69d..a1b6a4e 100644 --- a/gcc/builtins.c +++ b/gcc/builtins.c @@ -3150,6 +3150,9 @@ check_access (tree exp, tree, tree, tree dstwrite, || (tree_fits_uhwi_p (dstwrite) && tree_int_cst_lt (dstwrite, range[0])))) { + if (TREE_NO_WARNING (exp)) + return false; + location_t loc = tree_nonartificial_location (exp); loc = expansion_point_location_if_in_system_header (loc); @@ -3209,6 +3212,9 @@ check_access (tree exp, tree, tree, tree dstwrite, if (tree_int_cst_lt (maxobjsize, range[0])) { + if (TREE_NO_WARNING (exp)) + return false; + /* Warn about crazy big sizes first since that's more likely to be meaningful than saying that the bound is greater than the object size if both are big. */ @@ -3230,6 +3236,9 @@ check_access (tree exp, tree, tree, tree dstwrite, if (dstsize != maxobjsize && tree_int_cst_lt (dstsize, range[0])) { + if (TREE_NO_WARNING (exp)) + return false; + if (tree_int_cst_equal (range[0], range[1])) warning_at (loc, opt, "%K%qD specified bound %E " @@ -3253,6 +3262,9 @@ check_access (tree exp, tree, tree, tree dstwrite, && dstwrite && range[0] && tree_int_cst_lt (slen, range[0])) { + if (TREE_NO_WARNING (exp)) + return false; + location_t loc = tree_nonartificial_location (exp); if (tree_int_cst_equal (range[0], range[1])) diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-2.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-2.c new file mode 100644 index 0000000..6e3e2ca --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-2.c @@ -0,0 +1,30 @@ +/* PR tree-optimization/83508 - c-c++-common/Wrestrict.c fails since r255836 + Test to verify that only one of -Wrestrict and -Wstringop-overflow is + issued for a problem where either would be appropriate. + { dg-do compile } + { dg-options "-O2 -Wrestrict -Wstringop-overflow" } */ + +#define DIFF_MAX __PTRDIFF_MAX__ + +typedef __PTRDIFF_TYPE__ ptrdiff_t; +typedef __SIZE_TYPE__ size_t; + +void sink (void*); + +void f (ptrdiff_t i, size_t n) +{ + if (i < DIFF_MAX - 2 || DIFF_MAX - 1 > i) + i = DIFF_MAX - 2; + + if (n < 4 || 5 < n) + n = 4; + + char a[8] = "012"; + + /* The following could very well be diagnosed by -Wstringop-overflow + instead but there's no way to verify that only one of the two + warnings is issued and the choice of -Wrestrict simply reflects + the fact that -Wrestrict runs before -Wstringop-overflow. */ + __builtin_strncpy (a + i, a, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (a); +}