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

[Harold David Seigel]

Hi David,

Please review this updated webrev:

    http://cr.openjdk.java.net/~hseigel/bug_8209138.2/webrev/index.html

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.

Thanks, Harold


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> 
>>> 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
>>
>>