Avoid some GCC 10.X warnings in HotSpot
Koichi Sakata
sakatakui at oss.nttdata.com
Fri Jun 19 06:11:22 UTC 2020
> 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