Skip to content

Fix C++ function identity collisions in the benchmark pipeline - #91

Open
sjkim1127 wants to merge 8 commits into
Noelo-Lab:mainfrom
sjkim1127:fix/cpp-function-identity-clean
Open

sjkim1127 wants to merge 8 commits into
Noelo-Lab:mainfrom
sjkim1127:fix/cpp-function-identity-clean

Conversation

@sjkim1127

@sjkim1127 sjkim1127 commented Sep 17, 2026 •

Copy link
Copy Markdown

[AUTOMATED]

Supersedes #90. This contribution keeps binary-local function entry addresses
through the benchmark pipeline, so C++ overloads and same-named functions do not
collapse into a single short-name record. Native PDB/Mach-O transport and the
experimental C++ evaluator from the closed #103 are not included.

Identity contract

  • Keep the semantic name in FunctionDecompilation.name. Use name@0xADDRESS
    only as a collision-qualified storage key; unique targets keep plain keys.
  • Normalize backend addresses to DWARF low_pc, including supported PE RVA and
    ARM/Thumb forms. Retain the full target universe when selecting a subset.
  • Carry identity through raw backends, materialized artifacts, TypeMatch,
    ByteMatch context, source display, report/rebuild and re-evaluation readers.
  • Reject ICF zero-address sentinels only with ELF non-executable-section evidence;
    legitimate executable address-zero code remains admissible.
  • Abstain where the metric cannot prove which same-named function it is scoring.

Review follow-up

The validation concern is correct: the historical ci/pr91b-validation jobs
checked out 415f9750, not PR head 8eeceb8. The previous statement that those
jobs were green "on this head" was inaccurate. Those runs are historical design
and toolchain evidence only, not current merge evidence.

This revision integrates main ae070b8 and adds validation to this PR branch:
.github/workflows/function-identity.yml explicitly checks out the submitted
head, records its SHA/tree, runs compiler fixtures and a full-suite differential,
and uploads the evidence. The workflow must run successfully on the submitted
revision before merge; local results do not satisfy that remote gate.

Code corrections in the follow-up:

  1. Benchmark selection and eval-kit export and ingest share a resolver.
    Qualified requests select the exact native address; ambiguous legacy plain
    requests select all matching addresses. Missing results use canonical keys,
    without adding a phantom failed plain-name alias.
  2. Source display preserves a known canonical address for plain keys as well as
    collision-qualified ones. Disk rebuilds preserve marker addresses, and
    conflicting producer addresses do not select an arbitrary source.
  3. successful_count counts actual nonempty returned records rather than
    subtracting absent requested functions from the returned population.
  4. Legacy offline GED refuses collision-qualified artifacts before its
    name-based CFG lookup. It does not claim to implement general C++ CFG identity.
  5. One existing identity test fixture now supplies the address-first ground-truth
    cache shape used by current main. This changes the fixture, not TypeMatch policy.

Local validation

Local candidate review tree: d4638a8ebfe46fde9b3f1ccdafcd39e4d9e0228c.
These are unsubmitted-tree measurements recorded during review preparation,
not a claim about a remotely validated commit.

Check Local evidence
Focused identity/consumer suite 149 passed, 10 skipped
Pinned main full pytest 801 passed, 13 failed, 23 skipped
Candidate full pytest 875 passed, 13 failed, 24 skipped
JUnit identity differential No new failure identities; no lost passing identities
MinGW x86-64 PE/DWARF Passed compiler-generated RVA-to-entry probe
LLD ICF Passed folded-alias and executable-address-zero tests
ARM/Thumb Passed a separate GCC 14.2 Linux-musl cross-compiler probe; the arm-none-eabi tests are locally skipped and remain a remote gate
Real stripped LevelDB angr 9.2.213 and Ghidra 12.0.4 agree on both collision keys and singleton slice identity
Scoped static checks Ruff/Black passed; five mypy owners clean; workflow YAML/bash parsed

