Gate against relying on ClassCastException, which ParparVM does not throw (#5531) - #5532
Conversation
ParparVM's CHECKCAST is unchecked: BC_CHECKCAST expands to nothing and BytecodeMethod.optimize() drops the instruction, so a failed cast hands the wrong pointer to the next instruction rather than throwing. Code written to catch the failure therefore behaves differently on iOS than on the simulator and Android -- in the reported case StringToReal.parseDouble ran with a Double as its String argument and died at cn1_intrinsics.h:90 reading java_lang_String_value out of it, uncatchable from Java. Making CHECKCAST throw would put a class check on every cast in every app, so the rule is instead that our own code must not depend on the exception. Adds CastSemanticsVerifier, which reports a CHECKCAST inside a try whose handler catches ClassCastException or a supertype, and wires it into the PR CI Java 8 leg over JavaAPI, core, android and ios. The rule is about the cast, not the handler: a catch(ClassCastException) with no cast under it is fine, because an explicitly thrown ClassCastException still propagates normally (java.util.AbstractSet.equals is that case). Casts already guarded by an instanceof are recognised and never reported, so the remedy the gate asks for satisfies it. Findings are held against scripts/cast-semantics-baseline.txt -- a ratchet of 187 pre-existing sites, most of them casts that merely sit inside a broad defensive guard. New code cannot add entries. Fixes two genuine cases found this way: - Purchase.getReceipts read a storage entry of any type as a List<Receipt> - PropertyIndex.newInstance returned any newInstance() as a business object Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68f440f683
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public String key() { | ||
| return className + "#" + methodName + methodDesc; |
There was a problem hiding this comment.
When a method already present in cast-semantics-baseline.txt gains another unguarded CHECKCAST, this key remains unchanged and baseline.contains(violation.key()) suppresses the new violation along with the old one. Because the baseline already covers 133 methods—including several large methods with multiple existing casts—new code in precisely these debt-heavy areas can reintroduce the iOS crash while the gate passes; the key or comparison needs to preserve individual sites or per-method violation counts.
Useful? React with 👍 / 👎.
| - name: Check cast semantics | ||
| if: ${{ matrix.java-version == 8 }} | ||
| run: | | ||
| mvn -B -q -f vm/pom.xml -pl JavaAPI -am package -DskipTests | ||
| scripts/check-cast-semantics.sh |
There was a problem hiding this comment.
In the inspected .github/workflows/pr.yml, both the pull_request and push triggers ignore scripts/**, while parparvm-tests.yml only watches vm/**; consequently, a change limited to the newly added checker or its baseline runs neither this step nor the verifier tests. This allows enforcement logic or pre-added baseline exemptions to merge without validation, so these two script paths need to trigger an applicable workflow.
Useful? React with 👍 / 👎.
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Cloudflare Preview
|
The baseline shipped in the previous commit was generated against a stale maven/android/target/classes, so three AndroidImplementation methods that exist in the source were missing from it and PR CI failed on code that was not new. Rebuilds android + ios with -Pcompile-android and regenerates: nothing is removed, the four Android findings are added. Adds --require-all, used by CI, so a module that is not built fails the gate instead of silently shrinking its coverage -- which is what let the stale build through. Also records why maven/java-runtime (Ports/CLDC11) is deliberately out of scope: it runs on a real JVM, where a failed cast does throw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e534cb3cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (opcode == Opcodes.ASTORE) { | ||
| dropSlot(((org.objectweb.asm.tree.VarInsnNode) insn).var); | ||
| return; |
There was a problem hiding this comment.
When a guarded local is reassigned after a cast and execution loops back—such as if (o instanceof String) { while (...) { use((String) o); o = other; } }—the second iteration's cast is no longer safe. This linear scan exempts the CHECKCAST before it later encounters the ASTORE, then never revisits the cast after the backward jump, so this pattern passes the gate even though a failed cast can still reach iOS and use the wrong object. The guard analysis needs to account for back-edge control flow or conservatively reject proofs spanning such loops.
Useful? React with 👍 / 👎.
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 147 screenshots: 147 matched. |
|
Compared 147 screenshots: 147 matched. |
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 181 screenshots: 181 matched. |
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
|
Compared 217 screenshots: 217 matched. |
|
Compared 144 screenshots: 144 matched. |
iOS Metal screenshot updatesCompared 149 screenshots: 148 matched, 1 updated.
Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
1 participant
Refs #5531.
What actually happens
CHECKCASTis unchecked in ParparVM.BC_CHECKCASTexpands to nothing(
cn1_globals.h:351,// todo map instanceof and throw typecast exception) andBytecodeMethod.optimize()deletes the instruction outright, so a failed cast handsthe wrong pointer to the next instruction instead of throwing.
The reporter's
optDoubleiswith a
Doubleunder the key. On the simulator and Android the cast throws and thecatchreturns the fallback. On iOS theDoublereachesStringToReal.parseDouble,which calls
s.trim()--Stringis final, so ParparVM devirtualizes and inlines rawfield access -- and the process dies at
cn1_intrinsics.h:90dereferencings->java_lang_String_valueread out of aDouble. No Javacatchcan see it.Confirmed locally with
vm/benchmarks/translate-and-build.sh(SIGSEGV, backtraceabove) and fixed-up code returns the fallback correctly.
Why not make CHECKCAST throw
That would put a class check on every cast in every app. It measures inside benchmark
noise, but it is a cost every user pays to compensate for code that should not have
been written that way. The rule is instead that our own code must not depend on the
exception -- test the type and branch:
The reporter's
optDoublelives inca.weblite.codename1.json(CN1JSON), not in thisrepo; the fix belongs upstream there.
What this PR adds
CastSemanticsVerifier(vm/ByteCodeTranslator) reports aCHECKCASTinside atrywhose handler catchesClassCastExceptionor a supertype -- the handler thatwould absorb the failure everywhere except iOS.
Two things it deliberately does not report, both covered by tests:
catch (ClassCastException)with no cast under it. ParparVM delivers anexplicitly thrown
ClassCastExceptionperfectly well; only the implicit one from afailed
CHECKCASTis missing.java.util.AbstractSet.equalsis exactly this -- itshandler is there for a foreign
containsAll, and flagging it would send peopleediting correct code.
instanceof. javac still emits theCHECKCAST, sowithout this the gate would reject its own remedy. A small local-slot analysis
recognises
ALOAD n; INSTANCEOF T; IFEQ end, drops the proof on reassignment, andstops at the branch target. Anything it does not recognise stays reportable, so being
incomplete costs noise rather than correctness.
scripts/check-cast-semantics.shruns it over JavaAPI, core, android and ios, heldagainst
scripts/cast-semantics-baseline.txt-- a ratchet of 187 pre-existingsites, not an allow-list. Most are casts that merely sit inside a broad defensive
catch (Throwable)and cannot actually fail; new code cannot add entries.Wired into
pr.yml(Java 8 leg), where core/android/ios are already compiled forSpotBugs.
Two genuine cases fixed (both left the baseline as a result):
Purchase.getReceiptsread a storage entry of any type asList<Receipt>andrelied on the cast failing into
catch (Exception)to substitute an empty list.PropertyIndex.newInstancereturned whatevergetClass().newInstance()produced asa
PropertyBusinessObject, relying on the cast to fail intocatch (Exception).PrettyPrinter.valueToStringhad a vestigialtry/catcharound an already-guardedcast; left alone because the file carries a pre-existing Sun BSD header that touching
would pull into the copyright gate, and the
instanceofrecognition already exempts it.Verification
CastSemanticsVerifierTest-- 10 tests, all pinning one behaviour eachByteCodeTranslator: 0 findingsFollow-up
The 187 baselined sites are worth burning down;
IOSImplementation,CommonTransitions.paintandUIBuilder.createContainerare the densest clusters.🤖 Generated with Claude Code