decomp: Add ARM system intrinsic emission and robust userop handling#281
Merged
Conversation
Two-layer handling of CALLOTHER userops that encode privileged firmware operations (interrupt masking, MSR/MRS special registers, CP15 coprocessor, barriers, WFI/WFE). Previously these emitted under their raw SLEIGH names (disableIRQinterrupts(...), coprocessor_load(...)): preserved but unreadable and not recompilable, with no visibility into coverage gaps. Java (classify + inventory): - UseropClassifier.java maps each userop name to an arch-neutral taxonomy (irq_mask_*, sysreg_*, coproc_*, barrier_*, hint_*, trap, mode_switch, query, unknown) plus a decoded system_register; named CP15 coproc_*_<X> resolve the register from the name suffix. - PcodeSerializer tags recognized CALLOTHER targets with intrinsic_class/system_register/mapped, and emits a top-level intrinsic_manifest via layered discovery: a low-pcode instruction walk (authoritative presence, catches decompiler-elided userops) unioned with high/synthetic userops that reach serialization, each marked with origin and mapped so unmapped_count surfaces coverage gaps loudly. - An unknown userop index no longer drops the whole function; it becomes an opaque __patchestry_unknown_0x<hex> op, flagged in the manifest. C++ (spelling): - OperationTarget carries the new optional fields (parsed in JsonDeserialize). - emit_arm_system_intrinsic maps the class to a compiler builtin (ACLE) / CMSIS-Core name, dispatched first in create_callother: CPS -> __disable_irq/ __enable_irq, named M sysreg -> __get_<R>/__set_<R>, hints -> __wfi/__wfe/ __sev/__yield/__nop, coproc LDC/STC -> __arm_ldc/__arm_stc. Barriers stay on the C11-fence path; generic coproc MCR/MRC (ACLE arg reorder) and non-PRIMASK CPS deliberately fall through rather than mis-spell. Inline-asm implementation + register save/restore is deferred; the ACLE/CMSIS names are the substrate it would later back. Validated: LIT 105/105 (new callother_arm_system_intrinsics.json), gradle build + UseropClassifierTest, headless controller.elf (2789 fns; manifest 18 userops/ 6 unmapped; setISAMode origin:low x3069 proves the layered walk), and a clang cortex-m4 round-trip regenerating wfi/dmb/dsb/isb/mrs/msr + cpsid/cpsie. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Generic single-register coprocessor moves were classified (coproc_read/write/ cdp) but left on the raw registered-call path because ACLE's operand order differs from Ghidra's SLEIGH order (opc2 is in the middle in SLEIGH, last in ACLE). Add emit_arm_coproc, which reorders the serialized inputs and emits the ACLE intrinsic, reusing create_intrinsic_call for decl synthesis + output assignment: coprocessor_moveto(cpn,op1,op2,Rt,CRn,CRm) -> __arm_mcr(cpn,op1,Rt,CRn,CRm,op2) coprocessor_movefromRt(cpn,op1,op2,CRn,CRm) -> __arm_mrc(cpn,op1,CRn,CRm,op2) (returns) coprocessor_function(cpn,op1,op2,CRd,CRn,CRm) -> __arm_cdp(cpn,op1,CRd,CRn,CRm,op2) Arity-guarded: only the exact single-register shapes are reordered; the 3-arg movefromRt variant falls through. The *2 forms (MCRR/MRRC/CDP2) now get distinct classes (coproc_write2/read2/cdp2) in UseropClassifier so they cannot hit the single-register path; they stay on the registered-call path until handled. Validated: callother_arm_system_intrinsics.json extended with MCR/MRC ops (opc1=2/opc2=4 make the swap visible) -> @__arm_mcr(15,2,v,1,3,4) / @__arm_mrc(15,2,1,3,4); LIT 105/105; clang -march=armv7-a round-trip regenerates mcr/mrc; gradle build SUCCESSFUL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
coproc_load/store were spelled as __arm_ldc/__arm_stc, but their ACLE prototype takes the address as `const void *`. patchir emits the address operand with its varnode type (often an integer), so the emitted C failed to recompile against <arm_acle.h> with a hard "incompatible integer to pointer conversion" error -- a silent recompile break (42 such calls in the controller binary). Add an optional pointer_arg_index to create_intrinsic_call that forces an explicit `(const void *)` cast on that argument, and route LDC/STC through it with the address at index 2. The emitted call becomes __arm_ldc(11U, 0U, (const void *)addr), which recompiles cleanly. MCR/MRC are unaffected: their immediate fields emit as integer literals (satisfying the constant-argument requirement) and the value/return args are integers, which __arm_mcr/__arm_mrc accept. Validated: the actual emitted C recompiles via clang -mcpu=cortex-m4 against real <arm_acle.h> + CMSIS, regenerating cpsid/cpsie/ldc/mcr/mrc/mrs/msr/wfi; LIT 105/105 (fixture now asserts the pointer-typed LDC address). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The userop classification (Java) and spelling (C++) were hard-coded to ARM32
even though the taxonomy between them is architecture-neutral. Refactor both
layers behind a registry keyed on the program's processor string so other
architectures can be added without touching the serializer or the CALLOTHER
handler. ARM32 behavior is unchanged.
Java (classify): rename UseropClassifier -> IntrinsicClassifier, now a registry
+ taxonomy holder. The ARM vocabulary moves to Arm32IntrinsicClassifier behind a
new ArchIntrinsicClassifier interface; AArch64IntrinsicClassifier is a stub
(returns unknown) wired so dispatch is exercised. IntrinsicClassifier.classify
takes the processor string (case-insensitive); PcodeSerializer threads
this.architecture into the four classify call sites.
C++ (spell): introduce an ArchSpeller registry {arch, SpellFn, primary_header}
matching the existing data-table idiom. The ARM logic becomes
arm_emit_system_intrinsic; emit_arm_system_intrinsic -> emit_system_intrinsic
dispatches on program_arch() (the arch seam that already flowed but was unused).
aarch64 speller is a fall-through stub. primary_header is reserved for the
header-emission follow-up (not consumed yet).
Validated: LIT 105/105 (ARM fixture through the new dispatch); gradle build
SUCCESSFUL (real, srcDir-redirected) + in-image build SUCCESSFUL; headless
controller.elf manifest byte-identical to pre-refactor (18 distinct, 6 unmapped,
2789 fns) -- ARM path transparent through the registry.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two robustness fixes around the Ghidra decompiler builtin id range (0x10000000+), informed by checking userop.cc upstream (NationalSecurityAgency/ ghidra): Ghidra mints exactly 6 builtins, 0x10000000..0x10000005, contiguous and unchanged through HEAD, so the next free slot is 0x10000006. 1. Collision: patchestry's synthetic exception-return ids previously sat at 0x10000006/0x10000007 -- the next free Ghidra builtin slots. Move them to a dedicated PATCHESTRY_BUILTIN_BASE = 0x20000000 so a future Ghidra builtin cannot collide. The ids are referenced only symbolically (resolveUseropName switch + buildExceptionReturnOp injection), and remain >= BUILTIN_STRINGDATA so they still route through the generic intrinsic path; emitted names (exception_return[_cpsr]) are unchanged. Add GHIDRA_BUILTIN_MAX for clarity. 2. Manifest mis-flag: finalizeManifestOrigins force-marked any index >= 0x10000000 as mapped:true (origin synthetic), which would hide an unrecognized future Ghidra builtin from unmapped_count. Now only a builtin whose name actually resolves (resolveUseropName != null) is marked handled; an unresolved index in the synthetic range stays mapped:false so the manifest surfaces it. Validated: gradle build SUCCESSFUL; headless controller.elf manifest unchanged (18 distinct, 6 unmapped); makair.elf ISR tagging intact (Default_Handler/ SysTick_Handler still is_interrupt) after the id move. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The serialized JSON is the AST-generation input; it should carry only what create_callother consumes. The intrinsic_manifest block (the missing-intrinsic inventory) is a diagnostic that no AST generator reads -- and its low-pcode-walk entries describe userops not even used in any function. Remove it and its machinery: discoverIntrinsicManifest (the per-instruction low-pcode walk), recordManifestLow, finalizeManifestOrigins, serializeManifest, the ManifestEntry type, and the intrinsicManifest field. Kept (these ARE AST inputs): the per-op intrinsic_class/system_register/mapped tagging on used CALLOTHER targets, the is_intrinsic declarations for used userops, and the unknown-index stop-the-drop (__patchestry_unknown_0x<hex>, no whole-function drop). gradle build SUCCESSFUL; C++ LIT path (105/105) unaffected -- it never consumed the manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clearer naming that mirrors the Java side: ArchIntrinsicClassifier (classify what a userop is) <-> ArchIntrinsicEmitter (emit how the compiler sees it). Renames the C++ per-architecture registry: ArchSpeller -> ArchIntrinsicEmitter, SpellFn -> IntrinsicEmitFn, arch_spellers -> arch_intrinsic_emitters, get_speller -> get_intrinsic_emitter. Also drop the reserved primary_header field: the header-emission route is abandoned. Intrinsics are emitted as extern declarations (create_intrinsic_call -> get_or_create_intrinsic_decl, SC_Extern); the emitted C carries its own extern prototypes rather than #include-ing arch headers. No behavior change. LIT 105/105. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the flat is_known_cmsis_sysreg whitelist + "__get_"/"__set_" name concatenation with an explicit per-register CmsisSysreg table so the emitted accessor always matches a real CMSIS-Core builtin: - BASEPRI_MAX is write-only (no __get_BASEPRI_MAX) - IPSR/APSR/xPSR are read-only (no setters) - the combined PSR getter is __get_xPSR, not __get_XPSR An absent getter/setter for the requested direction now falls through to name-based dispatch instead of emitting a builtin that does not exist. Latent today (the Arm32 classifier only emits BASEPRI/MSP/PSP/MSPLIM/ PSPLIM/IPSR) but guards against wrong-name emission as the classifier grows. Also debloat the verbose comments added with the ARM system-intrinsic emitter across the touched files; no other behavior change. LIT callother_arm_system_intrinsics unchanged (__set_BASEPRI, __get_IPSR, coproc reorders, __arm_ldc pointer arg all preserved). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ource of truth Two design cleanups to the ARM system-intrinsic pipeline: (#3) Remove the redundant `mapped` JSON field. The presence of `intrinsic_class` is already the "is classified" signal, the serializer only ever wrote `mapped:true` (and only when classified), and one of the two serializer sites never emitted it. Drop it from the serialized JSON, the OperationTarget struct, and the deserializer. The Java IntrinsicClassifier.Result.mapped boolean stays -- it is real control flow (the serialize gate), not the dead wire field. (#4) Make cmsis_sysregs the single source of truth for register -> accessor. The Java classifier stays structural (sysreg_read/write + raw register); the CMSIS spelling and read-only/write-only validity live only in the C++ table. New cmsis_sysreg_accessor() helper distinguishes intentional fall-through (no accessor for this direction) from drift (a register the table does not cover at all) and logs the latter loudly so a missing row surfaces instead of silently degrading to a placeholder. The duplicated sysreg_read/sysreg_write branches collapse into one helper call; the Java side carries a cross-reference to the C++ table. LIT callother_arm_system_intrinsics unchanged (all known registers spell correctly); injecting an unknown register now warns and falls through. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Contributor
PR Review in Progress
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Two fixes from the claude[bot] PR #281 review: - LDC/STC (coproc_load/loadl/store/storel) now guards op.inputs.size() == 3 before emitting, mirroring emit_arm_coproc's arity checks. A wrong-arity op falls through to the placeholder path instead of emitting a malformed __arm_ldc/stc call (the pointer_arg_index=2 cast would otherwise target the wrong input or never apply). - cmsis_sysreg_accessor asserts klass is sysreg_read/sysreg_write before the getter/setter ternary, so a future caller passing an unexpected class fails loudly in debug instead of silently returning the setter. Verified: existing callother_arm_system_intrinsics fixture still passes; a 2-input LDC now emits __patchestry_missing_coprocessor_load instead of a bad-shape __arm_ldc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Contributor
PR Review Complete
No new blocking issues found. The two issues flagged in the prior round — missing LDC/STC arity guard and missing sysreg-class assert — were both addressed in commits |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds architecture-aware handling for system-level userops, with initial support for ARM CMSIS-Core and ACLE intrinsics. Ghidra-classified system userops are now mapped to compiler intrinsics during code generation, improving the accuracy and portability of emitted C code for ARM firmware.
Key changes include: