RFR: 8221554: aarch64 cross-modifying code

[David Holmes]

Hi Alan,

On 1/10/2020 2:30 am, Alan Hayward 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.

I don't agree with the change here. The cross_modify_fence() is not 
related to thread API imo, it belongs in OrderAccess. The name was 
deliberately selected to abstract away from the specific details of why 
a given platform may need this fence:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2019-March/037153.html

"The name "instruction_pipeline" seems a bit implementation specific 
about what HW architectural features need to be taken care of due to 
cross-modifying code, which may or may not apply to a given platform. 
Perhaps cross_modify_fence(), or something along those lines, would be 
better. That makes it more clear what we are protecting against, as 
opposed to what HW architectural features that might concern on a given 
platform."

@robehn , @fisk please chime in here. :)

Thanks,
David

> 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.
> 
> -------------
> 
> Commit messages:
>   - AArch64: Add cross modify fence verification
>   - AArch64: Use cross_modify_fence instead of maybe_isb
>   - Split cross_modify_fence
> 
> Changes: https://git.openjdk.java.net/jdk/pull/428/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=428&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8221554
>    Stats: 179 lines in 26 files changed: 127 ins; 7 del; 45 mod
>    Patch: https://git.openjdk.java.net/jdk/pull/428.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/428/head:pull/428
> 
> PR: https://git.openjdk.java.net/jdk/pull/428
>