On 2/8/21 4:26 PM, Jeff Law wrote:
On 2/8/21 4:07 PM, Martin Sebor wrote:
On 2/8/21 12:09 PM, Jeff Law wrote:
On 2/3/21 3:45 PM, Martin Sebor wrote:
On 2/3/21 2:57 PM, Jeff Law wrote:
On 1/28/21 4:03 PM, Martin Sebor wrote:
The GCC 11 -Warray-bounds enhancement to diagnose accesses whose
leading offset is in bounds but whose trailing offset is not has
been causing some confusion. When the warning is issued for
an access to an in-bounds member via a pointer to a struct that's
larger than the pointed-to object, phrasing this strictly invalid
access in terms of array subscripts can be misleading, especially
when the source code doesn't involve any arrays or indexing.
Since the problem boils down to an aliasing violation much more
so than an actual out-of-bounds access, the attached patch adjusts
the code to issue a -Wstrict-aliasing warning in these cases instead
of -Warray-bounds. In addition, since the aliasing assumptions in
GCC can be disabled by -fno-strict-aliasing, the patch also makes
these instances of the warning conditional on -fstrict-aliasing
being in effect.
Martin
gcc-98503.diff
PR middle-end/98503 -Warray-bounds when -Wstrict-aliasing would be
more appropriate
gcc/ChangeLog:
PR middle-end/98503
* gimple-array-bounds.cc (array_bounds_checker::check_mem_ref):
Issue -Wstrict-aliasing for a subset of violations.
(array_bounds_checker::check_array_bounds): Set new member.
* gimple-array-bounds.h (array_bounds_checker::cref_of_mref):
New
data member.
gcc/testsuite/ChangeLog:
PR middle-end/98503
* g++.dg/warn/Warray-bounds-10.C: Adjust text of expected
warnings.
* g++.dg/warn/Warray-bounds-11.C: Same.
* g++.dg/warn/Warray-bounds-12.C: Same.
* g++.dg/warn/Warray-bounds-13.C: Same.
* gcc.dg/Warray-bounds-63.c: Avoid -Wstrict-aliasing. Adjust
text
of expected warnings.
* gcc.dg/Warray-bounds-66.c: Adjust text of expected warnings.
* gcc.dg/Wstrict-aliasing-2.c: New test.
* gcc.dg/Wstrict-aliasing-3.c: New test.
What I don't like here is the strict-aliasing warnings inside the file
and analysis phase for array bounds checking.
ISTM that catching this at cast time would be better. So perhaps in
build_c_cast and the C++ equivalent?
It would mean we're warning at the cast site rather than the
dereference, but that may ultimately be better for the warning anyway.
I'm not sure.
I had actually experimented with a this (in the middle end, and only
for accesses) but even that caused so many warnings that I abandoned
it, at least for now. -Warray-bounds has the benefit of flow analysis
(and dead code elimination). In the front end it would have neither
and be both excessively noisy and ineffective at the same time. For
example:
I think we agree that this really is an aliasing issue and I would go
further and claim that it has nothing to do with array bounds checking.
Therefore warning for it within gimple-array-bounds just seems wrong.
I realize that you like having DCE run and the ability to look at
offsets and such, but it really feels like the wrong place to do this.
Aliasing issues are also more of a front-end thing since the language
defines what is and what is not valid aliasing -- one might even argue
that any aliasing warning needs to be identified by the front-ends
exclusively (though possibly pruned away later).
The aliasing restrictions are on accesses, which are [defined in
C as] execution-time actions on objects. Analyzing execution-time
actions unavoidably depends on flow analysis which the front ends
have only very limited support for (simple expressions only).
I gave one example showing how the current -Wstrict-aliasing in
the front end is ineffective against all but the most simplistic
bugs, but there are many more. For instance:
I'm aware of all this. Even so I think trying to deal with strict
aliasing in the middle-end is fundamentally wrong. The middle end is
not C and it can not assume C semantics and putting the warning in the
middle of the array bounds pass is, IMHO, just wrong.
All the existing access warnings in the middle end are designed
around C/C++ (and other language) rules, just as much as all middle
end optimizations, including aliasing, rely on them. I don't see
how this instance is any different.
-Warray-bounds is issued in the VRP pass (there is no dedicated
array bounds pass). It just happens to live in a file recently
named gimple-array-bounds.cc. The name was chosen only because
that was the only warning that happened to be implemented there
at the time. I have no problem with keeping separate things
separate but I also see nothing wrong with issuing two or more
warnings under different options from the same code/pass,
especially when they're related or when both employ the same
analysis. Some bugs are the results of violating multiple rules
and which one is chosen in each instance as the most representative
is a matter of deciding which is more important or relevant. For
example, in:
struct A { int a[2]; char *p; };
void f (struct A *p)
{
p->a[2] = 123;
}
the store to p->a[2] is both an out of bounds access and
an aliasing violation. If -Wstrict-aliasing is enhanced to run
in the middle end but independently from -Warray-bounds we will
end up with two warnings for this bug and have to work extra
hard to suppress one or the other. This is already the case with
-Warray-bounds and -Wstringop-overflow. There are benefits to
keeping related warnings together.
That doesn't change the need to address the problems with the warning,
namely that it makes references to array bounds violations for accesses
which aren't arrays.
The patch we are discussing does that for the -Wstrict-aliasing
subset of the warning.
More still can be done. -Warray-bounds refers to arrays even for
singleton objects purely for simplicity -- there's no difference
between a singleton object and an array of one element. This has
been so for a few releases now but can be changed.
The details of *how* to address those problems are still quite fuzzy.
The most obvious ideas to me are either rely on something like
__builtin_warning or to mark the problem expressions in the front-end,
but defer issuing the warning until we're done with the optimization
pipeline.
The purpose of the __builtin_warning I designed and implemented
in 2019 is to let DCE eliminate instances issued early on about
problems in conditional code that's later discovered to be
unreachable. The built-in itself carries no semantic information,
just the text of the warning. In straight line code like in
the example I gave:
int i;
void *p = &i; // valid
float *q = p; // valid
*q = 0; // aliasing violation
there isn't anything to annotate with __builtin_warning until
we know that the *q is used to access i, which we can't tell in
the FE. A -Wstrict-aliasing implementation early on in the middle
end (when the type-morphing placement new calls are still in
the IL so that the invalid *q = 0 can be distinguished from
the valid new (q) int()) could emit __builtin_warning at that
point and rely on later passes to eliminate it.
But I'm open to suggestions for other annotations/approaches.
Martin
PS For reference here's the __builtin_warning patch:
https://gcc.gnu.org/legacy-ml/gcc-patches/2019-10/msg01015.html
