On Thu, 13 Feb 2025 02:06:57 GMT, Chris Plummer <[email protected]> wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix Zero VM build
>
> 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.
> 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.
> 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.
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953732919
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953733212
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953738572
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953745389