The 13 full-suite failure identities are identical to pinned main: five content
cases, five ELF-assuming Glaurung/Manifold fixtures, and three cache/angr/type
controls. The local host is macOS with explicit Linux-musl GCC shims for ELF
fixtures; some inherited tests still choose native Clang/Mach-O. We are not
calling the full suite green. A separate existing mypy error in
models/decompilation.py::to_toml also reproduces on main.

The LevelDB probe is compile/decompile only. Producers receive stripped bytes and
requested addresses, while DWARF is retained by the evaluator. Stripping is
checked for preserved executable sections and removal of debug evidence.

Reproduction and scope are in docs/validation/function-identity.md. Detailed
local patches, JUnit files, producer output and hashes are retained in Fission's
benchmark/artifacts/pr91-followup/2026-10-23/.

Compatibility and limits

  • Unmapped backend output is dropped from the target universe and recorded as
    missing. This can change results for a producer reporting unsupported addresses.
  • Exact-address DWARF range lookup refuses same-name fallback rather than reading
    another function's range. Not every inherited byte extraction path is repaired.
  • Current cache generations are type_match=19, byte_match=8, ged=5 after the
    main integration. No additional metric arithmetic change is introduced here.
  • Some CFG producers remain name-keyed. Qualified collisions must abstain until
    an address-aware source/output CFG contract is implemented.
  • One concrete address remains one target under ICF; distinguishing folded aliases
    is a separate policy decision.
  • Frozen publication memberships require canonical result keys. The follow-up
    verifies selection/export/ingest consistency, not the entire publication path
    for ambiguous legacy manifests.
  • No full published C corpus metric replay, ranking improvement, unchanged C score,
    execution parity, or general C++ support is claimed. No leaderboard, result
    overlay, snapshot, generated site or CHANGELOG is authored by this follow-up;
    upstream main's existing changes are only brought in through the merge.

sjkim1127 and others added 2 commits September 17, 2026 14:42
Results are stored under the unqualified function name. That is
unambiguous for the C corpus, but C++ overloads and same-named methods in
different scopes share a short name while being different functions, so
they collapsed into one entry. Fixing a single backend was not enough:
relabeling, metrics, source lookup, result artifacts and re-evaluation
could each lose the distinction again.

Identity now derives from the complete DWARF target set rather than from
whatever subset a backend recovered. A name that is unique in the binary
keeps its historical plain key; only an ambiguous name becomes
name@0x<canonical-low-pc>. Backend addresses are normalized to the
matched DWARF low_pc, so every consumer keys off the same primitive.

The map used to decide ambiguity is the binary-wide target set, kept
separate from the per-run selection. Selecting one member of an overload
group therefore does not rename the others, which makes storage identity
invariant under sample-set slicing.

Where an exact identity is unavailable the code abstains instead of
guessing: an ambiguous CFG is not scored, a collision-qualified function
does not borrow name-keyed ground truth, and it contributes no prototype
to another overload's compile context. Likewise a DWARF range lookup
given an address matches that address or fails, rather than falling back
to the first DIE sharing the short name.

DWARF low_pc == 0 sentinels left by linker ICF are rejected only when the
ELF section table proves address zero is not executable, so genuine
bare-metal code at zero is preserved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers the collisions the identity change exists for: DWARF extraction
that must not collapse overloads, slice invariance of canonical keys,
metric and CFG abstention for ambiguous names, address-keyed source
recovery, linker-ICF zero-address sentinels, ARM Thumb address
normalization, and end-to-end backend behaviour.

Tests needing a cross-compiler or linker (arm-none-eabi, ld.lld, mingw)
skip when it is absent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sjkim1127 added a commit to sjkim1127/Decbench_SampleSet that referenced this pull request Sep 17, 2026
The implementation branch was reopened as Noelo-Lab/decbench#91 on
fix/cpp-function-identity-clean, so the previous links to
fix/cpp-function-identity and to the single-commit candidate no longer
describe where the work lives.

