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

[Leo Korinth]

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