[PATCH] D67301: [LLD] Use the unified llvm demangle frontend function. NFC.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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

> In D67301#1681906 <https://reviews.llvm.org/D67301#1681906>, @MaskRay wrote:
>
> > In D67301#1680905 <https://reviews.llvm.org/D67301#1680905>, @rnk wrote:
> >
> > > In D67301#1665612 <https://reviews.llvm.org/D67301#1665612>, @MaskRay wrote:
> > >
> > > > We haven't even heard people complaining about lack of demangling of `__Z`, `___Z` or `___Z` prefixed names. I still lean towards keeping the `"_Z"` check for ELF to avoid false positive.
> > >
> > >
> > > Weighing this desire to not change functionality against the cost of adding an extra boolean default argument to control the behavior, I'd prefer to change behavior to be consistent everywhere.
> > >
> > > I guess we can't predict which bug is more likely: failing to demangle something the user wanted us to demangle, or unintentionally demangling a real C symbol starting with `Z`. Regardless of which bug appears down the road, I favor the solution that takes less code and makes LLVM tools behave consistently.
> >
> >
> > Is there a scenario where both Itanium and Microsoft mangling schemes should be tried? (mingw?)
>
>
> No, practically, only one of them should be possible at a given time. But I don't see why that is relevant to the discussion at hand.


If a call site of llvm::demangle deals either exclusively Itanium mangled names, or exclusively MSVC-style mangled names. I don't see why we must have a single entry point (`llvm::demangle`) for demangling. Two entry points (for Itanium and MSVC-style) may be better. It avoids the problem that some platforms are unnecessarily affected by special rules from some particular platforms.

>> If no, separate entry points for itaniumDemangle and microsoftDemangle may be better.
> 
> The distinction between itanium and microsoft isn't an issue here, at all.
> 
> They have strictly very different mangling; microsoft mangled names start with a `?` while itanium mangled ones start with `_Z`. There is no risk of mixing them up at all.
> 
> On i386, all symbols (except for microsoft mangled and certain other calling conventions) start with an extra underscore, so itanium mangled names start with `__Z`. This is currently handled by always manually stripping one underscore on i386 before calling the demangler, but could be skipped as well if we use a more tolerant demangler. It could also be kept to avoid the risk of demangling a symbol like `Znwy` that originally didn't start with an underscore.
> 
>> I did not expect the solution to end up with an extra parameter `StrictPrefix`. The additional complexity was not what I intended. If mingw has to live with both mangling schemes, I want to first check if the demangling code will be used in more than one places. If yes, I am not against deleting StrictPrefix. The overhead to demangle `__Z`, `___Z` or `___Z` prefixed names for ELF should be small.
> 
> It's not at all about the two different mangling schemes.
> 
> It's only about whether we artifically should make the general itanium demangler routine more strict than it is right now, or accept that there's a small risk of demangling something that isn't supposed to, but this is already a risk that all existing callers of the general llvm demangle library are ok with.

First, I'd prefer a solution that does not need to annotate call sites like: `demangle(demangleInput, /*strictPrefix=*/true)`. This is prerequisite.

Next, if there is some way to not demangle `__Z`, `___Z` or `___Z` prefixed names for ELF, that will be great.. If mingw or apple may need more underscores, a new function will look good to me. That will document their extensions to the Itanium mangling scheme. However, if doing this would increase code complexity significantly, I'm fine with using a unified interface.


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

https://reviews.llvm.org/D67301