On Thu, 13 Feb 2025 03:26:19 GMT, Vladimir Kozlov <k...@openjdk.org> wrote:
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java line
>> 118:
>>
>>> 116: }
>>> 117:
>>> 118: public static Class<?> getClassFor(Address addr) {
>>
>> Did you consider using a lookup table here that is indexed using the kind
>> value?
>
> Example please.
static Class wrapperClasses = new Class[Number_Of_Kinds];
wrapperClasses[NMethodKind] = NMethodBlob.class;
wrapperClasses[BufferKind] = BufferBopb.class;
...;
wrapperClasses[SafepointKind] = SafepointBlob.class;
CodeBlob cb = new CodeBlob(addr);
return wrapperClasses[cb.getKind()];
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java line
>> 146:
>>
>>> 144: }
>>> 145: }
>>> 146: return null;
>>
>> Should this be an assert?
>
> I don't think we need it - the caller `CodeCache.createCodeBlobWrapper()`
> will throw `RuntimeException` when `null` is returned.
I guess my real question is whether or not it can be considered normal behavior
to return null. It seems it should never happen, which is why I was suggesting
an assert.
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java line
>> 213:
>>
>>> 211:
>>> 212: public boolean isUncommonTrapBlob() {
>>> 213: if (!VM.getVM().isServerCompiler()) return false;
>>
>> Why is the check needed? Why not just return the value `getKind() ==
>> UncommonTrapKind` result below?
>
> `UncommonTrapKind` and `ExceptionKind` are not initialized for Client VM
> because corresponding `CodeBlobKind` values are not defined. See
> `CodeBlob.initialize()`.
> Their not initialized value will be 0 which matches `CodeBlobKind::None`
> value. Returning true in such case will be incorrect.
Ok. Leaving UncommonTrapKind and ExceptionKind uninitialized seems a bit error
prone. Perhaps they can be given some sort of INVALID value.
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeCache.java line
>> 95:
>>
>>> 93: }
>>> 94:
>>> 95: public CodeBlob createCodeBlobWrapper(Address cbAddr, Address start) {
>>
>> I think the use of the name "start" here is a carryover from
>> `findBlobUnsafe(Address start)`. I find it a very misleading name. cbAddr
>> points to the "start" of the blob. "start" points somewhere in the middle of
>> the blob. In fact callers of this API somethimes pass in findStart(addr) for
>> cbAddr, which just adds to the confusion. Perhaps this is a good time to
>> rename "start" to something else, although I can't come up with a good
>> suggestion, but I think anything other than "start" would be an improvement.
>> Maybe "pcAddr". That aligns with the "for PC=" message below. Or maybe just
>> "ptr" which aligns with `createCodeBlobWrapper(findStart(ptr), ptr);`
>
> `cbPc` with comment explaining that it could be inside code blob.
That sounds fine.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953818292
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953819796
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953821968
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953822595
