[PATCH] D144049: [Symbolize][MinGW] Support demangling i386 call-conv-decorated C++ names

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:674
+  if (!IsVectorCall && (Front == '_' || Front == '@'))
+    SymbolName = SymbolName.drop_front();
 
----------------
alvinhochun wrote:
> mstorsjo wrote:
> > I see that the `demanglePE32ExternCFunc` function is restructured a fair bit here, but I kinda fail to see exactly what the intent is - is this a functional change, and in what cases does it change the output/behaviour?
> It fixes some unwanted stripping for vectorcall names. Notice that the vectorcall mangling does not add either an `_` or `@` prefix. The old code always tries to strip the prefix first, which for Itanium mangled names in vectorcall, the leading underscore of the Itanium name gets stripped instead, which breaks the Itanium demangler.
Oh, I see - thanks!

I think it'd be good to call out this explicitly in the commit message, as it wasn't obvious to me on the first read through. Now after you've pointed it out, it's quite obvious though :-)


================
Comment at: llvm/test/DebugInfo/symbolize-demangling-mingw32.s:21
+  .endef
+baz:
+  nop
----------------
alvinhochun wrote:
> mstorsjo wrote:
> > Hmm, both this symbol and `g` above, aren't technically valid manglings on i386 at all (although they can be considered reasonable internal symbols anyway, where tools/users could use any names they want).
> > 
> > Would it be useful to add a test for a plain `__cdecl` function with C mangling, i.e. just a plain underscore prefix?
> True, I agree it should test a mangled __cdecl C name, and I might as well also add a test each for the other calling conventions.
Yeah, plain C symbols with all calling conventions would be good too - thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144049