PING [PATCH] warn for strlen of arrays with missing nul (PR 86552, 86711, 86714) )

[Jeff Law]

On 08/25/2018 12:36 PM, Bernd Edlinger wrote:

>>>>
>>>
>>> Well, ya call it "layer one patch over the other"
>>> I call it "incremental improvements".
>> It is (of course) a case by case basis.  The way I try to look at these
>> things is to ask whether or not the first patch under consideration
>> would have any value/purpose after the second patch was installed.  If
>> so, then it may make sense to include both.  If not, then we really just
>> want one patch.
>>
> 
> Agreed.  I think the question is which of the possible STRING_CST
> semantics we want to have in the end (the middle-end).
> Everything builds on top of the semantic properties of STRING_CSTs.
This certainly plays a role.  I bumped pretty hard against the
STRING_CST semantics issue with Martin's patch.  I'm hoping that making
those more consistent will ultimately simplify things and avoid the
problems I'm stumbling over.

Of course, that means more delays in getting this sorted out.  I really
thought I had a viable plan a couple days ago, but I'm having to rethink
in light of some of the issues raised.


> 
> My first attempt of fix the STRING_CST semantic was trying to make
> string_constant happy.
> 
> My second attempt is trying to make Richard happy.  And when I look
> at both patches, I think the second one is better, and more simple.
In general I've found that Richie's advice generally results in a
cleaner implementation ;-)
> 
> 
> BTW I need to correct on statement in my last e-mail:
> 
> On 08/24/18 23:55, Bernd Edlinger wrote:>
>> here I quote form martins 1/6 patch:
>>> +  /* Compute the lower bound number of elements (not bytes) in the array
>>> +     that the string is used to initialize.  The actual size of the array
>>> +     will be may be greater if the string is shorter, but the important
>>> +     data point is whether the literal, including the terminating nul,
>>> +     fits in the array. */
>>> +  unsigned HOST_WIDE_INT array_elts
>>> +    = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (init))) / charsize;
>>> +
>>> +  /* Compute the string length in (wide) characters.  */
>>
>> So prior to my STRING_CST patch this will ICE on a flexible array member,
>> because those have TYPE_SIZE_UNIT = NULL, and tree_to_uhwi will ICE.
>>
>> I used:
>>
>>   compare_tree_int (TYPE_SIZE_UNIT (TREE_TYPE (init)),
>>                     TREE_STRING_LENGTH (init))
>>
>> and this will not ICE with NULL, but consider it like infinity,
>> and return 1.
>>
>> So my version did not ICE in that case.
>>
> 
> Oooops,
> 
> actually both versions will likely ICE on TYPE_SIZE_UNIT == NULL.
> I actually tried to test that case, but have done something wrong.
> 
> I hope we can get rid if the incomplete types in the middle-end.
> 
> So maybe just inject "if (!tree_fits_uhwi_p (...)) return NULL_TREE;"
> here?   Or maybe just defer until we have clarity about the semantics.
Not sure.  I've tried largely to not let VLA issues drive anything here.
 I'm not a fan of them for a variety of reasons and thus I tend to look
at all the VLA stuff as exceptional cases.

jeff