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
Martin
Or force -march=armv5t when running the test.
To give you an idea, you can look at
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/268043/report-build-info.html
the red cells correspond to the regressions, you can deduce the configure flags.
Christophe
Martin
