RFR: 8261837: SIGSEGV in ciVirtualCallTypeData::translate_from [v2]

[Dean Long]

On Tue, 21 Nov 2023 00:36:37 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Dean Long has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   redo for x86
>
> src/hotspot/cpu/x86/interp_masm_x86.cpp line 87:
> 
>> 85:   // profiling to this obj's klass
>> 86:   xorptr(obj, rscratch1); // get back original value before XOR
>> 87:   xorptr(obj, mdo_addr);
> 
> Was it bug here originally? We had 2 xors (including this) and now 3 for `obj`.

Yes, there was a bug here originally.  `obj` could have contained a bad value.  The code wants to store the Klass* from `load_klass` with the low `TypeEntries::null_seen` bit from `mdo_addr` ORed in.  However, because the low bits of the Klass* are 0, XOR was used to both OR the low bits and SET the high bits, assuming the high bits from `mdo_addr` are 0.
The problem comes from assuming the `mdo_addr` memory value is stable and reloading the value from memory to compare against 0 and TypeEntries::null_seen.  A transient 0 can appear because of non-atomic updates like this:
`orptr(mdo_addr, TypeEntries::null_seen);`
An alternative fix would be to get rid of the races and use atomic compare-and-swap to write new values.

The new 2nd XOR is an optimization, instead of calling load_klass() again, or saving the original `obj` value somewhere else.  The 3nd XOR is a repeat of the 1st XOR.  I don't think this slow path retry is that useful, and not all platforms do it, but I left it in because I wanted to minimize changes.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16750#discussion_r1399919812