This is an archive of the discontinued LLVM Phabricator instance.

Add an c++ itanium demangler to llvm
ClosedPublic

Authored by rafael on May 9 2016, 3:13 PM.

Details

Summary

This adds a copy of the demangler in libcxxabi.

The code also has no dependencies on anything else in LLVM. To enforce that I added it as another library. That way a BUILD_SHARED_LIBS will fail if anyone adds an use of StringRef for example.

The no llvm dependency combined with the fact that this has to build on linux, OS X and Windows required a few changes to the code. In particular:

  • No constexpr.
  • No alignas
  • The malloc_alloc was failing to bulid on OS X. Looks like it conflicted with the libcxx implementation of string.

So for now I disabled the custom allocators.

On OS X at least this library has only one global symbol: __ZN4llvm16itanium_demangleEPKcPcPmPi

My current plan is:

  • Commit something like this
  • Change lld to use it
  • Change lldb to use as the fallback
  • Add a few #ifdefs so that exactly the same file can be used in libcxxabi to export abi::__cxa_demangle.

Once the fast demangler in lldb can handle any names this implementation can be replaced with it and we will have the one true demangler.

Diff Detail

Event Timeline

rafael updated this revision to Diff 56641.May 9 2016, 3:13 PM
rafael retitled this revision from to Add an c++ itanium demangler to llvm.
rafael updated this object.
rafael added reviewers: dexonsmith, k8stone, majak.
rafael edited reviewers, added: majnemer; removed: majak.
rafael added a subscriber: llvm-commits.

getting phab to send email

davide added a subscriber: davide.May 9 2016, 5:47 PM

I haven't looked at the demangler in depth (and I assumed it to be
correct as it's used somewhere else).
Other than that, it looks good to me. Bonus point, I like the API.

emaste added a subscriber: emaste.May 9 2016, 6:03 PM

Overall approach sounds good to me

silvas added a subscriber: silvas.May 9 2016, 6:59 PM

Small nit.

include/llvm/Demangle/Demangle.h
14

Can you add some docs for the arguments and return.
Especially these questions jump out to me:

  • what is the return value? If we demangle into buf then wouldn't the demangled name be implicitly be at the beginning of buf?
  • what is status? Is it just 0/1?
rafael updated this revision to Diff 56704.May 10 2016, 6:16 AM

Updated to use a llvm style name and document the interface.

rafael updated this revision to Diff 56705.May 10 2016, 6:18 AM
silvas added inline comments.May 10 2016, 12:18 PM
include/llvm/Demangle/Demangle.h
17

typo: enought

kcc added a subscriber: kcc.May 11 2016, 5:14 PM
asl added a subscriber: asl.May 16 2016, 12:44 PM
grimar added a subscriber: grimar.Jul 14 2016, 9:17 AM
rafael updated this revision to Diff 69127.Aug 24 2016, 9:11 AM
rafael added a reviewer: mehdi_amini.

Update the version from libcxxabi to get the fixes to the asan failures.

mehdi_amini edited edge metadata.Aug 24 2016, 9:17 AM

I know that D.Majnemer is working on a new demangler for LLVM, I don't what is the status on it though. If there is nothing on sight in the near future and you need something for lld, starting with this sounds like a reasonable plan to me.

What about also something like tools/llvm-c++filt/llvm-c++filt.cpp?

bcraig added a subscriber: bcraig.Aug 24 2016, 9:25 AM
bcraig added inline comments.
lib/Demangle/ItaniumDemangle.cpp
13–15

This bothers me a lot, for a couple of different reasons.

  1. I think there are already too many build time dependencies crossing the compiler / runtime boundary. I'm tired of my libcxx and libcxxabi builds failing because of unrelated cmake changes in LLVM. To avoid that, I could build a standalone version... but now that won't be easily possible because of this change. Building libcxx and libcxxabi shouldn't require me to pull down llvm sources, or to have a "dev" install of llvm, and this change makes that problem even worse than it already is. Will this even work for the cross compilation cases?
  1. This (and the current demangle implementation) are major ODR liabilities. This is being built with exceptions turned off. Client code can build with exceptions turned on. Now, one process has two copies of some vector and string methods, some built with exceptions on and some with exceptions off. This has actually bitten me in the past.
rafael updated this revision to Diff 69237.Aug 25 2016, 6:54 AM
rafael edited edge metadata.

Include a llvm-c++filt tool.

bcraig added inline comments.Aug 25 2016, 7:15 AM
tools/llvm-c++filt/llvm-c++filt.cpp
20 ↗(On Diff #69237)

You've got a leak. You need to free(Demangled).

Not a big issue, but some sanitizer test will flag this eventually.

rafael updated this revision to Diff 69251.Aug 25 2016, 8:14 AM

Fixed leak.
Added missing dependencies.
Renamed to llvm-cxxfilt to match llvm-cxxdump and avoid regex problems in lit.

Ping. Is this OK?

mehdi_amini accepted this revision.Sep 6 2016, 10:12 AM
mehdi_amini edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Sep 6 2016, 10:12 AM