RFR: 8327647: Occasional SIGSEGV in markWord::displaced_mark_helper() for SPECjvm2008 sunflow [v6]

Dean Long dlong at openjdk.org
Sun Apr 28 07:29:08 UTC 2024


On Sun, 28 Apr 2024 03:01:36 GMT, SUN Guoyun <duke at openjdk.org> wrote:

>> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Improved comment
>
> src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp line 1787:
> 
>> 1785:   lea(cache, Address(cache, index));
>> 1786:   // Prevents stale data from being read after the bytecode is patched to the fast bytecode
>> 1787:   membar(MacroAssembler::LoadLoad);
> 
> if we put LoadLoad in here, maybe it is redundant for TemplateTable::patch_bytecode(..)
> 
> https://github.com/openjdk/jdk/blob/a08af97fda06e785d1c3a5a17e562e65150bbb07/src/hotspot/cpu/aarch64/templateTable_aarch64.cpp#L192
> 
> because there has a Load-acquire in L199.
> can we change 
> `load_field_entry(Register cache, Register index, int bcp_offset = 1) ` 
> to 
> `load_field_entry(Register cache, Register index, int bcp_offset = 1, bool needLoadLoad = 1) `?
> 
> then change L192 like this
> <pre>
> @@ -189,7 +189,7 @@ void TemplateTable::patch_bytecode(Bytecodes::Code bc, Register bc_reg,
>        // additional, required work.
>        assert(byte_no == f1_byte || byte_no == f2_byte, "byte_no out of range");
>        assert(load_bc_into_bc_reg, "we use bc_reg as temp");
> -      __ load_field_entry(temp_reg, bc_reg);
> +      __ load_field_entry(temp_reg, bc_reg, 1 /*bcp_offset*/, false /*needLoadLoad*/);
>        if (byte_no == f1_byte) {
>          __ lea(temp_reg, Address(temp_reg, in_bytes(ResolvedFieldEntry::get_code_offset())));
>        } else {
>          __ lea(temp_reg, Address(temp_reg, in_bytes(ResolvedFieldEntry::put_code_offset())));
>        }                                                                         
>        // Load-acquire the bytecode to match store-release in ResolvedFieldEntry::fill_in()
>        __ ldarb(temp_reg, temp_reg);
> </pre>

I believe patch_bytecode only needs a LoadStore between reading the cache bytecode and patching at bcp[0], so I would agree the LoadLoad in load_field_entry is not needed here.  But the reason is not because we have already load-acquire.  The reason is because there are not two loads that need ordering.  There is only a load and a store.  We can't replace the load-acquire with a LoadLoad, for example.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18477#discussion_r1582048233


More information about the hotspot-compiler-dev mailing list