morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #5137] [CLOSED] fix(ghidra): attribute indirect-branch warnings by source address #6783

New issue

Closed

opened 2026-11-01 21:43:00 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:43:00 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5137
Author: @jaelliot
Created: 10/19/2026
Status: ❌ Closed

Base: main ← Head: fix/ghidra-warning-attribution-upstream

📝 Commits (10+)

26fd63c feat(ghidra): report computed branches without a recovered jump table
ac520aa test(ghidra): assert computed branch warning evidence
523cad5 Merge commit '2895a18ca9' into codex/ci-review-pr2977-20261014
fb77645 fix(ghidra): address review on unresolved indirect branches
9a4866d Merge remote-tracking branch 'origin/main' into HEAD
d170ed2 fix(native): preserve indirect branch unknowns without API coverage
50b39ef fix(workflows): invalidate stale native API inspection snapshots
903e659 fix(ghidra): attribute indirect-branch warnings by source address
0e18a17 merge(main): integrate indirect-branch warning attribution with upstream
a02741b Merge main into feat/487-unresolved-indirect-branches

📊 Changes

27 files changed (+1710 additions, -44 deletions)

View changed files

📝 bridge/ghidra/ReaGhidraBridge.java (+214 -0)
📝 docs/native-investigation.md (+26 -1)
📝 docs/testing-lanes.md (+6 -0)
📝 knip.json (+1 -0)
📝 scripts/verify-real-ghidra-assertions.mjs (+3 -0)
📝 scripts/verify-real-ghidra-function.mjs (+153 -0)
➕ scripts/verify-real-ghidra-warning-attribution.mjs (+191 -0)
📝 scripts/verify-real-ghidra.mjs (+57 -25)
📝 src/application/WorkflowAnalysisProfile.ts (+5 -3)
📝 src/application/WorkflowEvidence.test.ts (+14 -4)
📝 src/application/binary/AnalysisSnapshotCache.test.ts (+35 -0)
📝 src/application/native/NativeApiInspection.test.ts (+235 -1)
📝 src/application/native/NativeApiInspection.ts (+34 -4)
📝 src/domain/ghidraValues.fixture.ts (+1 -0)
📝 src/domain/native/nativeApiBoundary.ts (+37 -0)
📝 src/ghidra/GhidraAnalysisProfile.ts (+1 -0)
📝 src/ghidra/GhidraFunctionValues.test.ts (+57 -0)
📝 src/ghidra/GhidraNativeAnalysisValues.ts (+11 -0)
📝 src/ghidra/GhidraProvider.behavior.test.ts (+4 -0)
📝 src/ghidra/GhidraRawProfileDigest.test.ts (+4 -2)

...and 7 more files

📄 Description

Summary and dependency

Draft: #5137 corrects unresolved indirect-branch warning attribution by Ghidra's typed instruction address. It depends on #2977 and must not merge independently of that feature.

Current source head: d9bb66a, after a test-only result-projection correction to the non-force merge 079d733, following the non-force, two-parent merge a8596d7 retaining the original #5137 branch and the rebased #2977/typed-warning integration. It contains current upstream main (d3ab0fa); GitHub reports the 27-file diff as mergeable. The inherited #2977 implementation, source-owner migrations, and regression tests account for most of that diff. Its incremental, historical eight-file source-address fix remains reviewable. Existing contributor history and credit are preserved.

Problem and approach

On real Ghidra, a warning printed as 0x0000100c can refer to an instruction at aarch64_overlay:0x100c, while AVR's printed word-address 0x0804 refers to byte offset 0x1008. Parsing that text as a listing address misattributes genuine warnings and loses ghidra-decompiler-warning Evidence. Caller-authored comments can also repeat genuine warning text.

The Ghidra producer now collects complete warning comments from ClangCommentToken with the exact typed source Address. It accepts only consistent token provenance and discounts an authored warning only when its exact rendered occurrence matches at the same source address. No fallback deletes a different warning when an authored comment is wrapped or omitted. A corrected indirect_branch_inventory analysis-profile commitment (listing-computed-jumps-typed-warning-address-v2) invalidates older cache/snapshot results. Unrecovered branch targets remain explicitly unknown.

