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

Wed Oct 3 08:02:47 UTC 2018

Thanks Harold the additional changes look fine.

David

On 3/10/2018 6:51 AM, Harold David Seigel wrote:
> Hi David,
> 
> Thanks for looking at this again.
> 
> Please see this latest webrev and also in-line comments below:
> 
>     http://cr.openjdk.java.net/~hseigel/bug_8209138.4/webrev/
> 
> 
> On 10/1/2018 9:45 PM, David Holmes wrote:
>> Hi Harold,
>>
>> Thanks for persevering with this!
> Thanks!
>>
>> On 2/10/2018 6:23 AM, Harold David Seigel wrote:
>>> Hi Coleen,
>>>
>>> Please review this updated webrev:
>>>
>>> http://cr.openjdk.java.net/~hseigel/bug_8209138.3/webrev/index.html
>>>
>>> It uses u1 for Symbol instead of char.  I also added a "char 
>>> char_at(int index)" function to class Symbol.  It's for callers that 
>>> want a char from a Symbol instead of a u1.
>>
>> I was in two minds about this as we really are comparing the i'th byte 
>> with a given char value. But I suppose if I read char_at(i) as 
>> byte_as_char_at(i) then it's just a type-conversion operation. Though 
>> I am curious what byte_at usages remain after this change?
> There were a few uses of byte_at() in heapDumper.cpp, 
> jvmciCompilerToVM.cpp, and jvmtiTagMap.cpp.  I changed these calls to 
> char_at() because the function results were being treated as chars.  
> Since there were no more uses of Symbol::byte_at() and 
> ciSymbol::byte_at(), I removed them.
>>
>> src/hotspot/share/ci/ciSymbol.hpp
>>
>> Can you humour me and please change this comment:
>>
>>  // Return the i-th char, where i < utf8_length
>>
>> to:
>>
>>  // Return the i-th utf byte as a char, where i < utf8_length
> Done
>>
>> ---
>>
>> src/hotspot/share/oops/symbol.hpp
>>
>>    char char_at(int index) const {
>>      assert(index >=0 && index < length(), "symbol index overflow");
>>      return base()[index];
>>    }
>>
>> I'm surprised this doesn't need a (char) cast to keep the compiler 
>> happy. Some compilers may need it I think and it doesn't hurt to add it.
> I added the cast.
>>
>> ---
>>
>> src/hotspot/share/runtime/vmStructs.cpp
>>
>> There's a comment in:
>>
>> ./jdk.hotspot.agent/share/classes/sun/jvm/hotspot/types/Field.java
>>
>> that needs updating:
>>
>>  FIXME: among other things, this interface is not sufficient to
>>  describe fields which are themselves arrays (like Symbol's
>>  jbyte _body[1]).
>>
>> s/jbyte/u1/
> Done
>>
>> Also:
>>
>> ./jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Symbol.java
>>
>> accesses the array as jbytes - that may need adjusting.
> The serviceability agent builds okay and passes all its tests.  So, I'd 
> prefer not to open this potential can of worms.
>>
>> ----
>>
>> Otherwise all seems fine.
> Thanks!
> Harold
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> Thanks! Harold
>>>
>>>
>>> On 9/27/2018 10:03 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> Hi Harold,  I think we've decided to make u1 the type we carry 
>>>> around in Symbol instead of char, since that's more accurate to 
>>>> represent utf8.  Since the goal is to make the types consistent, can 
>>>> you try to change it all to u1 instead. Keeping the name get_byte() 
>>>> and see how bad the casting situation is?
>>>> thank you!
>>>> Coleen
>>>>
>>>>
>>>> On 9/27/18 9:17 AM, Harold David Seigel wrote:
>>>>> 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
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
> 