RFR: 8315954: getArgumentValues002.java fails on Graal [v2]

[Tom Rodriguez]

On Fri, 15 Sep 2023 13:56:00 GMT, Doug Simon <dnsimon at openjdk.org> wrote:

>> This PR adds `HotSpotResolvedJavaMethod.getOopMapAt` to get the oop map for a method at a given BCI. This is required to do correct clearing of oops at OSR entry points.
>> 
>> As part of this addition, I needed to be able to detect requests for oop maps at invalid BCIs. For this, I added `InterpreterOopMap::has_valid_mask()`. When an oop map computation is requested for an invalid BCI, this method returns false.
>
> Doug Simon has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - generalized getLiveObjectLocalsAt to getOopMapAt
>  - need to zero oop_map_buf
>  - simplified getLiveObjectLocalsAt and moved it from ResolvedJavaMethod to HotSpotResolvedJavaMethod

This looks good now.

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 3099:

> 3097:   JVMCIPrimitiveArray oop_map = JVMCIENV->wrap(oop_map_handle);
> 3098:   int oop_map_len = JVMCIENV->get_length(oop_map);
> 3099:   if (nwords > oop_map_len) {

Should we sanity check against `mask.number_of_entries()`?  One wrinkle here is that `compute_one_oop_map` also computes information about the stack so the mask it computes can be larger than just max_locals.  For the purposes of OSR this doesn't matter as none of the JITs support OSR with a non-empty stack, so we would never call it for a bci with a non-empty stack.  So should we disallow calling it with a non-empty stack or just properly handle it by passing in an array long enough to contain `max_locals + max_stack`?

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethod.java line 136:

> 134:      * Computes which local variables contain live object values
> 135:      * at the instruction denoted by {@code bci}. This is the "oop map" used
> 136:      * by the garbage collector.

`by the garbage collector for interpreter frames.`

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java line 788:

> 786: 
> 787:     @Override
> 788:     public long getLiveObjectLocalsAt(int bci, BitSet bigOopMap) {

This seems overly complicated to me.  Why isn't it simply:

   public BitSet getLiveObjectLocalsAt(int bci) {
        int locals = getMaxLocals();
        int nwords = ((locals - 1) / 64) + 1;
        long liveness[] = new long[nwords];
        compilerToVM().getLiveObjectLocalsAt(this, bci, liveness);
        return new BitSet(liveness);
    }

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaMethod.java line 494:

> 492:      *         by the current JVMCI runtime
> 493:      */
> 494:     default long getLiveObjectLocalsAt(int bci, BitSet bigOopMap) {

I think this should be in HotSpotResolvedJavaMethod since there's no apparent need for it in SVM.

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

Marked as reviewed by never (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15705#pullrequestreview-1627408725
PR Review Comment: https://git.openjdk.org/jdk/pull/15705#discussion_r1326305491
PR Review Comment: https://git.openjdk.org/jdk/pull/15705#discussion_r1326294532
PR Review Comment: https://git.openjdk.org/jdk/pull/15705#discussion_r1324808658
PR Review Comment: https://git.openjdk.org/jdk/pull/15705#discussion_r1324809645