fix(labels): report demangled Rust names with the spacing rustc uses - #249
Open
r0ny123 wants to merge 1 commit into
Open
fix(labels): report demangled Rust names with the spacing rustc uses#249r0ny123 wants to merge 1 commit into
r0ny123 wants to merge 1 commit into
Conversation
Demangled names were passed through a port of Ghidra's CondensedString before being stored, so <Error as core::fmt::Display>::fmt reached the report as <Error_as_core::fmt::Display>::fmt and A, B became A,B. Ghidra needs that because a Ghidra symbol name cannot contain spaces; SMDA has no such constraint, and already stores names with spaces from another source -- 39 of the 399 names recovered from a PDB on a rust-lld x64 image contain them, because MSVC proc records spell them that way. The condensation was applying one spelling rule to names from a symbol table and another to names from a PDB, in the same report. Removing it exposed a printer bug it had been masking: a function pointer's ABI ran into its fn, giving unsafe extern "C"fn(*mut u8). The closing quote was emitted without the trailing space, and comparisons against the reference implementation could not see it because the condensation deleted that space from both sides. One existing test asserted the wrong spelling and is corrected. Measured against rustc-demangle 0.1.28 on 147 real symbols read out of a rust-lld x64 image: 93 of 147 matched byte for byte before, 146 after dropping the condensation, and 147 after the ABI fix. Cross-checked against the 18 mangled symbols in the reference implementation's own test corpus: 8 match, 9 raise and so keep the name the binary gave them, and 1 differs -- a punycode identifier this port renders as a placeholder, which is pre-existing and left alone here.
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
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.
Base of a linear stack: #249 → #250 → #254 → #255. #254 also touches
RustSymbolProvider, and the two do not merge cleanly in either order unless stacked — resolving that conflict in #254's favour would leave an import of the module this PR deletes.Demangled Rust symbols are passed through
remove_bad_spaces()before being stored, which rewrites<std::io::error::Error as core::fmt::Display>::fmtinto<std::io::error::Error_as_core::fmt::Display>::fmt. This stops doing that, and fixes a demangler bug the condensation was hiding.Why the condensation is wrong here
It is a port of Ghidra's
CondensedString. Ghidra needs it because a Ghidra symbol name cannot contain spaces. SMDA has no such constraint — names go into a JSON report — and the helper's own comments read as uncertainty rather than intent ("But typically Rust types don't have spaces inside unless it'swhere T: ...? Actually Ghidra converts it to underscore if surrounded by chars.").The decisive evidence is that SMDA already stores names with spaces from another provider. On a rust-lld/MSVC x64 image, 39 of the 399 function names recovered from the PDB contain spaces, because MSVC proc records spell them that way:
So the condensation is not enforcing a system-wide invariant. It applies one spelling rule to Rust names recovered from a symbol table and a different one to names recovered from a PDB, in the same report.
The bug it was hiding
With the condensation removed, a function pointer's ABI ran into its
fn:The printer emitted the closing quote without the trailing space that
rustc-demangleemits (this.print("\" ")). It was invisible while the condensation was in place, because that pass deleted the space from inside(...)anyway — including from the reference output it was being compared against. One existing test asserted the wrong spelling and is corrected here.What changes
Foo as Barstops becomingFoo_as_BarandA, Bstops becomingA,Binside<...>and(...); an ABI string is separated from itsfn. The same names are recognised and the same names are left alone.Validation
Measured against
rustc-demangle0.1.28 on 147 real_Rsymbols read out of a rust-lld/MSVC x64 image:Cross-checked separately against the 18 mangled symbols in the reference implementation's own test corpus: 8 match, 9 raise
UnableTov0Demangleand so keep the name the binary gave them, and 1 differs.The 9 are the
#[splat]grammar, which this port does not implement; raising is the safe outcome, since the caller then stores the mangled name unchanged rather than a wrong one.The 1 difference is untouched here — a punycode identifier renders as
utf8_idents::punycode{__-7hkackfecea1cbdathfdh9hlq6y}where the reference decodes it toutf8_idents::საჭმელად_გემრიელი_სადილი. It is a different concern from spacing and is fixed separately in #252, which takes the corpus to 9 exact and 0 disagreements.Compatibility
This changes reported symbol names for Rust binaries, so it is a visible output change rather than an internal cleanup. It affects only names the demangler rewrote. Nothing hashes or indexes on a demangled name — the PIC and escaped-instruction hashes are computed from instruction bytes — so similarity and report identity are unaffected.