Publish MethodStructure native/code state safely (volatile)

[lagergren]

Makes MethodStructure.m_fNative and m_code volatile. Two modifiers, plus a test that pins them.

Why this is needed

These two fields are written by one thread and read by others, with no happens-before edge between them — and, crucially, they are read together:

public Code ensureCode() {
    if (isNative() || !hasCode()) {      // reads m_fNative
        return null;
    }
    Code code = m_code;                  // then reads m_code
    if (code == null) {
        m_code = code = new Code(this);  // ...and writes it, unsynchronized
    }
    return code;
}

Meanwhile markNative() performs a multi-step transition over that same state:

public void markNative() {
    setAbstract(false);
    resetRuntimeInfo();      // clears cached runtime info
    m_fNative    = true;
    m_fTransient = true;
}

With both fields non-volatile there is nothing ordering those writes against those reads, so a racing reader can:

1. Pair a stale flag with missing state — observe native == false (not yet visible) together with a m_code that is still null, take the non-native branch, and fail. getOps() turns that into a hard IllegalStateException("... has no code").
2. Observe a partially constructed Code — the plain write m_code = new Code(this) can become visible before the object's own initialisation does.
3. Duplicate the construction — two threads each build a Code. Benign today, but it means "one Code per method" is not actually guaranteed.

Why it matters now rather than in principle

It is not reachable in a strictly single-threaded link-then-run sequence, which is why it has not bitten in the CLI. It becomes reachable the moment a second thread touches a MethodStructure while another marks it native or forces its code — concurrent compilation, a warm/resident runtime serving repeated work, an embedding host, or a JIT/interpreter mix. Those are all directions the project is actively moving in.

The failure mode is the expensive kind: intermittent, timing-dependent, and non-reproducible — an IllegalStateException from getOps() in a run that worked yesterday, with a stack that points at the reader and says nothing about the writer. Cheap to prevent, extremely tedious to diagnose.

Why volatile and nothing more

This is deliberately the minimal fix:

• Behaviour-neutral. No locking is introduced, no hot path is altered, no API changes. Single-threaded behaviour is identical.
• It fixes the two dangerous cases (1) and (2) above, which are the ones that corrupt or crash.
• It does not close case (3), duplicate construction, which needs a double-checked or synchronized ensureCode(). That is a real change to a frequently-called method, so it is deliberately left out of this PR; the duplicate objects are each complete and correct, so the remaining exposure is waste rather than corruption.

The test

MethodStructureVisibilityTest asserts by reflection that both fields are volatile.

It is a regression pin, not a race reproduction, and the PR should be read that way. The defect is provable from the Java Memory Model but not deterministically reproducible: markNative() runs at link time, so the racing window does not reliably overlap without instrumentation. What the test does guarantee is that the edge cannot be silently removed later by someone dropping a modifier — which is exactly how this kind of fix usually regresses. Verified that it fails if either volatile is removed.

Related

Same family as #548 (a diagnostic accessor NPE'ing on an ambient thread-local) and #547 (an unsynchronized scan cache): shared mutable state whose ownership and publication were left implicit. All three were surfaced by driving the compiler and runtime from ordinary Java threads rather than from the CLI's single-threaded path.

[cpurdy]

I'm curious about this: We shouldn't have multiple threads whacking away on a MethodStructure. So it was never intended to be thread-safe. I'm going to suggest Gene review this so because he has more context for this area; the problem may be "upstream" from this.

[lagergren]

Even if we pretend it is never going to happen, it has very much proven to me working on these things that you should write code that is thread safe anyway because you never know under which forms it will run or how it will be appropriated. This is ZERO COST and will case ZERO OVERHEAD, and I strongly encourage it since there have been several real issues with mutable fields where actually multiple threads CAN run them and I add reproducers as such .

Eitherway - you do recognize it is an "both fields at once" write / read - so it is just good form to code it like that, and my intellij even recognizes this and suggests it, even though it is not a specific problem that shows up in the problem view.

I have bumped into this, and I am not sure how, or if my Draft of the Tool API implementation POC does something unsafe, but I would always code these as volatiles. If it runs only in one thread it is free, if it runs in several it is zero cost given what we do and how often we do it and directly safe. Let's just merge it to avoid another future headache?

[lagergren]

Fair challenge, and you're right about two things — but I think the factual premise doesn't hold on current master, so let me put the evidence up rather than argue from principle.

"We shouldn't have multiple threads whacking away on a MethodStructure."

