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
