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

[Yasumasa Suenaga]

Hi Leo,

Thanks for clarifying.
I continue to wait Reviewer for webrev.00 .

>>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8235819/webrev.00/


Thanks,

Yasumasa


On 2019/12/13 22:33, Leo Korinth wrote:
> On 13/12/2019 03:03, Yasumasa Suenaga wrote:
>> Hi Leo,
>>
>> Thank you for your review.
>>
>> On 2019/12/12 23:26, Leo Korinth wrote:
>>> Hi,
>>>
>>> Your fix looks good to me (I am not a reviewer). Thanks for fixing this!
>>>
>>> In the long run, I believe a better solution would be two have a separate function for doing the indentation so that one could use format strings to make the indentation level work for any level or, at least, assert on over-run. This is a good fix for now however.
>>
>> Did you suggest like this?
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8235819/indent-function/
> no, I thought something more like this (not tested):
> 
> const char* indent(unsigned n) {
>    static const char* indents[] = {"", "  ", "    ", "      ", " ", "          "};
>    assert(n < ARRAY_SIZE(indents), "overflow");
>    return indents[n];
> }
> 
> or maybe:
> 
> const char* indent(unsigned n) {
>    static const char spaces[] = "                               ";
>    const unsigned SPACE_COUNT = 2;
>    const unsigned len = ARRAY_SIZE(spaces) - 1;
> 
>    assert(n*SPACE_COUNT <= len, "underflow");
>    return &spaces[len - n*SPACE_COUNT];
> }
> 
> Both of these will just change the code on the caller side from Indent[expression] to indent(expression), with the benefit of a runtime assert that will fail when the compiler can not infer the overrun at compile time.
> 
> Another solution would be to use format strings, and something like this:
> 
> expression("%*s%s", 2*indent, "", "indented string");
> 
> This requires no assert, but is harder to understand and requires more changes.
> 
> But as I said, I think your original fix is good.
> 
> Thanks,
> Leo
> 
>>
>> I agree with you basically, but this code is for logging. So I think it prefers simple.
>> In addition, we can catch the overrun via compiler warnings.
>>
>> Indents[] is also used with literal in G1GCPhaseTimes::info_time() and more.
>> If we change Indents[] in future, they might be broken.
>> Of course we can add assert() to each codes, but it is strange, IMHO.
>>
>> Thus I think we can take two approaches:
>>
>>    1) Extends Indents[] if we need (webrev.00)
>>    2) Add a function to generate indent string (indent-function)
>>
>> I prefer 1) at this time.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>> Thanks,
>>> Leo
>>>
>>>
>>> On 12/12/2019 13:51, Yasumasa Suenaga wrote:
>>>> Hi all,
>>>>
>>>> Please review this trivial change:
>>>>
>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8235819
>>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8235819/webrev.00/
>>>>
>>>> I saw format-overflow warnings in g1GCPhaseTimes.cpp when I built jdk/jdk with GCC 9 on Fedora 31 as below:
>>>>
>>>> ------------------
>>>> /home/ysuenaga/OpenJDK/jdk/src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp:319:17: error: '%s' directive argument is null [-Werror=format-overflow=]
>>>>    319 | out->print("%s", Indents[indent + 1]);
>>>>        | ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> ------------------
>>>>
>>>> Indents[] seems to be insufficient.
>>>> We need to give enough entries to it.
>>>>
>>>> This patch passed all tests on submit repo (mach5-one-ysuenaga-JDK-8235819-20191212-0925-7464553).
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa