RFR 8209138: Symbol constructor uses u1 as the element type of its name argument

[David Holmes]

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> wrote:
>>
>> Hi Harold,
>>
>> On 21/08/2018 12:43 AM, Harold David Seigel wrote:
>>> Hi,
>>> 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 'Symbol::char_at_put()'.
>>
>> 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.

David
-----

> 
>> 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 get_symbol()->char-at()
>>
>> ** 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.
>>
>> Cheers,
>> David
>>
>>
>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8209138/webrev/index.html
>>> 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 Linux-x64.
>>> Thanks, Harold
> 
>