RFR: 8283689: Update the foreign linker VM implementation [v7]

Tue May 10 21:06:18 UTC 2022

On Mon, 9 May 2022 10:28:27 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Hi,
>> 
>> This PR updates the VM implementation of the foreign linker, by bringing over commits from the panama-foreign repo.
>> 
>> This is split off from the main JEP integration for 19, since we have limited resources to handle this. As such, this PR might fall over to 20.
>> 
>> I've written up an overview of the Linker architecture here: http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful to read that first.
>> 
>> This patch moves from the "legacy" implementation, to what is currently implemented in the panama-foreign repo, except for replacing the use of method handle combinators with ASM. That will come in a later path. To recap. This PR contains the following changes:
>> 
>> 1. VM stubs for downcalls are now generated up front, instead of lazily by C2 [1].
>> 2. the VM support for upcalls/downcalls now support all possible call shapes. And VM stubs and Java code implementing the buffered invocation strategy has been removed  [2], [3], [4], [5].
>> 3. The existing C2 intrinsification support for the `linkToNative` method handle linker was no longer needed and has been removed [6] (support might be re-added in another form later).
>> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now implements RuntimeBlob directly. Binding to java classes has been rewritten to use javaClasses.h/cpp (this wasn't previously possible due to these java classes being in an incubator module) [7], [8], [9].
>> 
>> While the patch mostly consists of VM changes, there are also some Java changes to support (2).
>> 
>> The original commit structure has been mostly retained, so it might be useful to look at a specific commit, or the corresponding patch in the [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) repo as well. I've also left some inline comments to explain some of the changes, which will hopefully make reviewing easier.
>> 
>> Testing: Tier1-4
>> 
>> Thanks,
>> Jorn
>> 
>> [1]: https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358
>> [2]: https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49
>> [3]: https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3
>> [4]: https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3
>> [5]: https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120
>> [6]: https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a
>> [7]: https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062
>> [8]: https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f
>> [9]: https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9
>
> Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits:
> 
>  - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>  - Remove unneeded ComputeMoveOrder
>  - Remove comment about native calls in lcm.cpp
>  - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64
>    
>    Reviewed-by: jvernee, mcimadamore
>  - Update riscv and arm stubs
>  - Remove spurious ProblemList change
>  - Pass pointer to LogStream
>  - Polish
>  - Replace TraceNativeInvokers flag with unified logging
>  - Fix other platforms, take 2
>  - ... and 11 more: https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91

Nice work! Looks good. Some minor comments/questions follow.

src/hotspot/cpu/aarch64/frame_aarch64.cpp line 379:

> 377:   // need unextended_sp here, since normal sp is wrong for interpreter callees
> 378:   return reinterpret_cast<OptimizedEntryBlob::FrameData*>(
> 379:     reinterpret_cast<char*>(frame.unextended_sp()) + in_bytes(_frame_data_offset));

Maybe use `address` instead of `char*`?

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5531:

> 5529: }
> 5530: 
> 5531: // On 64 bit we will store integer like items to the stack as

Time for a cleanup? `64 bit` vs `64bit`, `abi`, `Aarch64`.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5547:

> 5545:   } else if (dst.first()->is_stack()) {
> 5546:     // reg to stack
> 5547:     // Do we really have to sign extend???

Obsolete? Remove?

src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 306:

> 304:   intptr_t exception_handler_offset = __ pc() - start;
> 305: 
> 306:   // Native caller has no idea how to handle exceptions,

Can you elaborate, please, how it is expected to work in presence of asynchronous exceptions? I'd expect to see a code which unconditionally clears pending exception with an assertion that verifies that the exception is of expected type.

src/hotspot/cpu/x86/foreign_globals_x86.hpp line 30:

> 28: #include "utilities/growableArray.hpp"
> 29: 
> 30: class outputStream;

Redundant declaration?

src/hotspot/cpu/x86/foreign_globals_x86_64.cpp line 52:

> 50: 
> 51:   objArrayOop inputStorage = jdk_internal_foreign_abi_ABIDescriptor::inputStorage(abi_oop);
> 52:   loadArray(inputStorage, INTEGER_TYPE, abi._integer_argument_registers, as_Register);

`loadArray` helper looks a bit misleading. In presence of `javaClass`-style accessors, it misleadingly hints that it refers to some Java-level operation/entity, though what it does it parses register list representation backed by a Java array. I suggest to rename it to something like `parse_argument_registers_array()`).

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 933:

