[PATCH] D67301: [LLD] Unify the demangleItanium and demangleMSVC functions. NFC.

Mon Sep 9 15:53:36 PDT 2019

rnk added a comment.

In D67301#1662223 <https://reviews.llvm.org/D67301#1662223>, @mstorsjo wrote:

> In D67301#1661748 <https://reviews.llvm.org/D67301#1661748>, @rnk wrote:
>
> > > Should the _ prefix (for itanium symbols in i386 mode) be stripped by the COFF wrapper function, or by the common demangle function (matching _Z or __Z)?
> >
> > Maybe, I forget if the extra _ comes before the __imp_ prefix or after it when dllimported.
>
>
> It's after the `__imp_` prefix, so the demangler could handle it.
>
> > But there's another consideration, which is that this code could hypothetically be shared with a MachO linker implementation, where the extra underscore prefix is also present. IMO it's best for any generic demangler to match any number of underscores followed by Z.
>
> I'm not entirely decided on whether I think this is a good thing or not.
>
> The caller knows with certainty whether there should be an extra underscore prefix or not, as it is well defined (always in MachO, never in ELF, plus on i386 in COFF). But if we make the demangler implicitly handle both cases, it can also accidentally e.g. try to demangle any symbol which just starts with a capital Z, without any real underscore prefix, on i386 in COFF or in MachO. (I realize the current code in LLD I just committed also has this issue though.) So if we make it a requirement for the caller to compensate for the prefix, it is less convenient, but also less ambiguous.
>
> WDYT?

I would hope that demangling a non-C++ symbol that happens to start with Z would simply fail and return the original string. In practice, we have tools like `demumble` that work on text streams and look for any sequence of `_+Z${alphanum}+` and they work well enough. But I don't feel strongly, I see the argument for safety.

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D67301