RFR 8209138: Symbol constructor uses u1 as the element type of its name argument
Harold David Seigel
harold.seigel at oracle.com
Thu Sep 27 13:17:43 UTC 2018
Please review this updated webrev:
It contains additional changes to use char for class Symbol.
Hopefully, your concerns w.r.t. signed and unsigned types were resolved
by our offline discussions.
On 8/21/2018 2:24 AM, David Holmes wrote:
> On 21/08/2018 3:19 PM, Kim Barrett wrote:
>>> On Aug 20, 2018, at 8:53 PM, David Holmes <david.holmes at oracle.com>
>>> Hi Harold,
>>> On 21/08/2018 12:43 AM, Harold David Seigel wrote:
>>>> Please review this change for bug JDK-8209138. The fix changes
>>>> class Symbol in symbol.hpp to use type char instead of types u1 and
>>>> jbyte and renames relevant functions by replacing 'byte' with
>>>> 'char'. For example, 'Symbol::byte_at_put()' is now
>>> It's not clear to me exactly what all these things should be. In a
>>> lot of places we seem to be dealing with UTF8 character
>>> representations, which to me should be u1 (afterall that is how they
>>> are specified in the classfile format!) which is just an unsigned
>>> "byte". But char is signed (in our build).
>> Not sure what’s meant by “in our build” but char is unsigned when
>> building HotSpot for aarch64 / arm. See, for example, JDK-8209586.
> Ah! We have always set -fsigned-char for the Oracle ARM and ARM64
> builds (and previously PPC32), but now I see that, as you say, Aarch64
> does not set this (nor PPC64). Interesting and unfortunate. I'm
> somewhat surprised that we haven't encountered more issues due to this.
> In any case we are still converting from an unsigned type to a signed
> type in many cases with this change.
>>> The current change just seems to push out the boundary where we
>>> convert between things. For example ciSymbol now has a char base()**
>>> but still has a byte_at() method - should that not know be char_at()
>>> for consistency? Especially given byte_at() returns
>>> ** Granted the existing jbyte seems the wrong choice too.
>>> I think we have a lot of type confusion in our code, but it isn't
>>> clear to me that this particular change is necessarily changing
>>> things in the right direction.
>>>> Open Webrev:
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8209138
>>>> The change was tested by running Mach5 tiers 1 and 2 tests and
>>>> builds on Linux-x64, Windows, and Mac OS X, running tiers 3-5 tests
>>>> on Linux-x64, and by running JCK-11 API, Lang and VM tests on
>>>> Thanks, Harold
More information about the hotspot-runtime-dev