RFR: 8232084: HotSpot build failed with GCC 9.2.1

[Chris Plummer]

On 10/10/19 12:03 AM, David Holmes wrote:
> 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.
Yes. Would be nice if gcc picked up on this and not give the warning.

Chris
>
>> 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)
>>
>>