Avoid some GCC 10.X warnings in HotSpot

[David Holmes]

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();