Agreed as intent. But the native flag is already written on a lazy, runtime-reachable path, not at link time.

Writer — TypeConstant.java:5156, inside TypeInfo construction:

if (fRebase && fHasNoCode && !fNative) {
    fNative = true;
    method.markNative();          // writes m_fNative on a SHARED MethodStructure
    pool.invalidateTypeInfos(id.getNamespace());
}

That sits inside ensureTypeInfo() — lazy, on first demand, reached from ~40 call sites under org/xvm/runtime (ClassComposition, CallChain, PropertyComposition, ClassTemplate, xRef, …). And fRebase covers small-s service and small-c const, so it is a common path, not an exotic one.

Reader — MethodInfo.java:1209, inside buildOptimizedMethodChain:

MethodStructure method = body.getMethodStructure();
if (method != null && method.isNative()) {     // reads m_fNative

Also lazy, also on the dispatch path via ensureOptimizedMethodChain().

Both sides run on the XVM service pool — Runtime.java:41, new ThreadPoolExecutor(parallelism, parallelism, …).

So the failure needs no deliberate parallelism, just two services: service A first-touches type T, enters the TypeInfo build, and flips m_fNative = true. Service B is concurrently optimizing a call chain containing that same method and reads isNative(). With no happens-before edge, B can read the stale false, keep Implementation.Explicit, and go on to getOps() → ensureCode(), which finds no code and throws IllegalStateException("has no code") — a crash that reads as a compiler/linker bug and is essentially undiagnosable after the fact.

Where I think you're right, and it's the more important point: "the problem may be upstream." It is. The real defect is that TypeInfo construction mutates shared, interned MethodStructure state at all — a build step writing into metadata that every container shares. volatile makes that mutation visible; it does not make it correct. So yes, please pull Gene in — if the upstream mutation gets removed, this patch becomes unnecessary rather than load-bearing, which is the better outcome.

I'd propose taking the volatile now as a stopgap (it converts an undiagnosable intermittent crash into correct behaviour) with the upstream question tracked separately, rather than blocking on the larger fix. Happy to go the other way if Gene would rather fix it at the source in one go.

One correction to my own earlier framing: I called this zero-overhead, and that is not strictly accurate. A volatile read is a plain load on x86, but on ARM it lowers to ldar (load-acquire), and isNative() is on the dispatch path. The cost is very small, but it isn't literally nothing, and I shouldn't have claimed otherwise.

[lagergren]

Following up with the case for taking the volatile regardless of how the upstream question lands — I don't think it should be held hostage to that fix.

1. The cost is measurable, and I overstated it in my own correction above. I implied isNative() sat on a hot dispatch path. Checking rather than asserting:

• The m_fNative read in buildOptimizedMethodChain happens once per chain build, because the result is cached in m_aBodyResolved — which is already volatile (MethodInfo.java:1695). That path is effectively cold.
• m_code is read through getOps(), called exactly once per Frame construction (Frame.java:148) and cached into final Op[] f_aOp. So: one acquire-load per method invocation, against a Frame constructor that already allocates and initialises a register file.

So the real figure is one acquire-load per invocation on a path that allocates. Not literally zero, but far below measurement noise.

2. The "never intended to be concurrent" prior has a poor record in this exact subsystem. Three fields in the same metadata layer were designed single-threaded and turned out not to be: the implicit-identity cache written from concurrent service threads, delegation synthesis publishing half-built method code, and the TypeInfo placeholder identity race — the last of which describes concurrent TypeInfo builds on one pool as routine. I don't read that as carelessness; I read it as the metadata layer being reachable from more places than any one design intent anticipated.

3. Fixing the upstream writer doesn't retire the field's exposure. TypeConstant.java:5156 is the one I can demonstrate, but it isn't the only writer: ClassStructure.java:3298, four sites in ClassTemplate.java, and the template initNative() calls in xConst/xNumber/xRTType/xArray. Those last run per container — and containers can be created concurrently. So even a clean upstream fix to TypeInfo construction leaves writers behind.

4. The outcomes are asymmetric. If I'm wrong about the race, we paid an acquire-load per invocation. If the single-threaded assumption is wrong anywhere, the symptom is an intermittent IllegalStateException("has no code") that presents as a compiler or linker bug, surfaces far from its cause, and is not reproducible on demand. I'd rather lose the first bet than the second.

5. It is also a declaration of intent. These are lazily-initialised fields read by code that doesn't own them. volatile says so at the declaration. Without it, the next person to touch this reasonably assumes plain-field semantics are fine — which is roughly how we got here.

6. It is trivially reversible, and cheap to reverse in the right direction. If the upstream work genuinely establishes single-threaded ownership, deleting a keyword is a one-line, reviewable change with the reasoning recorded in the field comment. Adding it after a visibility bug ships costs a debugging cycle nobody can reproduce. And it competes with nothing: no lock, no API, no structure that the real fix would have to unwind — it is the smallest possible placeholder.

To be explicit about where I've landed: I'm not arguing this is the right fix. You're correct that the design flaw is TypeInfo construction mutating shared interned MethodStructure state at all, and I'd like Gene to look at that. I'm arguing it's the right thing to have in the tree while that gets worked out, because it is nearly free, reversible, and the failure it prevents is one of the expensive kinds to diagnose.

[lagergren]

So to summarize - the ConstantPool is deeply unsafe for several reasons, especially if you run something while compiling, but not only. This will get my stability way up when I test other things and it is general a good thing to do. There will be negligible overhead for reading and writing this pair as volatiles given the frequency It is in zero hot paths

[cpurdy]

That's a lot of text to get through 😳

I think the biggest problem here is more fundamental: These structures should not be being read by one thread when another thread is modifying them. That's the project I'm working on related to the "bundle" effort, aka cpurdy/immut_xstructs, which disallows changes unless you've explicitly said "I need a mutable copy of this file structure" which either loads it from disk as mutable or takes a copy of a read-only one.

[lagergren]

Correction to my previous comment: this PR is unchanged — still the minimal two-word volatile diff. I briefly pushed a larger design here and have reverted it; the alternative lives on its own branch instead, for you to take or leave. Sorry for the churn.

The design worth considering, on branch lagergren/methodstructure-codestate-record (not a PR, just a branch to look at):

Two volatiles do not fully deliver what this PR's own field comment claims. The comment says the flag and the Code "must be published together". Volatile makes each write visible, but a reader still performs two separate reads, and markNative() is a multi-step transition:

public void markNative() {
    setAbstract(false);
    resetRuntimeInfo();     // m_code = null; m_fNative = false;   <-- intermediate
    m_fNative    = true;    // <-- settled
    m_fTransient = true;
}

Between those statements the object is observably (native=false, code=null). A reader interleaving there passes isNative(), finds a null Code, and builds one for a method that is about to be native — surfacing later as getOps()'s "has no code". Two volatiles narrow nothing about that window.

The branch holds both values in one immutable record behind one volatile reference:

private record CodeState(boolean isNative, Code code) { … }
private transient volatile CodeState m_codeState = CodeState.NONE;

Readers get an atomic snapshot; markNative() performs exactly one state write, so the intermediate state no longer exists rather than merely being visible. ensureCode() publishes via CAS so it cannot clobber a concurrent markNative(). 27 touch sites, all in one file; full javatools suite green on master (334 tests, 0 failures).

So there are three options, and it is your call:

1. Take this PR as-is (two words) — narrows the visibility hole, does not close the interleaving one.
2. Take the record branch instead — closes both, bigger diff.
3. Neither, and fix it upstream — your original instinct, and still the one I think matters most. TypeInfo construction mutating shared interned MethodStructure state is the actual design flaw; both options above make the state machine correct under concurrency without making that mutation right. Worth Gene's eyes regardless of which you pick.

One unrelated thing the branch surfaced, in markNative() on master:

if (getName().equals("compare") && getIdentityConstant().getNamespace().getName().equals("Const")) {
    int q= 0;
}

An empty if with an unused local — looks like a leftover breakpoint hook. Not touched by this PR; just flagging it.

[lagergren]

Removed the debug residue from markNative() in this PR, since it sits in the method the PR is about:

if (getName().equals("compare") && getIdentityConstant().getNamespace().getName().equals("Const")) {
    int q= 0;
}

An empty if with an unused local — a breakpoint hook someone parked and left. Say the word if it is load-bearing for a workflow and I will put it back.

Worth naming what it actually is, though, because the shape recurs: that is trace-level logging, implemented by hand. Someone wanted "tell me when Const.compare gets marked native", and with no logging facility to reach for, the only available mechanism was an empty branch to hang a breakpoint on. It cannot be enabled in a running system, it cannot be enabled for a different method without an edit-and-rebuild, it says nothing when it fires, and it survives into master because nothing flags it.

This is exactly the gap a pervasive logging framework fills with zero overhead on disabled log levels. Happy to help.