RFR: 8232084: HotSpot build failed with GCC 9.2.1

[Chris Plummer]

Hi Yasumasa,

A couple of comments on (B). First there is a typo in 
"stringop-truncatino". Second, can the macros be used just before and 
after the strncpy reference rather than around the whole function? It 
would clarify both that the strncpy use has these macros in place and 
that the macros are there for the strncpy. As it stands now, if you see 
the macro references it won't be readily obvious why they are there.

As for which I prefer, I'm leaning towards (A) for it's simplicity. Yes, 
I understands Kim's reason for opposing it, but (B) and (C) are no 
better in that regard. In all cases we are doing something that should 
be completely unnecessary to work around a gcc bug. I could see 
preferring (C) if we are concerned about this issue popping up in other 
places.

BTW, I had mentioned seeing another instance of this recently in a 
review I did:

http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-September/029361.html

I was thinking of pushing back on having to fix a perfectly valid 
strncpy, and like Kim was not that happy about having to switch it to a 
memcpy. I considered suggesting disabling the warning but decided that 
for this one issue bowing to memcpy was probably best, so I never 
brought it up. So following along on those same thoughts, for your fix 
memcpy might be acceptable (it's just "one" case if we ignore already 
fixed cases), but disabling the warning is probably best if we think 
this is going to be an issue that continues to pop up.

thanks,

Chris

On 10/11/19 6:38 AM, Yasumasa Suenaga wrote:
> Hi,
>
> Christos commented to B-1. Thanks!
> clang defines __GNU_C__ , but stringop-truncation is not supported.
>
> So I updated Plan B-1. It works fine on my Fedora30 box.
>
>
>   A. Use memcpy()
>        http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/
>
>   B-1. Use #pragma
> http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma.02/
>
>   C. Set -Wno-stringop-truncation in globally
>        http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.C/
>
>
> Of course I will push the change to submit repo before sending review 
> request.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2019/10/11 15:55, Yasumasa Suenaga wrote:
>> Hi,
>>
>> Thanks for a lot advises!
>>
>> We have following solutions for this issue.
>> I'd like to send RFR again with much consented patch in early next week.
>>
>>
>>    A. Use memcpy()
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/
>>
>>    B-1. Use #pragma
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma/
>>
>>    C. Set -Wno-stringop-truncation in globally
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.C/
>>
>>
>> If we fix with C, I will send RFR to hotspot-dev and build-dev.
>> Plan C also fixes other stringop-truncation problems such as 
>> JDK-8220074.
>> Thus it affects all of JDK code, but it would be useful if 
>> stringop-truncation
>> should be disabled in JDK build process.
>>
>>
>> Comments are welcome.
>>
>> Yasumasa
>>
>>
>> On 2019/10/11 10:34, Yasumasa Suenaga wrote:
>>> Hi,
>>>
>>> I want to get conclusion of this discussion.
>>>
>>> I understand the fix of macroAssembler_x86.hpp is ok, but we have 
>>> not yet had conclusion
>>> how we should fix diagnosticArgument.cpp .
>>>
>>> I think we can fix diagnosticArgument.cpp as following:
>>>
>>>
>>>    A. Use memcpy()
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/
>>>
>>>    B. Add -Wno-stringop-truncation to 
>>> make/hotspot/lib/JvmOverrideFiles.gmk
>>>         This option will be added diagnosticArgument.cpp only.
>>>
>>>    C. Set -Wno-stringop-truncation in globally
>>>         make/hotspot/lib/CompileJvm.gmk
>>>
>>>
>>> I prefer to fix like A because it affects minimally.
>>> Some issues might be found out by stringop-truncation in future.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2019/10/11 5:54, Kim Barrett wrote:
>>>>> On Oct 10, 2019, at 3:03 AM, David Holmes 
>>>>> <david.holmes at oracle.com> wrote:
>>>>>
>>>>> On 10/10/2019 4:50 pm, Chris Plummer wrote:
>>>>>>  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.
>>>>
>>>> We've run into and discussed problems with -Wstringop-truncation
>>>> before.  (See discussions of JDK-8214777 and JDK-8223186.) This is a
>>>> relatively recent warning option (introduced in gcc8, and included in
>>>> -Wall), and seems to have a considerable bug tail:
>>>>
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88781
>>>> A metabug for -Wstringop-truncation, currently with 16 open and 10
>>>> resolved associated bugs.
>>>>
>>>> I'm not a fan of replacing correct and idiomatic uses of strncpy with
>>>> strcpy or memcpy.  I've suggested in the past that we should turn off
>>>> this warning while it is so buggy.
>>>>