Re: [PATCH] Check the STRING_CSTs in varasm.c

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

On 08/17/18 06:46, Jeff Law wrote:
> On 08/05/2018 04:28 AM, Bernd Edlinger wrote:
>> Hi,
>>
>> I would like to do a minor tweak to the patch.
>> While staring at the other patch I realized that I should
>> better pass size and not thissize to the check
>> function, instead of making use of how thissize is
>> computed using MIN above.  This means that this condition
>>
>> +  if ((unsigned)len != size && (unsigned)len != size + elts)
>> +    return false;
>>
>> should better and more readable be:
>>
>> +  if (size < (unsigned)len && size != len - elts)
>> +    return false;
>>
>> Furthermore I wanted to have the check function static,
>> which I missed to do previously.
>>
>> For completeness, these are again the precondition patches
>> for this patch:
>>
>> [PATCH] Handle overlength strings in the C FE
>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00142.html
> I believe Joseph ack'd this already:
> 
> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00655.html
> 
> 

Yes.

>>
>> [PATCH] Make GO string literals properly NUL terminated
>> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01931.html
> Ian didn't object, deferring to the larger scoped review.  My
> understanding is this should be a nop for Go.  Given it takes us closer
> to have a canonical form, I'll go ahead and ACK for the trunk.
> 
> 

Yes, it should be a no-op, eventually the strings will go
into the string merge section, with the follow-up patch.
Everything works nicely, as I always bootstrap/regtest with GO.


>>
>> [PATCH] [Ada] Make middle-end string literals NUL terminated
>> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01929.html
> This is OK.
> 
> 
>>
>> [PATCH] Create internally nul terminated string literals in fortan FE
>> https://gcc.gnu.org/ml/fortran/2018-08/msg00000.html
> This is OK too.  THough it is unclear why we're not fixing
> gfc_build_string_const vs introducing gfc_build_hollerith_string_const.
> Are hollerith strings naturally not terminated in the fortran front-end
> and thus need special handling, while other calls to
> gfc_build_string_const are always with naturally nul terminated strings?
> 
> 

I think all other strings are null terminated, except the hollerith strings.

> 
> 
>>
>>
>> Bootstrapped, as usual.
>> Is it OK for trunk?
> So as a toplevel comment.    While there may be some interaction with
> Martin's code to detect and warn for unterminated strings passed to
> strlen and other cases, I think the overall goal of canonicalizing our
> internal representation of STRING_CST along with a verification step is
> a good thing and it may make some of Martin's work easier.
> 
> At this point are the prereqs of the varasm verification patch all ACK'd?
> 

Yes, there is a JIT code gen bug, that was caught by the assertion in my
patch:

[PATCH] Fix not properly nul-terminated string constants in JIT
https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00370.html


That is a simple bug, which means, that the JIT FE was never able to
assemble strings > 200 character length.

Fixing a bug like that makes me I wonder, if there is any test coverage
for JIT except the gcc test suite itself.


Thanks
Bernd.