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

[Alvin Wong via Phabricator via llvm-commits]

alvinhochun added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:674
+  if (!IsVectorCall && (Front == '_' || Front == '@'))
+    SymbolName = SymbolName.drop_front();
 
----------------
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.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:706
+    // may also be applied on top of the Itanium or Rust name mangling.
+    if (nonMicrosoftDemangle(DemangledCName.c_str(), Result))
+      return Result;
----------------
mstorsjo wrote:
> This part of the change makes sense!
> 
> I'm a bit undecided about whether the current flow in the function is the best/ideal/most correct one etc, but it probably works just right for all practical cases.
> 
> (For macho/darwin, where is the leading underscore stripped out?)
> For macho/darwin, where is the leading underscore stripped out?

Looks like it happens way earlier when loading symbols into `SymbolizeObjectFile`: https://github.com/llvm/llvm-project/blob/462227f1150fc4a12f95a7a101de477c06d35ba7/llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp#L212-L214


================
Comment at: llvm/test/DebugInfo/symbolize-demangling-mingw32.s:21
+  .endef
+baz:
+  nop
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144049