On Fri, 2020-01-31 at 12:04 -0700, Martin Sebor wrote:
> Attached is a reworked patch since the first one didn't go far
> enough to solve the major problems. The new solution relies on
> get_range_strlen_dynamic the same way as the sprintf optimization,
> and does away with the determine_min_objsize function and calling
> compute_builtin_object_size.
>
> To minimize testsuite fallout I extended get_range_strlen to handle
> a couple more kinds expressions(*), but I still had to xfail and
> disable a few tests that were relying on being able to use the type
> of the destination object as the upper bound on the string length.
>
> Tested on x86_64-linux.
>
> Martin
>
> [*] With all the issues around MEM_REFs and types this change needs
> extra scrutiny. I'm still not sure I fully understand what can and
> what cannot be safely relied on at this level.
>
> On 1/15/20 6:18 AM, Martin Sebor wrote:
> > The strcmp optimization newly introduced in GCC 10 relies on
> > the size of the smallest referenced array object to determine
> > whether the function can return zero. When the size of
> > the object is smaller than the length of the other string
> > argument the optimization folds the equality to false.
> >
> > The bug report has identified a couple of problems here:
> > 1) when the access to the array object is via a pointer to
> > a (possibly indirect) member of a union, in GIMPLE the pointer
> > may actually point to a different member than the one in
> > the original source code. Thus the size of the array may
> > appear to be smaller than in the source code which can then
> > result in the optimization being invalid.
> > 2) when the pointer in the access may point to two or more
> > arrays of different size (i.e., it's the result of a PHI),
> > assuming it points to the smallest of them can also lead
> > to an incorrect result when the optimization is applied.
> >
> > The attached patch adjusts the optimization to 1) avoid making
> > any assumptions about the sizes of objects accessed via union
> > types, and b) use the size of the largest object in PHI nodes.
> >
> > Tested on x86_64-linux.
> >
> > Martin
>
>
> PR tree-optimization/92765 - wrong code for strcmp of a union member
>
> gcc/ChangeLog:
>
> PR tree-optimization/92765
> * gimple-fold.c (get_range_strlen_tree): Handle MEM_REF and PARM_DECL.
> * tree-ssa-strlen.c (compute_string_length): Remove.
> (determine_min_objsize): Remove.
> (get_len_or_size): Add an argument. Call get_range_strlen_dynamic.
> Avoid using type size as the upper bound on string length.
> (handle_builtin_string_cmp): Add an argument. Adjust.
> (strlen_check_and_optimize_call): Pass additional argument to
> handle_builtin_string_cmp.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/92765
> * g++.dg/tree-ssa/strlenopt-1.C: New test.
> * g++.dg/tree-ssa/strlenopt-2.C: New test.
> * gcc.dg/Warray-bounds-58.c: New test.
> * gcc.dg/Wrestrict-20.c: Avoid a valid -Wformat-overflow.
> * gcc.dg/Wstring-compare.c: Xfail a test.
> * gcc.dg/strcmpopt_2.c: Disable tests.
> * gcc.dg/strcmpopt_4.c: Adjust tests.
> * gcc.dg/strcmpopt_10.c: New test.
> * gcc.dg/strlenopt-69.c: Disable tests.
> * gcc.dg/strlenopt-92.c: New test.
> * gcc.dg/strlenopt-93.c: New test.
> * gcc.dg/strlenopt.h: Declare calloc.
> * gcc.dg/tree-ssa/pr92056.c: Xfail tests until pr93518 is resolved.
> * gcc.dg/tree-ssa/builtin-sprintf-warn-23.c: Correct test (pr93517).
>
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index ed225922269..d70ac67e1ca 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
> @@ -1280,7 +1280,7 @@ get_range_strlen_tree (tree arg, bitmap *visited,
> strlen_range_kind rkind,
> c_strlen_data *pdata, unsigned eltsize)
> {
> gcc_assert (TREE_CODE (arg) != SSA_NAME);
> -
> +
> /* The length computed by this invocation of the function. */
> tree val = NULL_TREE;
>
> @@ -1422,7 +1422,42 @@ get_range_strlen_tree (tree arg, bitmap *visited,
> strlen_range_kind rkind,
> type about the length here. */
> tight_bound = true;
> }
> - else if (VAR_P (arg))
> + else if (TREE_CODE (arg) == MEM_REF
> + && TREE_CODE (TREE_TYPE (arg)) == ARRAY_TYPE
> + && TREE_CODE (TREE_TYPE (TREE_TYPE (arg))) == INTEGER_TYPE
> + && TREE_CODE (TREE_OPERAND (arg, 0)) == ADDR_EXPR)
> + {
> + /* Handle a MEM_REF into a DECL accessing an array of integers,
> + being conservative about references to extern structures with
> + flexible array members that can be initialized to arbitrary
> + numbers of elements as an extension (static structs are okay).
> + FIXME: Make this less conservative -- see
> + component_ref_size in tree.c. */
I think it's generally been agreed that we can look at sizes of _DECL
nodes and this code doesn't look like this walks backwards through
casts or anything like that. So the worry would be if we forward
propagated through a cast into the MEM_REF node.
It looks like forwprop only propagates through "compatible" pointer
conversions. It makes me a bit nervous.
Jakub/Richi, comments on this hunk?
> diff --git a/gcc/testsuite/gcc.dg/Wstring-compare.c
> b/gcc/testsuite/gcc.dg/Wstring-compare.c
> index 0ca492db0ab..d1534bf7555 100644
> --- a/gcc/testsuite/gcc.dg/Wstring-compare.c
> +++ b/gcc/testsuite/gcc.dg/Wstring-compare.c
In general I have a slight preference for pulling these into new files
when we need to xfail them. Why? Because a test which previously
passed, but now fails (even an xfail) causes the tester to flag the
build as failing due to a testsuite regression.
But I don't think that preference is significant enough to ask you to
redo the work. Just something to ponder in the future.
> diff --git a/gcc/testsuite/gcc.dg/strcmpopt_2.c
> b/gcc/testsuite/gcc.dg/strcmpopt_2.c
> index 57d8f651c28..f31761be173 100644
> --- a/gcc/testsuite/gcc.dg/strcmpopt_2.c
> +++ b/gcc/testsuite/gcc.dg/strcmpopt_2.c
So I'd pulled f1, f3, f5, f7 into a new file. But disabling them like
you've done is reasonable as well.
> diff --git a/gcc/testsuite/gcc.dg/strcmpopt_4.c
> b/gcc/testsuite/gcc.dg/strcmpopt_4.c
> index 4e26522eed1..b07fbb6b7b0 100644
> --- a/gcc/testsuite/gcc.dg/strcmpopt_4.c
> +++ b/gcc/testsuite/gcc.dg/strcmpopt_4.c
THanks for creating the new test. I'd done the exact same thing in my
local tree. I'm a little surprised that f_param passes. Can you
double check that?
diff --git a/gcc/testsuite/gcc.dg/strlenopt-69.c
b/gcc/testsuite/gcc.dg/strlenopt-69.c
> index 9ad8e2e8aac..9df6eeccb97 100644
> --- a/gcc/testsuite/gcc.dg/strlenopt-69.c
> +++ b/gcc/testsuite/gcc.dg/strlenopt-69.c
I'd pulled the offending tests into a new file, but your approach is
fine too.
>
> diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
> index ad9e98973b1..b9972c16e18 100644
> --- a/gcc/tree-ssa-strlen.c
> +++ b/gcc/tree-ssa-strlen.c
> -
> -/* Given strinfo IDX for ARG, set LENRNG[] to the range of lengths
> - of the string(s) referenced by ARG if it can be determined.
> - If the length cannot be determined, set *SIZE to the size of
> +/* Given strinfo IDX for ARG, sets LENRNG[] to the range of lengths
> + of the string(s) referenced by ARG if it can be determined.
> + If the length cannot be determined, sets *SIZE to the size of
> the array the string is stored in, if any. If no such array is
> - known, set *SIZE to -1. When the strings are nul-terminated set
> - *NULTERM to true, otherwise to false. Return true on success. */
> + known, sets *SIZE to -1. When the strings are nul-terminated sets
> + *NULTERM to true, otherwise to false. When nonnull uses RVALS to
> + determine range information. Returns true on success. */
"When nonnull uses RVALS to detemrine range information." That isn't a
sentence and just doesn't seem to make sense. Please review for
comment clarity.
This looks OK to me with the comment fixed. I like that we drop the
whole determine_min_objsize and replace it it the standard range bits
that we're using elsewhere.
Please give Richi and Jakub time to chime in on the gimple-fold.c
changes.
Jeff
