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

[David Holmes]

On Fri, 3 Jan 2025 22:00:34 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Please review this change that makes AccessFlags and modifier_flags u2 types and removes the last remnants of Hotspot adding internal access flags.  This change moves AccessFlags and modifier_flags in Klass to alignment gaps saving 16 bytes.  From pahole: so it's a bit better.
>> 
>> before:
>> 
>>         /* size: 216, cachelines: 4, members: 25, static members: 17 */
>>         /* sum members: 194, holes: 3, sum holes: 18 */
>> 
>> 
>> after:
>> 
>>         /* size: 200, cachelines: 4, members: 25, static members: 17 */
>>         /* sum members: 188, holes: 4, sum holes: 12 */
>> 
>> 
>> We may eventually move the modifiers to java.lang.Class but that's WIP.
>> 
>> Tested with tier1-7 on oracle platforms.  Did test builds on other platforms (please try these changes ppc/arm32 and s390).  Also requires minor Graal changes.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix more copyrights.

src/hotspot/share/ci/ciFlags.cpp line 95:

> 93: // ciFlags::print
> 94: void ciFlags::print(outputStream* st) {
> 95:   st->print(" flags=%x", _flags.as_unsigned_short());

Here, and elsewhere, are we relying on an implicit widening of the u2 result to int so that the format specifier is correct?

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.

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.

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.

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?

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1903619988
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1903621196
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1903621733
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1903624061
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1903624365
PR Review Comment: https://git.openjdk.org/jdk/pull/22246#discussion_r1903619429