RFR (trivial): 8235819: -Wformat-overflow is reported from GCC 9

[Leo Korinth]

On 16/12/2019 14:46, Yasumasa Suenaga wrote:
> Hi Thomas,
> 
> I uploaded new webrev:
> 
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8235819/webrev.01/
> 
> I guess static analyzer in GCC cannot trace each condition deeply.
> However, G1 might have a bug to access out of Indents[] in future. It is 
> the motivation of this fix
> This webrev introduces indent() to generate indent string.
> indent() has assert() to check indent level, but it would not affect in 
> release build (we can see same error).
> So I extends Indents[] in this webrev.

Just to be sure I understand this correctly:

- the code compiled with debug configuration AND without the extra 
indentation entry. But failed when using release build as the assert 
could not help the compiler when the assert was preprocessed away?

- you did run the tests with assert and /without/ extra indentation 
entry and it worked?

If both are correct I think your webrev is good.

> 
> `indent` is already used in some place. So I renamed it to avoid confusion.

I am okay with your rename.

Thanks,
Leo

> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2019/12/16 19:19, Thomas Schatzl wrote:
>> Hi,
>>
>> On 15.12.19 07:08, Yasumasa Suenaga wrote:
>>> Hi Thomas, Leo,
>>>
>>> How about this change?
>>>
>>>    http://hg.openjdk.java.net/jdk/submit/rev/10816e67561a
>>>
>>> It generates indent string via alloca().
>>> So get_indent() should be inlined.
>>
>> (typical Hotspot style is without the "get_" prefix).
>>
>>>
>>> However it was failed at TestGCLogMessages on macOS only on submit 
>>> repo (mach5-one-ysuenaga-JDK-8235819-1-20191215-0359-7545712).
>>>
>>> If you are generally ok to this change, could you share details of 
>>> this failure?
>>> I will fix it.
>>
>> The alloca-allocated buffer will be freed at the exit of the 
>> get_indent() method, so the new code effectively accesses freed memory 
>> as get_indent() returns a pointer to it.
>>
>> The intent of our comments has basically been:
>>
>> - remove the "5" (or "4") in the Indents static.
>> - add a static indent() method that does something like:
>>
>> static const char* indent(uint level) {
>>    assert(level < ARRAY_SIZE(Indents), "Too high indent level %u", 
>> level);
>>    return Indents[level];
>> }
>>
>> And replace the direct access to the Indents array in the code with a 
>> call to  the indent() method.
>>
>> If gcc still complains, add that additional indent level you added in 
>> webrev.00. I do not like easily avoidable pragmas in the code, and the 
>> additional level does not hurt, but others might disagree.
>>
>> Probably also file a bug with gcc.
>>
>> Thanks,
>>    Thomas