RFR: 8339113: AccessFlags can be u2 in metadata [v11]

[Coleen Phillimore]

On Mon, 6 Jan 2025 03:47:05 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix more copyrights.
>
> src/hotspot/share/ci/ciFlags.hpp line 71:
> 
>> 69: 
>> 70:   // Conversion
>> 71:   jint   as_int()                      { return _flags.as_unsigned_short(); }
> 
> It is unclear to me whether the fact we are dealing with u2 should be exposed in this API as well.

I don't think it should be.

> src/hotspot/share/ci/ciKlass.cpp line 225:
> 
>> 223:   assert(is_loaded(), "not loaded");
>> 224:   GUARDED_VM_ENTRY(
>> 225:     return get_Klass()->access_flags().as_unsigned_short();
> 
> Again it is unclear to me whether this API should also now return u2.

I don't think it should.  I think the boundary of where we promote the u2 to int should be at this API.  If those working on the compiler code would like to propagate the size of the storage (u2) around, they can decide to do that.

> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 1003:
> 
>> 1001:     JVMCI_ERROR_NULL("info must not be null and have a length of 4");
>> 1002:   }
>> 1003:   JVMCIENV->put_int_at(info, 0, fd.access_flags().as_unsigned_short());
> 
> Again are we relying on implicit widening from u2 to int?
> 
> It really isn't clear to me whether the only thing we should have changed here is the actual type of the `_flags` field and let everything else continue to represent flags as int, so we don't get these transitions from u2 to int  in these higher level APIs.

Yes, we can widen u2 to int.  See above.  The ci code represents the integral value of access flags as jint.  I am leaving that API in place.  For this, the widening happens when fetching the u2 field.  The conversion is implicit.  If this field were to be stored back to a u2 somewhere, all the code in the compiler should change but the current code doesn't do that.

> src/hotspot/share/jvmci/jvmciEnv.cpp line 1595:
> 
>> 1593:     HotSpotJVMCI::FieldInfo::set_signatureIndex(JVMCIENV, obj_h(), (jint)fieldinfo->signature_index());
>> 1594:     HotSpotJVMCI::FieldInfo::set_offset(JVMCIENV, obj_h(), (jint)fieldinfo->offset());
>> 1595:     HotSpotJVMCI::FieldInfo::set_classfileFlags(JVMCIENV, obj_h(), (jint)fieldinfo->access_flags().as_unsigned_short());
> 
> I'm curious why we need the explicit cast here - is it because we are going from unsigned to signed?

The casts were all there for all these fields, so I left it.  It is unnecessary but matches the style of the preceding lines.

> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/AccessFlags.java line 82:
> 
>> 80:   public int getStandardFlags() {
>> 81:     return (int)flags;
>> 82:   }
> 
> This function seems unused.

Ah thanks.  Another SA function to remove.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1904197071
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1904190829
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1904193482
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1904194643
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1904195078