On 9/9/18 3:57 AM, Bernd Edlinger wrote:
> On 09/09/18 01:47, Jeff Law wrote:
>> 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.
>>
>
> Yes, I agree. The API is trying to solve everything at once, that is not
> good.
>
> I have given Martin's part 2/6 patch a little massage tonight,
> and reached this nice result:
>
> XPASS: gcc.dg/warn-strlen-no-nul.c pr????? (test for warnings, line 91)
> XPASS: gcc.dg/warn-strlen-no-nul.c pr????? (test for warnings, line 95)
> XPASS: gcc.dg/warn-strlen-no-nul.c pr????? (test for warnings, line 98)
> XPASS: gcc.dg/warn-strlen-no-nul.c pr????? (test for warnings, line 100)
> XPASS: gcc.dg/warn-strlen-no-nul.c pr????? (test for warnings, line 107)
> XPASS: gcc.dg/warn-strlen-no-nul.c pr????? (test for warnings, line 109)
> XPASS: gcc.dg/warn-strlen-no-nul.c pr????? (test for warnings, line 110)
> XPASS: gcc.dg/warn-strlen-no-nul.c pr????? (test for warnings, line 118)
> XPASS: gcc.dg/warn-strlen-no-nul.c bug (test for warnings, line 122)
> XPASS: gcc.dg/warn-strlen-no-nul.c bug (test for warnings, line 123)
>
> I left the test case as-is, just removed the ugly
> "Prune out warnings with no location (pr?????)."
> at the end of the test.
>
> How do you like that?
It's pretty good. I updated the testcase constructed a ChangeLog from
this, the bits of the 1/6 patch and your changes. Bootstrapped and
regression tested on x86_64.
It subsumes the remainder of the 1/6 patch from Martin as well as the
2/6 patch in its entirety.
I'll be committing it to the trunk momentarily. I'm going to stop here
for the night and let the various automatic testers to their thing.
Jeff
