On Mon, 6 Jan 2025 03:47:05 GMT, David Holmes <dhol...@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