Also states plainly that the validation table was measured against the
earlier candidate d76561f and not against the branch under review, which
carries roughly 990 lines more — the slice-invariance, end-to-end and ICF
test work landed after that commit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review found three places where the new storage-key format was written but
not read back, so the distinction this change exists to preserve was lost
again at the boundary.

pipeline/materialized.py rebuilt results with its own `(\S+)` marker regex
and put the marker's first field straight into FunctionDecompilation.name.
For a collision that made the name `same@0x1000`, and ByteMatch feeds that
field to function_bytes(), the compile fixup and object symbol lookup as a
real C identifier -- so re-evaluating a materialized C++ artifact fed it a
string that is not a function name. The same regex could not see a marker
whose DWARF name contains a space (`operator new`) at all. It now reads
through results_tree.split_functions() and recovers the semantic name with
parse_function_storage_key(), keeping the storage key only as the map key.

TypeMatch's address-keyed ground-truth walker visited only the compile
unit's direct children, while identity extraction walks the whole DIE tree.
A C++ subprogram inside a namespace or class scope was therefore named
`foo@0x...` correctly and then had no ground truth at that address, so it
dropped out of the metric silently. It now walks CU.iter_DIEs().

Three further copies of the marker pattern are folded into the shared one:
repair_decompiled_flags compares against FunctionData keys and so wants the
storage key, while reeval_ged and audit_historical_ged_iso compare against
Joern body names and historical score keys and so want the semantic half.

Tests cover same@0x1000 / same@0x2000 / `operator new` round-tripping, and
namespace-scoped overloads reaching the address-keyed ground truth. The
latter needs real ELF: `g++` exists on macOS but links Mach-O, which made
this module raise ELFError instead of skipping, so the guard now probes the
linker output. That also turns the pre-existing sibling test from a failure
into a skip off Linux.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sjkim1127

Copy link
Copy Markdown
Author

Review addressed in 6bbac5c. Summary for anyone picking this up:

The blocker was real. pipeline/materialized.py rebuilt results with its own (\S+) marker regex and put the marker's first field straight into FunctionDecompilation.name. For a collision that made the name same@0x1000, and ByteMatch hands that field to function_bytes(), the compile fixup and object symbol lookup as a real C identifier — so re-evaluating a materialized C++ artifact fed it a string that is not a function name. The same regex could not see operator new at all. It now reads through results_tree.split_functions() and recovers the semantic name with parse_function_storage_key(), keeping the storage key only as the map key.

TypeMatch's address-keyed walker visited only the compile unit's direct children while identity extraction walks the whole DIE tree, so a subprogram inside a namespace or class scope was named foo@0x... correctly and then had no ground truth at that address — dropping out of the metric silently. Now CU.iter_DIEs(), with a real g++ -O2 -g namespace fixture asserting both overload addresses reach the map.

The remaining marker parsers are folded into the shared pattern, each in the form its call site actually needs: repair_decompiled_flags compares against FunctionData keys and takes the storage key, while reeval_ged and audit_historical_ged_iso compare against Joern body names and historical score keys and take the semantic half.

Validation on this head: all jobs green, and the differential reports main 5 failed / 657 passed against this branch 5 failed / 704 passed, no new failures relative to main. The five are pre-existing tests/test_content.py cases.

One thing worth flagging beyond the review: the ICF probe turned out to contradict the policy it was testing. It asserted that both folded aliases survive identity extraction, which cannot hold once the low_pc == 0 sentinel is filtered — identities are keyed by address, so a binary folding foo and bar onto one address yields exactly one. Both names had only survived because the bogus zero-address entry kept a second alive. Corrected to match the rule its own remaining assertions already encode, it passes. Whether folded aliases should be distinguishable is still open and would need a second identity dimension.

[AUTOMATED]

Integrate upstream main ae070b8 and share manifest population selection across
benchmark/export/ingest. Preserve source-display addresses, refuse legacy GED
collision scoring, and count returned successes without phantom deductions.
Add regression coverage and exact-head validation workflow.

This branch has not been deployed

No deployments
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.

1 participant