Link DWARF types only when prototypes match ##analysis - #26455
Conversation
|
@0verflowme thats a followup from your previous pr. @phix33 feedback is welcome |
|
I think it needs a few tweaks: When a DWARF signature is rejected because its parsed shape differs, the imported type remains in the global type database. Even though Also, a mismatching prototype can receive a new suffixed type name on every The new tests cover successful matches and user overrides, but not the mismatch path. Suggestion is to add a negative regression test confirming that:
|
0verflowme
left a comment
There was a problem hiding this comment.
I agree with direction, it matches with what we’ve been building.
I’d add only these correctness pieces also to close this PR’s invariant
- Reuse
fcn.<id>.typed_namewhen the DWARF function identity is unchanged, regardless of whether its current type matches. Matching should control only fcnlink publication. This prevents _2, _3, etc. after repeatedaaa - If a newly imported signature is normalized into a mismatch, do not leave it available to function-name fallback:
either delete that newly created signature; or
delete it and recreate the exact DWARF shape through the existing SDB fallback. - Revoke a stale
fcnlink.<addr>when reanalysis proves that its referenced type no longer matches. Merely refusing to write a new link can leave an old, now-invalid link active. - Perform the final prototype comparison after all importing/fallback work. Only this final stored representation may receive
fcnlink
| @@ -1751,7 +1759,9 @@ static void import_dwarf_function_type(Context *ctx, const char *sname, const ch | |||
| (void)import_dwarf_function_fallback (anal, typed_name, ret_type, variables, has_unspecified_parameters); | |||
There was a problem hiding this comment.
Withholding fcnlink does not prevent the rejected imported type from remaining in sdb_types. The name-based afs fallback can still display it
| const char *previous_name = sdb_const_getf (ctx->sdb, NULL, "fcn.%s.name", sname); | ||
| const char *previous = sdb_const_getf (ctx->sdb, NULL, "fcn.%s.typed_name", sname); | ||
| if (previous_name && !strcmp (previous_name, dwarf_fcn->name) | ||
| && previous && dwarf_function_type_matches (types, previous, |
There was a problem hiding this comment.
the previous typed_name is reused only when it matches. A persistent mismatch therefore creates foo_<addr>, foo_<addr>_2, etc. after repeated aaa
| CMDS=<<EOF | ||
| e asm.dwarf=false | ||
| aaa | ||
| k anal/types/fcnlink.00001169 |
There was a problem hiding this comment.
There is no negative mismatch regression proving link absence, safe afs fallback, stable naming, and no type accumulation
|
updated @phix33 @0verflowme |
0verflowme
left a comment
There was a problem hiding this comment.
One edge case still seems open: if fcnlink.<addr> points to an older differently named DWARF type and the new typed_name matches, we keep the stale link; since fcnlink.* is DWARF-owned, should we replace it with the current matching type and add a small regression test?
There's also:
|
|
Ok to merge now? |
Always overwrite fcnlink with the current typed name once type_matches is true, instead of only reusing it when it already equals the old value. A persistent mismatch previously kept re-minting suffixed names (foo_<addr>, foo_<addr>_2, ...) on every aaa; now the link tracks the latest matching type.
|
tests |
PR 26455: Link DWARF types only when prototypes match
What it does (one line)
Only install the
fcnlink.<addr>address link when the type stored insdb_typesactually has the same shape (return type, arg count, arg types,variadic) as the prototype DWARF describes. Before, the link was installed as
soon as the type existed, even if
r_anal_import_c_declshad normalized thedeclaration into something else.
Why
The C importer can rewrite declarations while importing (known case:
maingets conventional parameters appended; typedef resolution can also change the
spelling). With the old code,
afswould then authoritatively show theimporter's shape as if DWARF said so. This PR makes the link fail-closed:
no exact match, no link.
Behavior change
They fall back to the old name-based signature lookup, i.e. exactly the
pre-fcnlink behavior. Nothing is deleted, no metadata is lost.
verbatim, so they always match and always keep their links.
Pros
only known path that could install a wrong one.
dwarf_function_type_matcheshelper; +26/-11 lines, nonew APIs, no format changes, no extra sdb walk when the match was already
verified during name selection.
main(collides with libc main from the types db,gets suffixed to
main_1169, links with exactint main_1169 ();) and theC++ ctor (
Bird * const thissurvives the C parser round-trip and stayslinked).
Cons / what you might not want
strcmp: a type the importer spellsdifferently but equivalently (qualifier order, resolved typedef) also loses
its link. Conservative by design; if this turns out common, the fix is
canonicalizing both sides through one serializer, not loosening the compare.
show my dwarf signature" needs debugging. A one-line R_LOG_DEBUG would help.
address suffix, so
afsshowsmain_1169instead ofmain.How to test
Expect
int main_1169 ();,void Bird_Bird_ (Bird * const this);and thefcnlink key present. Then check any binary with DWARF where the C importer
rewrites a prototype: the address must have NO fcnlink key and afs must show
the legacy signature, never the importer's rewritten one attributed to DWARF.
Verdict
Safe to merge: strictly reduces the cases where a link is installed, never
produces new output, and every skipped case degrades to pre-26451 behavior.