RFR: 8232084: HotSpot build failed with GCC 9.2.1

[Chris Plummer]

On 10/10/19 1:07 AM, Yasumasa Suenaga wrote:
> On 2019/10/10 16:42, 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'
>
> I missed it, thanks!
> I think we can fix it as below.
>
> -------------
> diff -r b4f2e13d20ea src/hotspot/share/services/diagnosticArgument.cpp
> --- a/src/hotspot/share/services/diagnosticArgument.cpp Wed Oct 09 
> 19:38:11 2019 -0700
> +++ b/src/hotspot/share/services/diagnosticArgument.cpp Thu Oct 10 
> 17:04:09 2019 +0900
> @@ -151,7 +151,7 @@
> ResourceMark rm;
>
>        char* buf = NEW_RESOURCE_ARRAY(char, len + 1);
> -      strncpy(buf, str, len);
> memcpy(buf, str, len);
> buf[len] = '\0';
>        Exceptions::fthrow(THREAD_AND_LOCATION, 
> vmSymbols::java_lang_IllegalArgumentException(),
>          "Boolean parsing error in command argument '%s'. Could not 
> parse: %s.\n", _name, buf);
> -------------
>
> I will upload webrev if it is ok.
That looks right, although it's odd that the memcpy line does not have a 
+ in front of it to show that it's an addition.

Chris
>
>
> Thanks,
>
> Yasumasa
>
>
>> 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)
>>>>>
>>>>
>>>>
>>>