On 1/18/19 4:43 PM, Martin Sebor wrote: > On 1/18/19 2:35 AM, Christophe Lyon wrote: >> Hi Martin, >> >> >> On Thu, 17 Jan 2019 at 02:51, Martin Sebor <mse...@gmail.com> wrote: >>> >>> On 1/16/19 6:14 PM, Jeff Law wrote: >>>> On 1/15/19 8:21 AM, Martin Sebor wrote: >>>>> On 1/15/19 4:07 AM, Richard Biener wrote: >>>>>> On Tue, Jan 15, 2019 at 1:08 AM Martin Sebor <mse...@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> The gimple_fold_builtin_memory_op() function folds calls to memcpy >>>>>>> and similar to MEM_REF when the size of the copy is a small power >>>>>>> of 2, but it does so without considering whether the copy might >>>>>>> write (or read) past the end of one of the objects. To detect >>>>>>> these kinds of errors (and help distinguish them from -Westrict) >>>>>>> the folder calls into the wrestrict pass and lets it diagnose them. >>>>>>> Unfortunately, that can lead to false positives for even some fairly >>>>>>> straightforward code that is ultimately found to be unreachable. >>>>>>> PR 88800 is a report of one such problem. >>>>>>> >>>>>>> To avoid these false positives the attached patch adjusts >>>>>>> the function to avoid issuing -Warray-bounds for out-of-bounds >>>>>>> calls to memcpy et al. Instead, the patch disables the folding >>>>>>> of such invalid calls (and only those). Those that are not >>>>>>> eliminated during DCE or other subsequent passes are eventually >>>>>>> diagnosed by the wrestrict pass. >>>>>>> >>>>>>> Since this change required removing the dependency of the detection >>>>>>> on the warning options (originally done as a micro-optimization to >>>>>>> avoid spending compile-time cycles on something that wasn't needed) >>>>>>> the patch also adds tests to verify that code generation is not >>>>>>> affected as a result of warnings being enabled or disabled. With >>>>>>> the patch as is, the invalid memcpy calls end up emitted (currently >>>>>>> they are folded into equally invalid MEM_REFs). At some point, >>>>>>> I'd like us to consider whether they should be replaced with traps >>>>>>> (possibly under the control of as has been proposed a number of >>>>>>> times in the past. If/when that's done, these tests will need to >>>>>>> be adjusted to look for traps instead. >>>>>>> >>>>>>> Tested on x86_64-linux. >>>>>> >>>>>> I've said in the past that I feel delaying of folding is wrong. >>>>>> >>>>>> To understand, the PR is about emitting a warning for out-of-bound >>>>>> accesses in a dead code region? >>>>> >>>>> Yes. I am keeping in my mind your preference of not delaying >>>>> the folding of valid code. >>>>> >>>>>> >>>>>> If we think delaying/disablign the folding is the way to go the >>>>>> patch looks OK. >>>>> >>>>> I do, at least for now. I'm taking this as your approval to commit >>>>> the patch (please let me know if you didn't mean it that way). >>>> Note we are in stage4, so we're supposed to be addressing regression >>>> bugfixes and documentation issues. >>>> >>>> So I think Richi needs to be explicit about whether or not he wants >>>> this in gcc-9 or if it should defer to gcc-10. >>>> >>>> I have no technical objections to the patch and would easily ack it in >>>> stage1 or stage3. >>> >>> The warning is a regression introduced in GCC 8. I was just about >>> to commit the fix so please let me know if I should hold off until >>> stage 1. >>> >> >> After your commit (r268037), I'm seeing excess errors on some arm >> targets: >> FAIL: c-c++-common/Wrestrict.c -Wc++-compat (test for excess errors) >> Excess errors: >> /gcc/testsuite/c-c++-common/Wrestrict.c:195:3: warning: 'memcpy' >> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2 >> bytes at offset [2, 3] [-Wrestrict] >> /gcc/testsuite/c-c++-common/Wrestrict.c:202:3: warning: 'memcpy' >> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2 >> bytes at offset [2, 3] [-Wrestrict] >> /gcc/testsuite/c-c++-common/Wrestrict.c:207:3: warning: 'memcpy' >> accessing 4 bytes at offsets [2, 3] and 0 overlaps between 1 and 2 >> bytes at offset [2, 3] [-Wrestrict] >> >> >> This is not true for all arm toolchains, so for instance if you want >> to reproduce it, you can build for target arm-eabi and keep default >> cpu/fpu/mode. > > The warnings are valid, the test just hardcodes the wrong byte counts > in the xfailed dg-warning directives. I've fixed the byte counts so > that the test just shows XPASSes. > > The other issue here is that the -Wrestrict warning only triggers for > built-ins and whether GCC keeps those around or folds them to MEM_REFs > depends on the target. On common targets, a memcpy (d, d + 2, 4) call, > for instance, (i.e., one with a small power-of-2 size) is folded to > MEM_REF, so there is no -Wrestrict warning despite the overlap. > Strictly, it's a false negative, but in reality it's not a problem > because GCC gives the MEM_REF copy the same safe semantics as with > memmove, so the overlap is benign. > > But on targets that optimize for space by default (like arm-eabi) > the folding doesn't happen, memcpy gets called for the overlapping > regions, and we get the helpful warning. > > If there was a way to tell at compile time which target the test is > being compiled for, whether a folding or non-folding one, that would > give us a way to conditionalize the dg-warnings and avoid these pesky > regressions. I just posted a patch to do that so if it's approved, > these failures should all be resolved. Ultimately, though, I'd like > to make the warnings detect invalid accesses in MEM_REFs as much as > in built-in calls, so this should be just a temporary stop-gap fix. > > https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01111.html I'm not sure if a way to control or predict this stuff. It's dependent on the target's capabilities, its MOVE_RATIO defintion as well as any conditions in its block move patterns.
The only way would be to auto-detect this kind of stuff in the dejagnu framework by passing in suitable C code and analyzing the result. Ugh. Not having good knobs/switches on this stuff is a bit of a design & implementation wart. I noticed in another thread you were investigating -m<whatever>-strategy. All -m arguments are supposed to be target specific, so you can't rely on those for generic tests. jeff