The existing source-owned AArch64, AVR8 and MIPS32 probe exercises non-entry computed branches, overlay/default collisions, word-address units, delay slots, misleading string literals, wrong addresses, and missing/wrapped comment sources. The current REA schema/application owners and recent workflow revisions are preserved rather than overwritten. This correction adds no general-purpose target execution, arbitrary PSP support or external dependency.

Executed verification and limits

Negative real-provider control: fork run 41359021877 reached Ghidra 12.1.4 and failed specifically at aarch64_overlay_outcome before the typed-producer correction.
Positive real-provider correction: fork run 41359355962 passed the same source-owned AArch64 default/overlay, AVR word-address and MIPS delay-slot probe, including misleading-comment controls, bridge compilation and owned cleanup.
Profile and snapshot validation: fork run 41359939917 passed the corrected real-Ghidra probe and 216/216 focused tests across raw profile digests, provider behavior and snapshot replay. An additional general AArch64 lane in that fork-only run did not qualify: its unrelated native-value compare_functions request exceeded the existing default 1,024 MCP input-element limit.
Current upstream PR head: CI run 41367791962 has passed package build, documentation and Source tests at d9bb66a. Built-dependent test shards remain in progress at this checkpoint; the run is not fully green. The preceding 079d733 run passed package build, documentation and source tests; one built test failed because the new legacy-snapshot regression passed an unprojected {ok,value} to parseEvidence. That regression now uses the existing projectCliAnalysisResult helper for both current and replayed results. Static checks on the previous head listed only 16 format issues also present on upstream main, none in these changed files.

The final 27-file tree shares its implementation and verification-code blobs with the positively tested fork revision, except for the updated documentation. The temporary fork-only GitHub Actions trigger is not part of this PR. The exact merge commit has not been rerun through the optional real-Ghidra lane. Earlier historical results in this thread apply to their original revisions, not automatically to the current head.

Not established: live PSP/Allegrex or Monster Hunter runtime correctness, arbitrary module/PRX admission, true overlay residency, Windows/macOS equivalence, or the complete public CLI/MCP path for this source-owned typed-warning probe. AVR testing covers the Ghidra-producing boundary, not AVR admission.

Author readiness

Human owner: @jaelliot

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Prepared with AI assistance from ChatGPT/Codex. This draft remains dependent on #2977 and human review.

Before merge

