On 6/16/20 3:33 AM, Richard Biener wrote:
On Mon, Jun 15, 2020 at 7:11 PM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
On 6/14/20 12:37 PM, Jeff Law wrote:
On Sat, 2020-06-13 at 17:49 -0600, Martin Sebor wrote:
On 6/13/20 3:50 PM, Sandra Loosemore wrote:
On 6/2/20 6:12 PM, Martin Sebor via Gcc-patches wrote:
The compute_objsize() function started out as a thin wrapper around
compute_builtin_object_size(), but over time developed its own
features to compensate for the other function's limitations (such
as its inability to work with ranges). The interaction of these
features and the limitations has started to become increasingly
problematic as the former function is used in more contexts.
A complete "fix" for all the problems (as well as some other
limitations) that I'm working on will be more extensive and won't
be appropriate for backports. Until then, the attached patch
cleans up the extensions compute_objsize() has accumulated over
the years to avoid a class of false positives.
To make the warnings issued based on the results of the function
easier to understand and fix, the patch also adds an informative
message to many instances of -Wstringop-overflow to point to
the object to which the warning refers. This is especially
helpful when the object is referenced by a series of pointer
operations.
Tested by boostrapping on x86_64-linux and building Binutils/GDB,
Glibc, and the Linux kernel with no new warnings.
Besides applying it to trunk I'm looking to backport the fix to
GCC 10.
This patch (commit a2c2cee92e5defff9bf23d3b1184ee96e57e5fdd) has broken
glibc builds on nios2-linux-gnu, when building sysdeps/posix/getaddrinfo.c:
../sysdeps/posix/getaddrinfo.c: In function 'gaih_inet.constprop':
../sysdeps/posix/getaddrinfo.c:1081:3: error: 'memcpy' writing 16 bytes
into a region of size 8 overflows the destination
[-Werror=stringop-overflow=]
1081 | memcpy (&sin6p->sin6_addr,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
1082 | at2->addr, sizeof (struct in6_addr));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../include/netinet/in.h:3,
from ../resolv/bits/types/res_state.h:5,
from ../include/bits/types/res_state.h:1,
from ../nptl/descr.h:35,
from ../sysdeps/nios2/nptl/tls.h:45,
from ../sysdeps/generic/libc-tsd.h:44,
from ../include/../locale/localeinfo.h:224,
from ../include/ctype.h:26,
from ../sysdeps/posix/getaddrinfo.c:57:
../inet/netinet/in.h:249:19: note: destination object 'sin_zero'
249 | unsigned char sin_zero[sizeof (struct sockaddr)
| ^~~~~~~~
I have to say that I don't understand the "note" diagnostic here at all.
:-( Why does it think the destination object is a field of struct
sockaddr_in, while this memcpy is filling in a field of struct
sockaddr_in6? (And, the sin6_addr field is indeed of type struct
in6_addr, matching the sizeof expression.)
Most likely because some earlier pass (from my exchange with Jeff
about this instance of the warning I suspect it's PRE) substitutes
one member for the other in the IL when offsets into them happen
to evaluate to the same offset from the start of the enclosing
object. The Glibc code does this:
Yes, this is the same issue we were discussing privately.
struct sockaddr_in6 *sin6p =
(struct sockaddr_in6 *) ai->ai_addr;
sin6p->sin6_port = st2->port;
sin6p->sin6_flowinfo = 0;
memcpy (&sin6p->sin6_addr,
at2->addr, sizeof (struct in6_addr));
and the warning doesn't see sin6p->sin6_addr as the destination
but something like
&MEM <struct sockaddr_in> [(void *)ai_10 + 4B].sin_zero;
The details in this and all other middle end warnings are only as
reliable as the IL they work with. If the IL that doesn't correspond
to the original source code they're going to be confusing (and may
trigger false positives).
True, but the transformation done by PRE is valid. PRE is concerned only with
value equivalences and the two addresses are the same and PRE can and will
replace one with the other.
That's fine. Since they are treated as equivalent it shouldn't
matter which of the equivalent alternatives is chosen (there
may be many). It's the particular choice of the smaller member
that makes it a problem: both in the terms of triggering a false
positive and in terms of the note referencing a member the source
code doesn't use.
If PRE instead picked the bigger member it wouldn't necessarily
trigger the warning. But if that member was also too small,
the note might still reference the wrong member.
But if PRE picked another equivalent representation not involving
any member at all but rather an offset from the base object (i.e.,
just a MEM_REF) there would be no problem either way: no false
positive, and if it overflowed, the warning wouldn't reference
any member but just the base object.
Instead of substituting one member for another in the COMPONENT_REF
when both happen to be accessed at the same offset, using a MEM_REF
alone into the enclosing struct or union plus the offset of
the members would avoid the problem. Something like this:
Ultimately that's just a bandaid over a flawed implementation. Fundamentally
the
problem is the diagnostics should not be depending on the type of those MEM
expressions. As long as we continue to do that we're going to run into
problems.
Hence my suggestion we look at attaching suitable type information to the calls
early in the pipeline, possibly at AST generation time.
It's not the MEM_REF type that's a problem here. The general
issue with the reliability of late warnings like this one also
isn't isolated to just function calls. Any checked statement
is susceptible to them.
In this case, the problem is that the COMPONENT_REF member is
not the one referenced in the source code. So more reliable
type information for MEM_REFs wouldn't solve it: we would also
need more reliable COMPONENT_REFs.
I can think of a few ways to deal with it.
1) Introduce some alternate, more reliable, on-the-side
representation for warnings (your suggestion).
2) Accept that warnings that depend on these transforms are
inherently prone to false positives as a result (the status
quo).
3) Tighten up the requirements on the transforms/representations
to more faithfully correspond to the source code (my suggestion).
Taken to its natural conclusion, (1) means we would need a whole
separate IL just for warnings. That doesn't seem sustainable.
(2) is clearly suboptimal for users. Issuing some false positives
may be defensible but referencing code that doesn't even exist in
the source is wrong.
Meeting the needs of both optimizations and warnings within the same
IL (i.e., (3)), seems like the right and only viable approach to me.
For the case of this warning, when both of these
&MEM[(struct sockaddr_in6 *)ai_10 + 4B].sin6_addr;
&MEM <struct sockaddr_in> [(void *)ai_10 + 4B].sin_zero;
are treated as equivalent (because they evaluate to the same
address), is there some disadvantage or difficulty in encoding
them as
&MEM <TYPE> [(void *)ai_10 + 4B + offsetof sin_zero];
instead?
Well, we are _explicitely_ not doing this for the sake of diagnostics...
Not doing the transformation I suggest at all, or not doing it in
cases when the member does match the source?
I'm suggesting to use the MEM_REF (offset) form only when the member
doesn't correspond to the source.
It also falls apart if you have
&MEM[].a[i_1] vs. &MEM[].b[i_1]
do you want us to look for which array, a or b, is the bigger?
If the expression in the source code is 'p->a[i]' and the IL ends
up with p->b[j] then, as I replied to Jeff, that's also a problem.
All the access warnings assume the object in an ARRAY_REF and
COMPONENT_REF (and MEM_REF) corresponds to what's in the source
code.
But sure - specifically talking about PRE - we can simplify
expressions inserted on the fly easily - we even pre-compute
that offset and it is readily available. What we cannot
do is do this _only_ when there are "conflicting" expressions
(because we only know one of them at that point), we'd do
it always. Look into create_component_ref_by_pieces_1
and the vn_reference_op_t's off member (see vn_reference_eq
how it's used to only consider offsets). It will be a bit iffy
to fold trailing recursive elements into the MEM_REF base
but I guess by modifying the MEM_REF type and offset
in-place instead of attaching a component with non -1 offset
would be easiest.
Thanks, I'll look into it.
You are suggesting to drop information (the access path that
carries no semantics on GIMPLE). So you're fine with
removing the crippling of FRE before inlining (which is
there to preserve access paths to make __builtin_object_size
"happy"?).
I'm looking for feedback on the suggestion. I don't know all
the "tricks" in place to make __builtin_object_size (or other
things) happy. If it isn't viable or if there are better/easier
approaches I'd like to know before I spend time trying to get it
to work.
For what it's worth, my hope is to ultimately make the following
possible:
1) Replace the _FORTIFY_SOURCE machinery with attribute access
(i.e., have calls to functions with attribute access emit
the same runtime checks as done in the __builtin____xxxcpy_chk()
functions.
2) Lay the foundation for a future implementation of C++ Contracts
(function attributes to express arbitrary constraints on function
arguments and return values checked at compile time if possible
and enforced at runtime).
(2) is a generalization of (1). The primary purpose of both is
detecting bugs. The secondary purpose is enabling optimization.
So I'd like whatever I/we do now to be aligned with this bigger
goal (or at least not fundamentally incompatible with it).
Martin
Mind you can't have both ;)
Richard.
Martin
