Avoid some GCC 10.X warnings in HotSpot
Koichi Sakata
sakatakui at oss.nttdata.com
Tue Jul 7 06:46:58 UTC 2020
Hi all,
>>> > https://bugs.openjdk.java.net/browse/JDK-8247818
Could we start addressing the issue (about GCC 10 build warnings)?
There is an unclear point as follows.In my opinion, that doesn't cause
any problems if it is historical or an alignment issue. Mainly the
reason is that the byte_at_put function is a private function.
>>>>> + } else {
>>>>> + memcpy(_body, name, length);
>>>>> }
>>>>> }
>>>>
>>>> So you are replacing byte_at_put with a memcpy call. On the surface
>>>> that seems reasonable, but I have to wonder why we were using the
>>>> loop in the first place. It may just be historical or it may relate
>>>> to an alignment issue, or something else. Hopefully someone else
>>>> (e.g. Coleen :) ) can shed more light here.
Thanks,
Koichi
On 2020/06/19 15:11, Koichi Sakata wrote:
> > Probably best to re-send as the mention of "Hotspot" in subject might
> > put off core-libs folk from looking at it. :)
>
> I will do so.Thank you.
>
> Thanks,
> Koichi
>
> On 2020/06/19 14:56, David Holmes wrote:
>> On 19/06/2020 11:59 am, Koichi Sakata wrote:
>>> Hi David,
>>>
>>> > This is in relation to the hotspot part as these issues need to be
>>> > handled separately. I have filed:
>>> >
>>> > https://bugs.openjdk.java.net/browse/JDK-8247818
>>>
>>> Thank you, David.I have something to ask you.
>>> Should I send only the other part of the patch (i.e.
>>> NetworkInterface.c and k_standard.c) to core-lib ML again? I've sent
>>> the whole one to core-lib before.
>>
>> Probably best to re-send as the mention of "Hotspot" in subject might
>> put off core-libs folk from looking at it. :)
>>
>> Cheers,
>> David
>>
>>> > I'm not really clear on the warning here but this is an area where we
>>> > trick the compiler somewhat. The _body[] is declared with a size
>>> of 2,
>>> > but when we allocate Symbols we allocate sufficient memory for
>>> _body to
>>> > contain the entire symbol.
>>> >
>>> > That said I'm struggling to see how we allocate the additional space
>>> > needed for the _hash_and_refcount and _length fields ???
>>>
>>> I was thinking exactly the same thing. I've learned a lot from Ioi's
>>> explanation.
>>>
>>> > The check for length==0 introduces more overhead than just always
>>> > setting _body[0]=0, so there is no need to add it.
>>>
>>> I understood that clearly. Thank you for teaching me.
>>>
>>> Thanks,
>>> Koichi
>>>
>>> On 2020/06/18 10:56, David Holmes wrote:
>>>> Hi Koichi,
>>>>
>>>> This is in relation to the hotspot part as these issues need to be
>>>> handled separately. I have filed:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8247818
>>>>
>>>> On 18/06/2020 8:46 am, Koichi Sakata wrote:
>>>>> Hi all,
>>>>>
>>>>> I tried to build OpenJDK fastdebug with GCC 10.1 on Ubuntu 18.04,
>>>>> but I saw some compiler warnings as follows:
>>>>>
>>>>> In file included from
>>>>> /home/jyukutyo/code/jdk/src/hotspot/share/classfile/systemDictionary.hpp:31,
>>>>>
>>>>> from
>>>>> /home/jyukutyo/code/jdk/src/hotspot/share/classfile/javaClasses.hpp:28,
>>>>>
>>>>> from
>>>>> /home/jyukutyo/code/jdk/src/hotspot/share/precompiled/precompiled.hpp:35:
>>>>>
>>>>> In member function 'void Symbol::byte_at_put(int, u1)',
>>>>> inlined from 'Symbol::Symbol(const u1*, int, int)' at
>>>>> /home/jyukutyo/code/jdk/src/hotspot/share/oops/symbol.cpp:55:16:
>>>>> /home/jyukutyo/code/jdk/src/hotspot/share/oops/symbol.hpp:130:18:
>>>>> error: writing 1 byte into a region of size 0
>>>>> [-Werror=stringop-overflow=]
>>>>> 130 | _body[index] = value;
>>>>> | ~~~~~~~~~~~~~^~~~~~~
>>>>
>>>> I'm not really clear on the warning here but this is an area where
>>>> we trick the compiler somewhat. The _body[] is declared with a size
>>>> of 2, but when we allocate Symbols we allocate sufficient memory for
>>>> _body to contain the entire symbol.
>>>>
>>>> That said I'm struggling to see how we allocate the additional space
>>>> needed for the _hash_and_refcount and _length fields ???
>>>>
>>>>> I can resolve them with the following patch. I believe it fixes
>>>>> those potential bugs, so I'd like to contribute it.
>>>>> (Our company has signed OCA.)
>>>>>
>>>>> Thanks,
>>>>> Koichi
>>>>>
>>>>> ===== PATCH =====
>>>>> diff -r 20d92fe3ac52 src/hotspot/share/oops/symbol.cpp
>>>>> --- a/src/hotspot/share/oops/symbol.cpp Tue Jun 16 03:16:41 2020
>>>>> +0000
>>>>> +++ b/src/hotspot/share/oops/symbol.cpp Thu Jun 18 07:08:50 2020
>>>>> +0900
>>>>> @@ -50,9 +50,10 @@
>>>>> Symbol::Symbol(const u1* name, int length, int refcount) {
>>>>> _hash_and_refcount =
>>>>> pack_hash_and_refcount((short)os::random(), refcount);
>>>>> _length = length;
>>>>> - _body[0] = 0; // in case length == 0
>>>>> - for (int i = 0; i < length; i++) {
>>>>> - byte_at_put(i, name[i]);
>>>>> + if (length == 0) {
>>>>> + _body[0] = 0;
>>>>
>>>> The check for length==0 introduces more overhead than just always
>>>> setting _body[0]=0, so there is no need to add it.
>>>>
>>>>> + } else {
>>>>> + memcpy(_body, name, length);
>>>>> }
>>>>> }
>>>>
>>>> So you are replacing byte_at_put with a memcpy call. On the surface
>>>> that seems reasonable, but I have to wonder why we were using the
>>>> loop in the first place. It may just be historical or it may relate
>>>> to an alignment issue, or something else. Hopefully someone else
>>>> (e.g. Coleen :) ) can shed more light here.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> diff -r 20d92fe3ac52 src/hotspot/share/oops/symbol.hpp
>>>>> --- a/src/hotspot/share/oops/symbol.hpp Tue Jun 16 03:16:41 2020
>>>>> +0000
>>>>> +++ b/src/hotspot/share/oops/symbol.hpp Thu Jun 18 07:08:50 2020
>>>>> +0900
>>>>> @@ -125,11 +125,6 @@
>>>>> return (int)heap_word_size(byte_size(length));
>>>>> }
>>>>>
>>>>> - void byte_at_put(int index, u1 value) {
>>>>> - assert(index >=0 && index < length(), "symbol index overflow");
>>>>> - _body[index] = value;
>>>>> - }
>>>>> -
>>>>> Symbol(const u1* name, int length, int refcount);
>>>>> void* operator new(size_t size, int len) throw();
>>>>> void* operator new(size_t size, int len, Arena* arena) throw();
>>>>
>>>>
More information about the hotspot-runtime-dev
mailing list