Re: [PATCH] Handle overlength strings in the C FE

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 08/01/18 19:07, Martin Sebor wrote:
> On 08/01/2018 05:20 AM, Bernd Edlinger wrote:
>> On 07/30/18 17:49, Joseph Myers wrote:
>>> On Mon, 30 Jul 2018, Bernd Edlinger wrote:
>>>
>>>> Hi,
>>>>
>>>> this is how I would like to handle the over length strings issue in the C FE.
>>>> If the string constant is exactly the right length and ends in one explicit
>>>> NUL character, shorten it by one character.
>>>
>>> I don't think shortening should be limited to that case.  I think the case
>>> where the constant is longer than that (and so gets an unconditional
>>> pedwarn) should also have it shortened - any constant that doesn't fit in
>>> the object being initialized should be shortened to fit, whether diagnosed
>>> or not, we should define GENERIC / GIMPLE to disallow too-large string
>>> constants in initializers, and should add an assertion somewhere in the
>>> middle-end that no too-large string constants reach it.
>>>
>>
>> Okay, there is an update following your suggestion.
>>
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> 
> The ChangeLog description says:
> 
>      * c-typeck.c (digest_init): Fix overlength strings.
> 
> suggesting there is a bug but there is no test case.  If there
> is a bug in there that can be triggered by C code (valid or
> otherwise), it would be good to have a test case and a bug
> in Bugzilla.  If there is no bug and this is just cleanup,
> I would suggest to adjust the description.
> 

Yes, thanks for looking at this.  This is an attempt to
reduce the number of different encodings for otherwise
identical strings in the middle-end.

I could say "Shorten overlength strings." is that better?
There are already numerous test cases with overlength strings.
I verified that, because I have tested this patch on top
of https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00050.html

> Other than that, while making improvements here, I think it
> would be helpful to also add more detail to the text of
> the warning:
> 

Sure, but it is important to do only one thing at a time.
I see this as a preparation to fixing the remaining
string_constant folding issues which are potential wrong-code
issues.

> 1) mention the type of the array being initialized in case
> it's not obvious from the declaration (the array bound could
> be a symbol, not a literal, or the type could be a typedef)
> 
> 2) mention the number of elements in the initializer in case
> it's a macro (such as __FILE__) whose definition isn't visible
> in the diagnostic
> 
> 3) mention that the excess elements are ignored (since it's
> undefined in the standard, it will let users know what
> happens in GCC).
> 
> Here's a test case and a suggested warning:
> 
>    #define S __FILE__ "\000"
>    enum { N = sizeof __FILE__ };
>    const char a[N] = S;
> 
>    warning: discarding 1 excess element from initializer-string for 'char[4]' [-Wc++-compat]
>     #define S __FILE__ "\000"
>               ^~~~~~~~
>    note: in expansion of macro ‘S’
>     const char a[N] = S;
>                       ^
> (Similarly for more than 1 excess element.)
> 

Yes, definitely helpful, but not part of this patch.
Probably one of your next patches, I would assume.

Bernd.