Skip to content

Fix Ghidra comment read fidelity#199

Merged
mahaloz merged 6 commits into
binsync:mainfrom
amattas:pr/ghidra-comment-read-fidelity
Jul 15, 2026
Merged

Fix Ghidra comment read fidelity#199
mahaloz merged 6 commits into
binsync:mainfrom
amattas:pr/ghidra-comment-read-fidelity

Conversation

@amattas

@amattas amattas commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Problem

Ghidra stores several comment slots for each code unit: plate, pre, EOL, post, and repeatable comments. The current declib Ghidra reader only looks at a subset of those slots and collapses multiple comments at the same address by overwriting earlier text. In practice this loses comments when round-tripping state from Ghidra into clients such as BinSync.

Two concrete failure modes this fixes:

  • EOL and PRE comments at the same address are not both preserved.
  • PLATE, POST, and REPEATABLE comments are not represented when reading comments back from Ghidra.

Solution

  • Read all populated Ghidra comment slots in Ghidra display order.
  • Preserve multiple comments at one address by joining them with explicit slot labels.
  • Set decompiled=True only when the merged comment is purely pseudocode/PRE text; mixed disassembly+pseudocode comments remain disassembly-side comments.
  • Extend the existing decompiler tests with eight parameterized subTest cases covering every singleton slot, empty input, mixed classification, and full display ordering.
  • Exercise all cases in one multi-code-unit traversal and preserve addr/func_addr assertions.

Verification

  • TEST_BINARIES_DIR=/tmp/declib-empty-test-binaries python -m pytest tests/test_decompilers.py::TestHeadlessInterfaces::test_ghidra_comment_slots -q -> 1 passed, 8 subtests passed
  • PYTHONPYCACHEPREFIX=/tmp/declib-pr199-pyc python -m compileall -q declib tests
  • git diff --check
  • Mutation checks confirmed failures for incorrect func_addr, last-slot-only classification, missing empty-comment filtering, first-code-unit-only traversal, and missing Java-string conversion.

Notes

This PR is independent of the BinSync PRs. It only changes how declib reads comments from Ghidra. The focused test uses an empty TEST_BINARIES_DIR only to satisfy the existing module-level fixture-directory check; the test itself uses mocks and does not consume a binary fixture.

@amattas

amattas commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Updated the test layout as requested: removed the new tests/test_ghidra_comments.py file and consolidated its coverage into parameterized subTest cases in the existing tests/test_decompilers.py. The focused test now reports 1 test and 4 passing subtests.

@amattas

amattas commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up test hardening pushed. The existing-file test now has 8 subTest cases covering empty input, all five singleton slots, mixed PLATE/PRE classification, and all-slot ordering in one multi-code-unit traversal. It also restores addr/func_addr assertions and verifies the exact Ghidra traversal calls. Mutation checks killed the previously surviving regressions.

@mahaloz

mahaloz commented Jul 15, 2026

Copy link
Copy Markdown
Member

@amattas please rebase on main (your other PR), so the conflicts get resolved

amattas added 5 commits July 14, 2026 21:34
_comments() read only EOL and PRE comments and let PRE silently
overwrite EOL at the same address, and never read PLATE comments -- so
function-level comments written by _set_comment could not be read back
out. Read all five Ghidra comment types per code unit and join the
populated ones, tagging entry-point PLATE comments with func_addr so
they round-trip through _set_comment. Iterate functions directly for
func_addr context and drop the now-unused __function_code_units helper.
@amattas amattas force-pushed the pr/ghidra-comment-read-fidelity branch from afe3cc0 to cfaebdf Compare July 15, 2026 01:37
@amattas

amattas commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@mahaloz Donezo on the rebase.

@mahaloz mahaloz merged commit 36123cd into binsync:main Jul 15, 2026
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants