On 08/25/2018 01:32 PM, Bernd Edlinger wrote:
> On 08/25/18 21:02, Jeff Law wrote:
>> 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.
>>
>
> I think we should slow down.
>
>>
>>>
>>> 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.
>>
>
> We should have a test case with flexible array members, those behave
> slightly different than VLAs.
>
> struct {
> int i;
> char x[];
> } s;
>
> const struct s s = { 1, "test" };
>
> int f()
> {
> return strlen(s.x);
> }
>
>
> By the way, when you change so much on Martin's patch it might be
> good to post it again on this list, when it's ready, so maybe I can have a
> look at it and help out with some review comments? (please put me on CC)
Actually very little changed other than mechanical stuff. It's one
chunk of code that assumed STRING_CSTs are always NUL terminated that's
problem with that code in isolation.
But I also have to evaluate your patches in the same area and also look
at the known follow-ups to see how the various patches are likely to
interact.
And each time core assumptions/issues are reexamined parts of the
evaluation have to be redone. In some cases they may simplify, in
others they make the analysis more complex.
Jeff
>
>
> Bernd.
>
