RFR: 8221554: aarch64 cross-modifying code [v3]

[Andrew Haley]

On Mon, 12 Oct 2020 12:45:19 GMT, Alan Hayward <github.com+4146708+a74nh at openjdk.org> wrote:

>> The AArch64 port uses maybe_isb in places where an ISB might be required
>> because the code may have safepointed. These maybe_isbs are very conservative
>> and are used in many places are used when a safepoint has not happened.
>> 
>> cross_modify_fence was added in common code to place a barrier in all the
>> places after a safepoint has occurred. All the uses of it are in common code,
>> yet it remains unimplemented on AArch64.
>> 
>> This set of patches implements cross_modify_fence for AArch64 and reconsiders
>> every uses of maybe_isb, discarding many of them. In addition, it introduces
>> a new diagnostic option, which when enabled on AArch64 tests the correct
>> usage of the barriers.
>> 
>> Advantage of this patch is threefold:
>> * Reducing the number of ISBs - giving a theoretical performance improvement.
>> * Use of common code instead of backend specific code.
>> * Additional test diagnostic options
>> 
>> Patch 1: Split cross_modify_fence
>> =================================
>> This is simply refactoring work split out to simplify the other two patches.
>> 
>> instruction_fence() is provided by each target and simply places
>> a fence for the instruction stream.
>> 
>> cross_modify_fence() is now a member of JavaThread and just calls
>> instruction_fence. This function will be extended in Patch 3.
>> 
>> Patch 2: Use cross_modify_fence instead of maybe_isb
>> ====================================================
>> 
>> The [n] References refer to the comments for cross_modify_fence in
>> thread.hpp.
>> 
>> This is all the existing uses of maybe_isb in the AArch64 target:
>> 
>> 1) Instances of Java code calling a VM function
>>    * This encapsulates the changes to:
>>    ** MacroAssembler::call_VM_leaf_base()
>>    ** generate_fast_get_int_field0()
>>    ** stubGenerator_aarch64 generate_throw_exception()
>>    ** sharedRuntime_aarch64 generate_handler_blob()
>>    ** SharedRuntime::generate_resolve_blob()
>>    ** C1 LIR_Assembler::rt_call
>>    ** C1 StubAssembler::call_RT(): used by Used by generate_exception_throw,
>>                                    generate_handle_exception, generate_code_for.
>>    ** OptoRuntime::generate_exception_blob()
>>    * Any changes will be caught due to calls to [2] or [3] by the VM function.
>>    * Any calls that do not call [2] or [3] do not require an ISB.
>>    * This patch is more optimal for these cases.
>> 
>> 2) Instances of Java code calling a JNI function
>>    * This encapsulates the changes to:
>>    ** SharedRuntime::generate_native_wrapper()
>>    ** TemplateInterpreterGenerator::generate_native_entry()
>>    * A safepoint still in progress after the call with be caught by [4].
>>    * An ISB is still required for the case where there was a safepoint
>>      but it completed during the call. This happens if the code doesn't
>>      branch on safepoint_in_progress
>>    * In the SharedRuntime version, the two possible calls to
>>      reguard_yellow_pages and complete_monitor_unlocking_C are after the thread
>>      goes back into it's original state, so are covered by [2] and [3], the
>>      same as a normal VM call.
>>    * This patch is only more optimal for the two post-JNI calls.
>> 
>> 3) Patching functions
>>    * This encapsulates the changes to:
>>    ** patch_callers_callsite() (called by gen_c2i_adapter())
>>    * This results in code being patched, but does not safepoint
>>    * Therefore an ISB is required.
>>    * This patch introduces no change here.
>> 
>> 4) C1 MacroAssembler::emit_static_call_stub()
>>    * Calls ISB (not maybe_isb)
>>    * By design, the patching doesn't require that the up-to-date
>>      destination is required for proper functioning.
>>    * However, the ISB makes it most likely that the new destination will
>>      be picked up.
>>    * This patch introduces no change here.
>> 
>> Patch 3: Add cross modify fence verification
>> ============================================
>> 
>> The VerifyCrossModifyFence diagnostic flag enables confirmation to the correct
>> usage of instruction barriers. It can safely be enabled on any Java run.
>> 
>> Enabling it will cause the following:
>> 
>> * Once all threads have been brought to a safepoint, each thread will be
>>   marked.
>> 
>> * On a cross_modify_fence and safepoint_fence the mark for that thread
>>   will be cleared.
>> 
>> * On entry to a method and in a safepoint poll, then the thread is checked.
>>   If it is marked, then the code will error.
>
> Alan Hayward has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
> 
>  - AArch64: Add cross modify fence verification
>  - AArch64: Use cross_modify_fence instead of maybe_isb
>  - Split cross_modify_fence

Still pending.

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

> 5315: #endif
> 5316: }
> 5317: 

Unless VerifyCrossModifyFence is turned on in debug builds it will almost never be used. Please turn this on by default in AArch64 debug builds.

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

Changes requested by aph (Reviewer).

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