This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] fix and improve strlen conditional handling of merged stores (PR 91183, 91294, 91315)
On 8/8/19 7:05 PM, Jeff Law wrote:
On 7/31/19 6:36 PM, Martin Sebor wrote:
More extensive testing of the last week's strlen patch for
PR91183 on various non-mainstream targets and with better tests
has exposed a few gaps and a couple of bugs. The attached patch
addresses all in one change. I considered splitting it up but
in the end decided the changes were small and sufficiently
isolated that it wasn't necessary. (If someone feels strongly
otherwise it can be easily split t up.)
The wrong-code bug (PR 91294) is due to handle_store neglecting
to fully consider the case when a single multi-byte store
involving a PHI of two "strings" the same size (so they are merged
into a single int store) but of unequal length. The function
simply takes the length of the shorter string as the resulting
length when it needs to only set the lower bound of the length
(it does that treating the result as potentially not nul-
terminated).
The gaps are in not handling some MEM_REF forms that come up
in multi-byte assignments (this is the rest of PR 91183 and was
exposed on strictly aligned targets), and in handle_builtin_strlen
discarding the lower bound on a string length instead of exposing
it to downstream passes. This is PR 91315 that was exposed by
a few cases in the existing tests for PR 91294 failing after
the fix for PR 91294.
Tested on x86_64-linux and spot-checked with a sparc-solaris2.11
cross-compiler.
Martin
gcc-91294-2.diff
PR tree-optimization/91315 - missing strlen lower bound of a string known to be at least N characters
PR tree-optimization/91294 - wrong strlen result of a conditional with an offset
PR tree-optimization/91183 - strlen of a strcpy result with a conditional source not folded
gcc/testsuite/ChangeLog:
PR tree-optimization/91315
PR tree-optimization/91294
PR tree-optimization/91183
* gcc.dg/strlenopt-44.c: Avoid using length of 1.
* gcc.dg/strlenopt-70.c: Disable overly optimistic tests.
* gcc.dg/strlenopt-73.c: New test.
* gcc.dg/strlenopt-74.c: New test.
* gcc.dg/strlenopt-75.c: New test.
* gcc.dg/strlenopt-76.c: New test.
* gcc.dg/strlenopt-77.c: New test.
gcc/ChangeLog:
PR tree-optimization/91315
PR tree-optimization/91294
PR tree-optimization/91183
* gimple-fold.c (gimple_fold_builtin_strlen): Add expected argument.
* tree-ssa-strlen.c (set_strlen_range): Add function argument.
(maybe_set_strlen_range): Add expected argument.
(handle_builtin_strlen): Call set_strlen_range.
(count_nonzero_bytes): Add function arguments. Handle strinfo
first. Handle "single" assignment.
(handle_store): Set the lower bound of length for multibyte stores
of unequal lengths.
* tree-ssa-strlen.h (set_strlen_range): Add function argument.
Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c (revision 273914)
+++ gcc/tree-ssa-strlen.c (working copy)
@@ -1434,6 +1434,12 @@ handle_builtin_strlen (gimple_stmt_iterator *gsi)
tree adj = fold_build2_loc (loc, MINUS_EXPR,
TREE_TYPE (lhs), lhs, old);
adjust_related_strinfos (loc, si, adj);
+ /* Use the constant minimim length as the lower bound
s/minimim/minimum/
@@ -3408,7 +3457,13 @@ static bool
}
gimple *stmt = SSA_NAME_DEF_STMT (exp);
- if (gimple_code (stmt) != GIMPLE_PHI)
+ if (gimple_assign_single_p (stmt))
+ {
+ tree rhs = gimple_assign_rhs1 (stmt);
+ return count_nonzero_bytes (rhs, offset, nbytes, lenrange, nulterm,
+ allnul, allnonnul, snlim);
+ }
+ else if (gimple_code (stmt) != GIMPLE_PHI)
return false;
What cases are you handling here? Are there any cases where a single
operand expression on the RHS affects the result. For example, if we've
got a NOP_EXPR which zero extends RHS? Does that change the nonzero
bytes in a way that is significant?
I'm not opposed to handling single operand objects here, I'm just
concerned that we're being very lenient in just stripping away the
operator and looking at the underlying object.
I remember adding the code because of a test failure but not
the specifics anymore. No tests fail with it removed so it
may not be needed. As you know, I've been juggling a few
enhancements in this area and copying code between them as
I need it so it's possible that I copied too much, or that
some other change has obviated it, or also that the test
failed somewhere else and I forgot to copy the test along
with the code I'll remove it until it's needed.
@@ -3795,7 +3824,14 @@ handle_store (gimple_stmt_iterator *gsi)
}
else
si->nonzero_chars = build_int_cst (size_type_node, offset);
- si->full_string_p = full_string_p;
+
+ /* Set FULL_STRING_P only if the length of the strings being
+ written is the same, and clear it if the strings have
+ different lengths. In the latter case the length stored
+ in si->NONZERO_CHARS becomes the lower bound.
+ FIXME: Handle the upper bound of the length if possible. */
+ si->full_string_p = full_string_p && lenrange[0] == lenrange[1];
So there seems to be a disconnect between the comment and the code.
The comment indicates you care about the lengths of the two strings
being the same. But what you're really comparing when the lenrange[0]
== lenrange[1] test is that the min and max of RHS are the same.
The comment tries to make clear that all the arrays on the RHS
of the assignment must have the same length in order to set
FULL_STRING_P. Like here where LENRANGE = { 4, 4, 4 }:
void f (char *s)
{
if (__builtin_strlen (s) != 2)
return;
*(int*)a = i ? 0x11111111 : 0x22222222;
}
but not here where LENRANGE = { 1, 4, 4 }:
*(int*)a = i < 0 ? 0x11111111 : i ? 0x22220022 : 0x33003333;
If the bounds of the range of lengths of all the strings on
the RHS are the same they're all the same length.
I'm open to phrasing it better.
It generally looks reasonable, so I think we just need to reach a
conclusion on the gimple_assign_single_p cases we're trying to handle
and the possible mismatch between the comment and the code.
Do you want me to post another revision with
the gimple_assign_single_p test removed?
Martin