RFR: 8232084: HotSpot build failed with GCC 9.2.1

[Chris Plummer]

On 10/10/19 12:42 AM, David Holmes wrote:
> On 10/10/2019 5:25 pm, serguei.spitsyn at oracle.com wrote:
>> On 10/9/19 23:39, Chris Plummer wrote:
>>> I think the +1 gets rid of the warning, but it also means there is 
>>> no need for the following line that sets buf[len].
>>
>> Yes, it was my guess.
>> Then I agree this statement must be removed: buf[len] = '\0';
>
> I don't think so!
>
> Given this description:
>
> template <> void DCmdArgument<bool>::parse_value(const char* str,
>                                                  size_t len, TRAPS) {
>   // len is the length of the current token starting at str
>
> it is not evident to me that we are even dealing with a NUL-terminated 
> str. Rather str is the start of the next token, with length len, after 
> which may be another token and so forth. So we may be passing in:
>
> str = "token1 token2"
> len = 6
>
> so we still need the explicit buf[len]='\0'
>
All calls to parse_value() are passing in strlen(str) for the len 
argument. That's why gcc knows to produce this warning. That also means 
we could safely use strcpy() instead. We use strncpy() in a bunch of 
other places and don't get this warning.

However, the comment and the fact that len is passed in makes me think 
that at one point it worked as you suggested and the string was not nul 
terminated. As things stand now  you could just pass in str and let 
parse_value() do the strlen computation. But if at one point it was not 
nul terminated, then that would not have worked, thus len is passed in.

Chris
> David
>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> Chris
>>>
>>> On 10/9/19 11:31 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Yasumasa,
>>>>
>>>> The fix in src/hotspot/cpu/x86/macroAssembler_x86.hpp looks Okay.
>>>>
>>>> But I don't understand why you need this fix in 
>>>> src/hotspot/share/services/diagnosticArgument.cpp ?:
>>>>         char* buf = NEW_RESOURCE_ARRAY(char, len + 1);
>>>> - strncpy(buf, str, len);
>>>> + strncpy(buf, str, len + 1);
>>>>         buf[len] = '\0';
>>>> The buf[len] is set to '\0' anyway.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 10/9/19 22:55, 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 .
>>>>> (Please see JBS for details)
>>>>>
>>>>> This change has passed tests on submit repo.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa (ysuenaga)
>>>>
>>>
>>>
>>