RFR: 8337396: Cleanup usage of ExternalAddess [v2]

[Andrew Dinn]

On Thu, 1 Aug 2024 03:19:01 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> `ExternalAddess` should be used only for data load. For calls (and jump) instructions we should use `RuntimeAddress` which uses `runtime_call_Relocation`.
>> 
>> I found few places where `ExternalAddess` is used incorrectly and fixed them.
>> 
>> I also added code to print "hottest" (most referenced) `ExternalAddess` addresses in global table to move them into static global tables which will be introduced by [JDK-8334691](https://bugs.openjdk.org/browse/JDK-8334691) and [JDK-8337519](https://bugs.openjdk.org/browse/JDK-8337519).
>> 
>> Here is current output from debug VM on MacBook M1 (Aarch64):
>> 
>> External addresses table: 6 entries, 324 accesses
>> 0:      158 0x00000001082de0f0 : extn: vmClasses::_klasses+480
>> 1:       84 0x00000001082ddf20 : extn: vmClasses::_klasses+16
>> 2:       40 0x00000001082c4790 : extn: SharedRuntime::_partial_subtype_ctr
>> 3:       24 0x00000001082bdb04 : extn: JvmtiExport::_should_notify_object_alloc
>> 4:       18 0x0000000118384080 : stub: forward exception
>> 
>> 
>> on MacOS-x64:
>> 
>> External addresses table: 143 entries, 44405 accesses
>> 0:    11766 0x00000001047922a0 : extn: CompressedOops::_narrow_oop
>> 1:    11002 0x0000000104474384 : 'should not reach here'
>> 2:     9672 0x0000000104581a90 : extn: ClassLoader::file_name_for_class_name(char const*, int)::class_suffix+882068
>> 3:     2447 0x0000000104508005 : extn: ClassLoader::file_name_for_class_name(char const*, int)::class_suffix+383753
>> 4:     1916 0x000000010458188e : extn: ClassLoader::file_name_for_class_name(char const*, int)::class_suffix+881554
>> 
>> 
>> and on linux-x64:
>> 
>> External addresses table: 143 entries, 77297 accesses
>> 0:    22334 0x00007f35d5b9c000 : ''
>> 1:    19789 0x00007f35d55eea1f : 'should not reach here'
>> 2:    18366 0x00007f35d5747bb8 : 'MacroAssembler::decode_heap_oop: heap base corrupted?'
>> 3:     5036 0x00007f35d56e4d40 : 'uncommon trap returned which should never happen'
>> 4:     3643 0x00007f35d57479f8 : 'MacroAssembler::encode_heap_oop: heap base corrupted?'
>> 
>> 
>> Few points about difference in output:
>> 1. aarch64 does not use `ExternalAddess` or any relocation for messages (strings).
>> 2. `stub: forward exception` corresponds to `StubRoutines::forward_exception_entry()` for which C2 generates tail-call from [C2's stubs](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/generateOptoStub.cpp#L258C48-L258C87). It is difficult to convert it to `RuntimeAddress` because how relocation for constants in C2 are handled.
>> 3...
>
> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add missed ExternalAddress changes

Hi Vladimir,

If I am following your comment regarding forward exception correctly, it seems that this is being encoded as an ExternalAddress because:

1. the raw address constant for StubRoutines::forward_exception_entry() passed into the TailCallNode gets processed in the AD back end by moving the constant into a register
2. there is no way to mark the constant passed to the TailCallNode as requiring a runtime_call_type reloc
3. so the address move defaults to using external_word_type

I'm wondering if this can be fixed in the back end.

Firstly, can I just clarify that is it just the forward exception target that needs a runtime_call_type reloc? The other uses of TailCallNode and TailJumpNode transfer control from a stub to a return address passed as argument to the call. So, there is no constant supplied as argument and hence nothing to relocate for those cases.

If so then could we use a match rule to detect this case e.g.

    instruct TailCalljmpInd(immP jump_target, inline_cache_RegP method_ptr)
    %{
      match(TailCall jump_target method_ptr);
      . . .

If we provide this rule with an encoding that loads the target using a runtime reloc then it should be preferred on cost grounds to the loadConP rule which uses the default external reloc.

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

PR Comment: https://git.openjdk.org/jdk/pull/20412#issuecomment-2263153016