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/
>
