RFR: 8365229: ARM32: c2i_no_clinit_check_entry assert failed after JDK-8364269 [v3]

[Aleksey Shipilev]

On Wed, 13 Aug 2025 13:12:25 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> I don't believe this fix is correct and I'm not clear that it is even needed. Is something actually breaking with the zero code before the latest commit?
>> 
>> The CPU-specific shared runtime code is called to fill in entry addresses for an AdapterHandlerEntry. That does not imply that any such handler is backed by an AdapterBlob. In particular on Zero there cannot be a corresponding AdapterBlob because we have no runtime compiler capable of generating one. But this situation is not just specific to Zero. On all other architectures there is one other case where an AdapterHandlerEntry has no corresponding AdapterBlob -- the abstract method handler's AdapterHandlerEntry is assembled usng several disparate methods that do not belong to a single generated blob.
>> 
>> On CPUs where a blob is created the offsets of its secondary entries must be stored in the blob in order to allow the blob to be correctly saved to and restored from the AOT cache. The offsets are saved when the blob is created and saved when the blob is written to the AOT cache. The associated entries are written into the corresponding AdapterHandlerEntry. At restore time we populate the entry array using the offsets foudn in the restored blob and then update a newly created AdapterHandlerEntry using the blob start address and this array. That never happens on Zero. We never have to translate entries in a Zero AdapterHandlerEntry to offsets and we never have to translate stored offsets to entry addresses.
>> 
>> So, I don't think there is any need to change the assignment of entry addresses in the Zero implementation of SharedRuntime::generate_i2c2i_adapters(). Unless there is something that has actually broken. Indeed, I would argue that the latest patch is not just needless but damaging as it actually removes a sanity check. The original code sets the first 3 entries to a dummy stub that catches an invalid use of the AdapterHandlerEntry. If we ony set the first entry then we have less protection against invalid calls.
>
>> Is something actually breaking with the zero code before the latest commit?
> 
> Yes. In Zero entries point to the same (fake, error-throwing) stub, mostly for diagnostics. Which _also_ means their offsets are all zero, which trips the assert. This readily fails even the simple `make images` on Zero.
> 
> I think Zero can just skip setting the entries, so that we ride on the current code that treats unset entries well.

The alternative is to modify the asserts to accept zero offsets for non-i2c entries, if you think that is better? It caters for Zero, but relaxes the condition for non-Zero code. So it is like choosing where you want to open the assert gap: for Zero, or for everything else?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26746#discussion_r2273443434