This is an archive of the discontinued LLVM Phabricator instance.

cxa_demangle: make demangler's parsing functions overridable
ClosedPublic

Authored by labath on Oct 8 2018, 10:20 AM.

Details

Summary

This uses CRTP (for performance reasons) to allow a user the override
demangler functions to implement custom parsing logic. The motivation
for this is LLDB, which needs to occasionaly modify the mangled names.
One such instance is already implemented via the TypeCallback member,
but this is very specific functionality which does not help with any
other use case. Currently we have a use case for modifying the
constructor flavours, which would require adding another callback. This
approach does not scale.

With CRTP, the user (LLDB) can override any function it needs without
any special support from the demangler library. After LLDB is ported to
use this instead of the TypeCallback mechanism, the callback can be
removed.

More context can be found in D50599.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Oct 8 2018, 10:20 AM
rsmith added inline comments.Oct 11 2018, 4:12 PM
src/demangle/ItaniumDemangle.h
2140–2211 ↗(On Diff #168679)

I'd like for us to be moving towards making this particular allocator implementation be libc++abi-specific (LLVM's use of the demangler code doesn't want it, for instance, and I suspect it's not really what LLDB wants either). This change seems to entrench the use of this allocator by giving Db it as a default argument, which seems like a step backwards. I also think this change is orthogonal to the rest of the patch; we don't need these classes to be in the header to convert Db to a CRTP base class, right?

2214 ↗(On Diff #168679)

If you're going to do a mass rename on this class anyway, have you considered giving it a more reasonable name (maybe something like ManglingParser)?

Thanks for doing this!

src/demangle/ItaniumDemangle.h
2214 ↗(On Diff #168679)

Yep, I agree. Db is a really horrible name for this.

2246 ↗(On Diff #168679)

I think libcxxabi's clang-format moved all the pointers to the left. I just committed r344316, which overrides libcxxabi's behaviour for this directory. Can you re-run this through clang-format-diff?

labath added inline comments.Oct 15 2018, 12:03 PM
src/demangle/ItaniumDemangle.h
2140–2211 ↗(On Diff #168679)

You're right, they are orthogonal. I needed an allocator since I wanted to write a test for this (in LLVM), and I didn't want to reimplement an allocator there. Later, I realised that I can just use allocator from LLVMSupport instead and that this will not cause a dependency cycle, since it's test-only, but i didn't get around to updating the test (AFK).

I'll revert this part in the next update.

2214 ↗(On Diff #168679)

ManglingParser sounds good.

2246 ↗(On Diff #168679)

Will do.

labath updated this revision to Diff 169737.Oct 15 2018, 12:22 PM
  • rename Db into (Abstract)ManglingParser
  • revert Allocator changes
  • re-clang-format
labath marked 7 inline comments as done.Oct 15 2018, 12:23 PM
erik.pilkington accepted this revision.Oct 15 2018, 1:58 PM

LGTM, thanks again! Are you planning on updating itaniumFindTypesInMangledName to use this too? (If not, I'll get around to it eventually!)

This revision is now accepted and ready to land.Oct 15 2018, 1:58 PM

Thanks for the review.

Are you planning on updating itaniumFindTypesInMangledName to use this too? (If not, I'll get around to it eventually!)

My plan was to implement the equivalent to itaniumFindTypesInMangledName within LLDB and then delete the LLVM copy together with the callback hook (this seems like a very specialized piece of functionality for such a low-level library). However, if you prefer keeping this in libDemangle, I can do that too.

This revision was automatically updated to reflect the committed changes.