On 8/28/19 3:12 PM, Martin Sebor wrote: > On 8/22/19 3:31 PM, Jeff Law wrote: >> On 8/20/19 8:10 PM, Martin Sebor wrote: >>> Jeff, >>> >>> Please let me know if you agree/disagree and what I need to >>> do to advance this work: >>> >>> https://gcc.gnu.org/ml/gcc-patches/2019-08/msg00643.html >> For the official record, I agree :-) > > Great! :) > > Any comments/suggestions on the patch? > > https://gcc.gnu.org/ml/gcc-patches/2019-08/msg00643.html > > Martin Yea, they were in an earlier message. I'll extract the relevant comments since some we addressed independently:
>> >> >> >> @@ -325,7 +333,7 @@ state_ident_by_name (const char *name, enum >> insert_option optins) >> namlen = strlen (name); >> stid = >> (struct state_ident_st *) xmalloc (sizeof (struct state_ident_st) + >> - namlen); >> + namlen + 1); >> memset (stid, 0, sizeof (struct state_ident_st) + namlen); >> strcpy (stid->stid_name, name); >> *slot = stid; > How did you find this goof? > > > This was more a curiosity than anything. Nothing we need to change here. >> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c >> index fc57fb45e3a..582768090ae 100644 >> --- a/gcc/gimple-fold.c >> +++ b/gcc/gimple-fold.c >> @@ -1346,6 +1346,10 @@ get_range_strlen_tree (tree arg, bitmap *visited, >> strlen_range_kind rkind, >> } >> } >> >> + /* Set if VAL represents the maximum length based on array size (set >> + when exact length cannot be determined). */ >> + bool maxbound = false; >> + >> if (!val && rkind == SRK_LENRANGE) >> { >> if (TREE_CODE (arg) == ADDR_EXPR) >> @@ -1441,6 +1445,7 @@ get_range_strlen_tree (tree arg, bitmap *visited, >> strlen_range_kind rkind, >> pdata->minlen = ssize_int (0); >> } >> } >> + maxbound = true; >> } >> >> if (!val) >> @@ -1454,7 +1459,7 @@ get_range_strlen_tree (tree arg, bitmap *visited, >> strlen_range_kind rkind, >> && tree_int_cst_lt (val, pdata->minlen))) >> pdata->minlen = val; >> >> - if (pdata->maxbound) >> + if (pdata->maxbound && TREE_CODE (pdata->maxbound) == INTEGER_CST) >> { >> /* Adjust the tighter (more optimistic) string length bound >> if necessary and proceed to adjust the more conservative > So inside the conditional guarded by the test you're changing above we have: > > if (TREE_CODE (val) == INTEGER_CST) > { > if (TREE_CODE (pdata->maxbound) == INTEGER_CST) > { > if (tree_int_cst_lt (pdata->maxbound, val)) > pdata->maxbound = val; > } > else > pdata->maxbound = build_all_ones_cst (size_type_node); > } > > Isn't the inner test that pdata->maxbound == INTEGER_CST always true and > we should remove the test and the else clause? Does the else clause > need to be handled elsewhere (I don't see that it would be handled after > your changes). Or perhaps it just doesn't matter... The redundant test of TREE_CODE (pdata->maxbound) == INTEGER_CST is a bit of nit, but we might as well clean that up. I couldn't convince myself that losing the else clause handling was correct or not. > > > > @@ -1653,8 +1661,11 @@ get_range_strlen (tree arg, bitmap *visited, > > /* Try to obtain the range of the lengths of the string(s) referenced > by ARG, or the size of the largest array ARG refers to if the range > - of lengths cannot be determined, and store all in *PDATA. ELTSIZE > - is the expected size of the string element in bytes: 1 for char and > + of lengths cannot be determined, and store all in *PDATA which must > + be zero-initialized on input except PDATA->MAXBOUND may be set to > + a non-null tree node other than INTEGER_CST to request to have it > + set to the length of the longest string in a PHI. ELTSIZE is > + the expected size of the string element in bytes: 1 for char and Is there any reason we can't just make a clean distinction between input and output objects in this routine? As an API this seems awkward at best. Any thoughts on the API question raised? The rest are just nits/typos: > @@ -2862,51 +2865,78 @@ handle_builtin_memset (gimple_stmt_iterator *gsi) > return true; > } > > -/* Handle a call to memcmp. We try to handle small comparisons by > - converting them to load and compare, and replacing the call to memcmp > - with a __builtin_memcmp_eq call where possible. > - return true when call is transformed, return false otherwise. */ > +/* Return a pointer to the first such equality expression if RES is used > + only in experessions testing its equality to zero, and null otherwise. */ s/experessions/expressions/ > > -static bool > -handle_builtin_memcmp (gimple_stmt_iterator *gsi) > +static gimple* > +used_only_for_zero_equality (tree res) Nit. A space between "gimple" and "*". > + > +/* If IDX1 and IDX2 refer to strings A and B of unequal lengths, return > + the result of 0 == strncmp (A, B, BOUND) (which is the same as strcmp > + for s sufficiently large BOUND). If the result is based on the length > + of one string being greater than the longest string that would fit in > + the array pointer to by the argument, set *PLEN and *PSIZE to > + the corresponding length (or its complement when the string is known > + to be at least as long and need not be nul-terminated) and size. > + Otherwise return null. */ s/null/NULL/ > +/* Diagnose pointless calls to strcmp whose result is used in equality > + epxpressions that evaluate to a constant due to one argument being > + longer than the size of the other. */ s/epxressions/expressions/ > +/* Optimize a call to strcmp or strncmp either by folding it to a constant > + when possible or by transforming the latter to the former. Warn about > + calls where the length of one argument is greater than the size of > + the array to which the other aargument points if the latter's length > + is not known. Return true when the call has been transformed into > + another and false otherwise. */ s/aargument/argument/ > > - unsigned HOST_WIDE_INT var_sizei = 0; > - /* try to determine the minimum size of the object pointed by var_string. > */ > - tree size = determine_min_objsize (var_string); > + /* Determine either the length or the size of each of the string > + orguments, whichever is available. */ s/orguments/arguments/ >