#2977's canonical integration and dependency relationship are accepted upstream; reduce the inherited diff after that merge.
Review final scope, source attribution and relevant tests; confirm no unsupported authority, unrelated generated files, or overstated provider claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/5137 **Author:** [@jaelliot](https://github.com/jaelliot) **Created:** 10/19/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/ghidra-warning-attribution-upstream` --- ### 📝 Commits (10+) - [`26fd63c`](https://github.com/morluto/rea/commit/26fd63c3966f7845145df8d9ca2437a0134eac22) feat(ghidra): report computed branches without a recovered jump table - [`ac520aa`](https://github.com/morluto/rea/commit/ac520aab522882e75f96fa72b1f04701db634ced) test(ghidra): assert computed branch warning evidence - [`523cad5`](https://github.com/morluto/rea/commit/523cad54fc41c2bb5762679d2b96caef8e02cde4) Merge commit '2895a18ca92f415ae62f87167ba9c7a672e3b7dc' into codex/ci-review-pr2977-20261014 - [`fb77645`](https://github.com/morluto/rea/commit/fb77645644435c7092700f9e1b0f0495466cf80f) fix(ghidra): address review on unresolved indirect branches - [`9a4866d`](https://github.com/morluto/rea/commit/9a4866d0fbb687e631efda477321401a3be00c73) Merge remote-tracking branch 'origin/main' into HEAD - [`d170ed2`](https://github.com/morluto/rea/commit/d170ed2ceaa327a0fd8081735f613b3becf1a695) fix(native): preserve indirect branch unknowns without API coverage - [`50b39ef`](https://github.com/morluto/rea/commit/50b39effa77e3cf15c1f821c5608b2a9d705d6ea) fix(workflows): invalidate stale native API inspection snapshots - [`903e659`](https://github.com/morluto/rea/commit/903e6592d8f7f4bc0f64b678b54e233cfb84831f) fix(ghidra): attribute indirect-branch warnings by source address - [`0e18a17`](https://github.com/morluto/rea/commit/0e18a17dbd7c6d8151bfd25ace37f385b3e96915) merge(main): integrate indirect-branch warning attribution with upstream - [`a02741b`](https://github.com/morluto/rea/commit/a02741b2117f99a179bef1f1ca821fff5fd30711) Merge main into feat/487-unresolved-indirect-branches ### 📊 Changes **27 files changed** (+1710 additions, -44 deletions) <details> <summary>View changed files</summary> 📝 `bridge/ghidra/ReaGhidraBridge.java` (+214 -0) 📝 `docs/native-investigation.md` (+26 -1) 📝 `docs/testing-lanes.md` (+6 -0) 📝 `knip.json` (+1 -0) 📝 `scripts/verify-real-ghidra-assertions.mjs` (+3 -0) 📝 `scripts/verify-real-ghidra-function.mjs` (+153 -0) ➕ `scripts/verify-real-ghidra-warning-attribution.mjs` (+191 -0) 📝 `scripts/verify-real-ghidra.mjs` (+57 -25) 📝 `src/application/WorkflowAnalysisProfile.ts` (+5 -3) 📝 `src/application/WorkflowEvidence.test.ts` (+14 -4) 📝 `src/application/binary/AnalysisSnapshotCache.test.ts` (+35 -0) 📝 `src/application/native/NativeApiInspection.test.ts` (+235 -1) 📝 `src/application/native/NativeApiInspection.ts` (+34 -4) 📝 `src/domain/ghidraValues.fixture.ts` (+1 -0) 📝 `src/domain/native/nativeApiBoundary.ts` (+37 -0) 📝 `src/ghidra/GhidraAnalysisProfile.ts` (+1 -0) 📝 `src/ghidra/GhidraFunctionValues.test.ts` (+57 -0) 📝 `src/ghidra/GhidraNativeAnalysisValues.ts` (+11 -0) 📝 `src/ghidra/GhidraProvider.behavior.test.ts` (+4 -0) 📝 `src/ghidra/GhidraRawProfileDigest.test.ts` (+4 -2) _...and 7 more files_ </details> ### 📄 Description ## Summary and dependency **Draft: #5137 corrects unresolved indirect-branch warning attribution by Ghidra's typed instruction address. It depends on [#2977](https://github.com/morluto/rea/pull/2977) and must not merge independently of that feature.** Current source head: [`d9bb66a`](https://github.com/jaelliot/rea/commit/d9bb66a98aef6b50975a11fe5939253d9ee25919), after a test-only result-projection correction to the non-force merge [`079d733`](https://github.com/jaelliot/rea/commit/079d73370d4c4105bf17b996b02c82fb8bfcb78a), following the non-force, two-parent merge [`a8596d7`](https://github.com/jaelliot/rea/commit/a8596d791ec2d42b8b81c42b0f36b07a50c0e7a2) retaining the original #5137 branch and the [rebased #2977/typed-warning integration](https://github.com/jaelliot/rea/commit/b0b081141977923b5a32e283a24ffe8ee6d00440). It contains current upstream `main` (`d3ab0fa`); GitHub reports the 27-file diff as mergeable. The inherited #2977 implementation, source-owner migrations, and regression tests account for most of that diff. Its incremental, historical [eight-file source-address fix](https://github.com/jaelliot/rea/compare/50b39effa77e3cf15c1f821c5608b2a9d705d6ea...903e6592d8f7f4bc0f64b678b54e233cfb84831f) remains reviewable. Existing contributor history and credit are preserved. ## Problem and approach On real Ghidra, a warning printed as `0x0000100c` can refer to an instruction at `aarch64_overlay:0x100c`, while AVR's printed word-address `0x0804` refers to byte offset `0x1008`. Parsing that text as a listing address misattributes genuine warnings and loses `ghidra-decompiler-warning` Evidence. Caller-authored comments can also repeat genuine warning text. The Ghidra producer now collects *complete warning comments* from `ClangCommentToken` with the exact typed source `Address`. It accepts only consistent token provenance and discounts an authored warning **only when its exact rendered occurrence matches at the same source address**. No fallback deletes a different warning when an authored comment is wrapped or omitted. A corrected `indirect_branch_inventory` analysis-profile commitment (`listing-computed-jumps-typed-warning-address-v2`) invalidates older cache/snapshot results. Unrecovered branch targets remain explicitly unknown. The existing source-owned AArch64, AVR8 and MIPS32 probe exercises non-entry computed branches, overlay/default collisions, word-address units, delay slots, misleading string literals, wrong addresses, and missing/wrapped comment sources. The current REA schema/application owners and recent workflow revisions are preserved rather than overwritten. This correction adds no general-purpose target execution, arbitrary PSP support or external dependency. ## Executed verification and limits - **Negative real-provider control:** [fork run 41359021877](https://github.com/jaelliot/rea/actions/runs/41359021877) reached Ghidra 12.1.4 and failed specifically at `aarch64_overlay_outcome` before the typed-producer correction. - **Positive real-provider correction:** [fork run 41359355962](https://github.com/jaelliot/rea/actions/runs/41359355962) passed the same source-owned AArch64 default/overlay, AVR word-address and MIPS delay-slot probe, including misleading-comment controls, bridge compilation and owned cleanup. - **Profile and snapshot validation:** [fork run 41359939917](https://github.com/jaelliot/rea/actions/runs/41359939917) passed the corrected real-Ghidra probe and **216/216 focused tests** across raw profile digests, provider behavior and snapshot replay. An additional general AArch64 lane in that fork-only run did **not** qualify: its unrelated native-value `compare_functions` request exceeded the existing default 1,024 MCP input-element limit. - **Current upstream PR head:** [CI run 41367791962](https://github.com/morluto/rea/actions/runs/41367791962) has passed package build, documentation and **Source tests** at `d9bb66a`. Built-dependent test shards remain in progress at this checkpoint; the run is not fully green. The preceding `079d733` run passed package build, documentation and source tests; one built test failed because the new legacy-snapshot regression passed an unprojected `{ok,value}` to `parseEvidence`. That regression now uses the existing `projectCliAnalysisResult` helper for both current and replayed results. Static checks on the previous head listed only 16 format issues also present on upstream `main`, none in these changed files. The final 27-file tree shares its implementation and verification-code blobs with the positively tested fork revision, except for the updated documentation. The temporary fork-only GitHub Actions trigger is **not** part of this PR. The exact merge commit has not been rerun through the optional real-Ghidra lane. Earlier historical results in this thread apply to their original revisions, not automatically to the current head. **Not established:** live PSP/Allegrex or Monster Hunter runtime correctness, arbitrary module/PRX admission, true overlay residency, Windows/macOS equivalence, or the complete public CLI/MCP path for this source-owned typed-warning probe. AVR testing covers the Ghidra-producing boundary, not AVR admission. ## Author readiness Human owner: @jaelliot - [ ] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [ ] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [ ] I declare this PR ready for initial maintainer review. Prepared with AI assistance from ChatGPT/Codex. This draft remains dependent on #2977 and human review. ## Before merge - [ ] #2977's canonical integration and dependency relationship are accepted upstream; reduce the inherited diff after that merge. - [ ] Review final scope, source attribution and relevant tests; confirm no unsupported authority, unrelated generated files, or overstated provider claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-11-01 21:43:00 +00:00

bsakaguchi changed title from ~~[PR #5137] fix(ghidra): attribute indirect-branch warnings by source address~~ to [PR #5137] [CLOSED] fix(ghidra): attribute indirect-branch warnings by source address

2026-11-04 01:57:42 +00:00

bsakaguchi closed this issue

2026-11-04 01:57:42 +00:00

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#6783

No description provided.

Rows
Columns