Re: [PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Jeff Law <law at redhat dot com>]

On 08/23/2018 04:21 AM, Bernd Edlinger wrote:
> On 08/22/18 10:14, Richard Biener wrote:
>> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
>>
>>> On 08/22/18 09:26, Richard Biener wrote:
>>>> On Wed, 22 Aug 2018, Bernd Edlinger wrote:
>>>>
>>>>> On 08/21/18 10:59, Richard Biener wrote:
>>>>>> On Tue, 21 Aug 2018, Bernd Edlinger wrote:
>>>>>>
>>>>>>> gcc -S -O2 -Wall -Wformat-overflow -ftrack-macro-expansion=0 -fshort-wchar builtin-sprintf-warn-20.c
>>>>>>> builtin-sprintf-warn-20.c: In function 'test':
>>>>>>> builtin-sprintf-warn-20.c:19:39: warning: hex escape sequence out of range
>>>>>>> 19 |     ? (char*)L"\x4142\x4344" : (char*)L"\x41424344\x45464748";
>>>>>>>        |                                       ^~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>
>>>>>>> Hmm, this test might create some noise on short-wchar targets.
>>>>>>>
>>>>>>> I would prefer a warning here, about the wrong type of the parameter.
>>>>>>> The buffer overflow is only a secondary thing.
>>>>>>>
>>>>>>> For constant objects like those, the GIMPLE type is still guaranteed to be reliable,
>>>>>>> right?
>>>>>>
>>>>>> TREE_TYPE of tcc_declaration and tcc_constant trees should more-or-less
>>>>>> (minus qualifications not affecting semantics) be those set by
>>>>>> frontends.
>>>>>>
>>>>>
>>>>> and in this case:
>>>>>
>>>>> const union
>>>>> { struct {
>>>>>        wchar_t x[4];
>>>>>      };
>>>>>      struct {
>>>>>        char z[8];
>>>>>      };
>>>>> } u = {{L"123"}};
>>>>>
>>>>> int test()
>>>>> {
>>>>>      return __builtin_strlen(u.z);
>>>>> }
>>>>>
>>>>>
>>>>> string_constant works out the initializer for u.x
>>>>> which has a different type than u.z
>>>>
>>>> Yes.  That's because it uses ctor-for-folding and friends.  It's
>>>> a question of the desired semantics of string_constant whether
>>>> it should better return NULL_TREE in this case or whether the
>>>> caller has to deal with type mismatches.
>>>>
>>>
>>> Yes, absolutely.
>>>
>>> c_getstr needs to bail out if the string is not zero-terminated
>>> within the limits given by the decl, or the string_cst-type or whatever
>>> may help.
>>>
>>> Furthermore I also consider it possible that the byteoffset
>>> is not a multiple of eltsize.  So fail in that case as well.
>>>
>>>
>>> I am currently boot-strapping a patch for this (pr87053):
>>> $ cat u.c
>>> const union
>>> { struct {
>>>       char x[4];
>>>       char y[4];
>>>     };
>>>     struct {
>>>       char z[8];
>>>     };
>>> } u = {{"1234", "567"}};
>>>
>>> int test()
>>> {
>>>     return __builtin_strlen(u.z);
>>> }
>>>
>>> gets folded to 4.
>>>
>>> ... but unfortunately it will depend on my pr86714 fix which fixes
>>> the mem_size parameter returned from string_constant.
>>>
>>> Frankly, in the moment I feel like I fell in a deep deep hole.   :-O
>>
>> /me too with > 100 mails in this and related threads unread ;)
>>
>> I thought Jeff applied the mem_size parameter thing but maybe it
>> was something else.  *fetches coffee*  Ah, Jeff is still "working"
>> on it.
>>
> 
> The dependence is not because of the missing mem_size parameter,
> but because there is another place where strlen(c_getstr) is used,
> and my other patch fixes c_getstr to use mem_size and
> not return non-zero terminated strings.
> 
> But since Martin claims his patch is superior to mine we
> are stuck in this situation.
> 
> I think all uses of string_constant all over where mem_size
> is not used, are actually broken.  I should make mem_size
> a mandatory parameter.
I think Martin's patch to handle the problems with unterminated strings
is more complete and it provides infrastructure to allow for sensible
warnings WRT unterminated strings.

Where I'm having trouble with Martin's patch is convincing myself it's
right for wchar types.   I also want to side test it against your new
tests.  This is going to require me to twiddle Martin's patch for the
current trunk and likely twiddle at least a couple places that I think
need adjustment for wchars.

Your patch is simpler and easier to generally understand.  My concern is
that even if we installed your patch now we'd end up wanting something
similar to Martin's very soon.  I also want to side test yours against
Martin's new tests.  Side testing your patch is simpler.

Losing half my day Tuesday and half of Thursday to meetings isn't
helping.  Nor did the repeated power and internet drops yesterday.  I'm
not getting nearly as much work done as normal.

Jeff