> 931:     } else {
> 932:       assert(dst.is_single_reg(), "not a stack pair: (%s, %s), (%s, %s)",
> 933:        src.first()->name(), src.second()->name(), dst.first()->name(), dst.second()->name());

Wrong indentation.

src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 36:

> 34: #include "code/nativeInst.hpp"
> 35: #include "code/vtableStubs.hpp"
> 36: #include "compiler/disassembler.hpp"

Redundant includes? No new code added in the file.

src/hotspot/share/c1/c1_GraphBuilder.cpp line 4230:

> 4228: 
> 4229:   case vmIntrinsics::_linkToNative:
> 4230:     print_inlining(callee, "Native call", /*success*/ false);

Since the message is appended, lower case is preferred:`"native call"`.

src/hotspot/share/code/codeBlob.hpp line 754:

> 752: class ProgrammableUpcallHandler;
> 753: 
> 754: class OptimizedEntryBlob: public RuntimeBlob {

What's the motivation to move `OptimizedEntryBlob` up in the hierarchy (from `BufferBlob` to `RuntimeBlob`)?

src/hotspot/share/opto/callGenerator.cpp line 1131:

> 1129: 
> 1130:     case vmIntrinsics::_linkToNative:
> 1131:     print_inlining_failure(C, callee, jvms->depth() - 1, jvms->bci(),

Why is it unconditionally reported as inlining failure?

src/hotspot/share/prims/foreign_globals.cpp line 147:

> 145: // based on ComputeMoveOrder from x86_64 shared runtime code.
> 146: // with some changes.
> 147: class ForeignCMO: public StackObj {

Considering how seldom it is used, I don't see much value in abbreviating it. Also, the comment is misleading: there's no such entity as `ComputeMoveOrder` in the code. And `compute_move_order` is completely removed by this change.

src/hotspot/share/prims/foreign_globals.cpp line 217:

> 215: 
> 216:  public:
> 217:   ForeignCMO(int total_in_args, const VMRegPair* in_regs, int total_out_args, VMRegPair* out_regs,

I propose to turn it into a trivial ctor and move all the logic into a helper static function which returns the computed moves.

src/hotspot/share/prims/foreign_globals.hpp line 35:

> 33: #include CPU_HEADER(foreign_globals)
> 34: 
> 35: class CallConvClosure {

Just a question on terminology: why is it called a `Closure`?

src/hotspot/share/prims/foreign_globals.hpp line 62:

> 60: 
> 61: 
> 62: class JavaCallConv : public CallConvClosure {

Does it really worth to abbreviate `CallingConvention` to `CallConv`?

src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 313:

> 311:         MethodType newType = oldType.dropParameterTypes(destIndex, destIndex + 1);
> 312:         int[] reorder = new int[oldType.parameterCount()];
> 313:         if (destIndex < sourceIndex)

Misses braces.

src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/AArch64Architecture.java line 169:

> 167:             stackAlignment,
> 168:             shadowSpace,
> 169:                 targetAddrStorage, retBufAddrStorage);

Wrong indentation.

src/java.base/share/classes/jdk/internal/foreign/abi/x64/X86_64Architecture.java line 156:

> 154:             stackAlignment,
> 155:             shadowSpace,
> 156:                 targetAddrStorage, retBufAddrStorage);

Wrong indentation.

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

PR: https://git.openjdk.java.net/jdk/pull/7959