Re: [PATCH] avoid issuing -Warray-bounds during folding (PR 88800)

[Christophe Lyon <christophe.lyon@linaro.org>]

Hi Martin,


On Thu, 17 Jan 2019 at 02:51, Martin Sebor <msebor@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 <msebor@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.
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