RFR: 8359646: C1 crash in AOTCodeAddressTable::add_C_string

[Andrew Dinn]

On Tue, 17 Jun 2025 02:34:24 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

> It is concurrency issue. Call to `AOTCodeAddressTable::add_C_string()` happened after checks that AOT code cache is still opened. But, because there is no synchronization, other thread (VM) closed/delete AOT code cache (after dumping) before code in `add_C_string()` accessed it.
> 
> Added missed AOTCodeCStrings_lock in places where we modify, store and delete AOT strings table. Moved MutexLocker from `AOTCodeAddressTable::add_C_string()` to its caller and do additional check after it.
> 
> I also noticed that we missed similar check after Compile_lock when we are storing AOT code.
> 
> Tested hs-tier1-6,hs-tier10-rt,stress,xcomp

Looks good

src/hotspot/share/code/aotCodeCache.cpp line 823:

> 821:   // and the main thread generating adapter
> 822:   MutexLocker ml(Compile_lock);
> 823:   if (!is_on()) {

Just for the record:

I can see why this is needed to stop compiler threads dereferencing a null cache pointer or null table pointer when some other thread might concurrently be closing the cache.

That led me to wonder why we don't need to further synchronize concurrent execution of non-compiler threads. I convinced myself that whenever a non-compiler thread calls `AOTCodeCache::close()` the only other running threads that might try to access the AOT cache are compiler threads -- calls to the `close()` method are from `MetaspaceShared::preload_and_dump_impl` (metaspaceShared.cpp), `before_exit` (java.cpp) and `Threads::create_vm` (threads.cpp). (Well, modulo a rogue jcmd, perhaps?).

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

Marked as reviewed by adinn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25841#pullrequestreview-2934593364
PR Review Comment: https://git.openjdk.org/jdk/pull/25841#discussion_r2151615622