[PATCH] D110664: [Symbolize] Demangle Rust symbols

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:657-675
   if (Name.substr(0, 2) == "_Z") {
     int status = 0;
     char *DemangledName =
         itaniumDemangle(Name.c_str(), nullptr, nullptr, &status);
     if (status != 0)
       return Name;
     std::string Result = DemangledName;
----------------
tmiasko wrote:
> dblaikie wrote:
> > tmiasko wrote:
> > > dblaikie wrote:
> > > > Can/should we make this somehow a generic piece of code that could be used here and in llvm-cxxfilt, so it's only written in one place? (eg: there's patches to add D demangling going in at the moment & so would be handy if adding support only had to go in one place rather than 2 or more)
> > > > 
> > > > Speaking of the D demangling support - I was hoping it could be broken down into pieces like the Rust work you did (really appreciate that, btw) - I thought the original Rust proposal was a fully featured single patch that then got broken down, am I remembering that correctly? I couldn't find the original single patch version - if you happen to have a pointer to it, I'd appreciate it for comparison/understanding of how things went there.
> > > Reusing the llvm::demangle might be an option, but looking back in discussion from D68133, so far code here was intentionally using microsoftDemangle directly with different options. Maybe at least llvm-cxxfilt could use llvm::demangle? But then again llvm-cxxfilt, its not demangling MSVC symbols right now, and I don't have particular insight whether it should and if so how exactly.
> > > 
> > > The single patch version of Rust demangling support was in D99981.
> > seems OK to me to use a generic demangling for all these tools - but at least llvm-cxxfilt and symbolize here are currently both doing everything except MSVC mangling - so maybe some "unix-y demangle" function would abstract these two callers at least.
> > 
> > Cross referencing to: D110577 - that patch and this one are both adding a case to two places that seem like they should be kept in sync, and I'd like to see that done probably by this patch, so that 110577 doesn't have to touch two places.
> The function here is handling MSVC mangling, though. Hence, my comment about an earlier attempt to use `llvm::demangle` in this function.
ah, sorry, right, misread. I think it'd be fine/perhaps helpful to add llvm-cxxfilt support for MSVC demangling too (though maybe it's too ambiguous for a general "filter any string starting with '?' to try to demangle it") - but if not, at least the first part of LLVMSymbolizer::DemangleName could defer to a function that wraps up the itanium-ish demanglings over C++, Rust, and pending D support. Returning some "it wasn't mangled in any way we recognize" value, which in this caller would then fallback to the MSVC case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110664/new/

https://reviews.llvm.org/D110664