RFR: 8232084: HotSpot build failed with GCC 9.2.1

[David Holmes]

On 10/10/2019 4:50 pm, Chris Plummer wrote:
> On 10/9/19 11:36 PM, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 10/10/2019 3:55 pm, Yasumasa Suenaga wrote:
>>> Hi all,
>>>
>>> Please review this change:
>>>
>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8232084
>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.00/
>>>
>>>
>>> I tried to build OpenJDK on Fedora 30 x64 with GCC 9.2.1, but it was 
>>> failed in macroAssembler_x86.hpp and diagnosticArgument.cpp .
>>
>> The macroAssembler fix is fine - we need to ensure there is a NULL 
>> check for variables that are passed to %s.
>>
>> The strncpy fix in diagnosticArgument.cpp I'm less clear about. I 
>> don't understand what the warning is warning about. It seems to be 
>> concerned about possible truncation, but IIUC truncation is fine, in 
>> fact that's exactly why we use strncpy to ensure we don't overflow the 
>> buffer if the value we are copying is too long! Is this a case of gcc 
>> trying to be too helpful ?? I found this article:
>>
>> https://developers.redhat.com/blog/2018/05/24/detecting-string-truncation-with-gcc-8/ 
>>
>>
>> which suggests that pattern we have of explicitly setting the NUL 
>> after the strncpy should be fine. I can't see anything wrong with the 
>> existing code. That said the modified code is also not incorrect.
>  From JBS:
> 
>   /home/ysuenaga/OpenJDK/jdk/src/hotspot/share/services/diagnosticArgument.cpp:154:14: warning: 'char* strncpy(char*, const char*, size_t)' output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
>    154 | strncpy(buf, str, len);
>        | ~~~~~~~^~~~~~~~~~~~~~~
> 
> I assume this means that in all cases the "len" value is seen to be 
> derived from strlen, and therefore strncpy is always copying one byte 
> short of \0, and this is most likely not what the user wants. I seem to 

Yes but we then explicitly set the NULL at buf[len] which is the 
expected/required pattern for this.

> recall another recent similar fix that was done by switching to using 
> memcpy instead.
> 
> Here's a discussion of interest, also suggesting memcpy:
> 
> https://stackoverflow.com/questions/50198319/gcc-8-wstringop-truncation-what-is-the-good-practice 

Seems to me that strncpy and memcpy are semantically equivalent here so 
all this does is avoid gcc's over zealous warnings. I'm inclined to use the:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-truncation"

solution.

YMMV.

Cheers,
David

> 
> Chris
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> (Please see JBS for details)
>>>
>>> This change has passed tests on submit repo.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa (ysuenaga)
> 
>