On 9/8/18 3:47 PM, Bernd Edlinger wrote:
> Hi,
>
>
>> -fold_builtin_strlen (location_t loc, tree type, tree arg)
>> +fold_builtin_strlen (location_t loc, tree fndecl, tree type, tree arg)
>> {
>> if (!validate_arg (arg, POINTER_TYPE))
>> return NULL_TREE;
>> else
>> {
>> - tree len = c_strlen (arg, 0);
>> -
>> + tree nonstr = NULL_TREE;
>> + tree len = c_strlen (arg, 0, &nonstr);
>> if (len)
>> - return fold_convert_loc (loc, type, len);
>> + {
>> + if (loc == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (arg))
>> + loc = EXPR_LOCATION (arg);
>> +
>> + /* To avoid warning multiple times about unterminated
>> + arrays only warn if its length has been determined
>> + and is being folded to a constant. */
>> + if (nonstr)
>> + warn_string_no_nul (loc, NULL_TREE, fndecl, nonstr);
>> +
>> + return fold_convert_loc (loc, type, len);
>> + }
>>
>> return NULL_TREE;
>
> If I see that right, this will do a wrong folding,
> just to suppress duplicate warnings.
>
> But it will re-introduce a path to PR87053, since c_strlen
> is supposed to return the wrong value because nonstr
> suggests the caller is able to handle this.
>
> I think c_strlen should never return anything that is invalid.
> Returning len and nonstr should be mutually exclusive events.
Conceptually I agree and have already been looking at this. But in
practice I'm still on the fence.
I'm fairly unhappy with the APIs we're using here and the inconsistency
in what nonstr means in terms of the return value from various
functions. We see this when we start layering the warnings on top of
the trunk (which has the 87053 fix). In some cases we want nonstr to
force the function to return an "I don't know value, typically
NULL_TREE", but in other cases we really want the function to still
return the length and let the caller decide what to do about the
termination problem.
It doesn't help that we also have memsize as well.
Rationalizing the APIs here is likely going to be a prereq to move forward.
jeff
