https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93982
Jakub Jelinek <jakub at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |REOPENED
Resolution|FIXED |---
--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I don't really think this is fixed.
Besides the bogus hunk mentioned above (either do it unconditionally, or not at
all, what it does isn't even a good heuristics, the pointer type on the MEM_REF
first operand really is irrelevant both for the VN reasons and for because user
can as the testcase shows use other pointer types too, I'd say the
count_nonzero_bytes function isn't well designed.
The problem is that it doesn't differentiate between whether the EXP it is
looking at are the bytes that are being stored, or is a pointer which points to
those bytes. Each of those cases needs to be treated significantly
differently.
When not counting count_nonzero_bytes called recursively or from the other
count_nonzero_bytes, I see it called from handle_store twice, each time the
argument is the gimple_assign_rhs1 (store), i.e. either the constant that is
being stored (e.g. INTEGER_CST), or e.g. a MEM_REF and similarly in
handle_integral_assign where it is called with the MEM_REF that is being
loaded.
Now, count_nonzero_bytes starts with get_stridx (exp) call. This makes no
sense, that doesn't give you anything related to the bytes being stored, but to
bytes pointed by exp.
Then it goes through into the ADDR_EXPR case and just uses its operand.
Now, with the PR93829 change it at least sets nbytes to something, but
shouldn't e.g. the get_stridx call be done only if nbytes is non-zero? A clean
design would be to have separate functions, one that handles the length of the
exp bytes itself and another one that handles the case where exp points to
those bytes. Otherwise, you just need to hope zero sized objects (such as
MEM_REFs) won't appear anywhere etc.
E.g. in
void
foo (char *p)
{
char buf[16];
__builtin_memcpy (buf, "abcde", 6);
*(char **) p = &buf[0];
...
}
case get_stridx (exp) returns > 0, even when it is in any way relevant to the
store.
The native_encode_expr call should be guarded, the usual check is
CHAR_BIT == 8 && BITS_PER_UNIT == 8.
And the formatting is off, e.g. the hunk mentioned in #c3 is misindented as
whole,
const value_range_equiv *vr
= CONST_CAST (class vr_values *, rvals)
->get_value_range (si->nonzero_chars);
should have been something like:
const value_range_equiv *vr
= (CONST_CAST (class vr_values *, rvals)
->get_value_range (si->nonzero_chars));
or better
vr_values *r = CONT_CAST (vr_values *, rvals);
const value_range_equiv *vr = r->get_value_range (si->nonzero_chars);
