RFR: 8306767: Concurrent repacking of extra data in MethodData is potentially unsafe [v4]

[Emanuel Peter]

On Thu, 30 Nov 2023 18:21:29 GMT, Tom Rodriguez <never at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
>> 
>>  - manual merge with master after JDK-8267532
>>  - more locking, still fails tho - WIP
>>  - adding more verification and more locking, WIP
>>  - add locks for jvmci calls to allocate_bci_to_data
>>  - 8306767
>
> I agree with Roland that none of the accesses from C++ code are performance critical so always requiring a lock shouldn't matter for performance.  It is somewhat intrusive though.  The alternative is to make the API distinguish clearly between preallocated data and the extra data and adjust all internal usages to select the right one.  For instance speculative_trap_data_tag usages are only in the extra_data section so access to those could have a distinct API.  The only other thing in extra_data are bit_data_tag so any caller that's looking for a different kind of record is always safe since it must be from the preallocated section.  A change like that might be clarifying in general as well but I think there's a question of effort vs benefit.
> 
> Also to clarify, I never actually observed this problem in practice but inferred the possibility while addressing MDO concurrency issues with Graal.  It would be very hard to notice and very transient but it could lead to crashes since SpeculativeTrapData contains a Method*.

@tkrodriguez Yes, we could now do quite the refactoring. The question is how far I should take this. I think I will just make things safe with locks now, and then if someone really wants to fully refactor all of this, then they can do that separately.

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

PR Comment: https://git.openjdk.org/jdk/pull/16840#issuecomment-1835673004