Page MenuHomePhabricator

Add profiling and canonicalization support to demangler nodes. No functionality change intended.
AbandonedPublic

Authored by rsmith on Aug 15 2018, 5:40 PM.

Details

Summary

Adds the ability to deduplicate demangler nodes as they're being created. I've
kept the new functionality behind #ifdefs to make it easier to keep this in
sync with the libc++abi version of this code. This is a prerequisite for
profile data remapping functionality; you can see the big picture here:
https://reviews.llvm.org/differential/diff/160950/

Diff Detail

Event Timeline

rsmith created this revision.Aug 15 2018, 5:40 PM

Thanks for fixing up some of the const inconsistencies! I agree with you that it'd be nice to move all the profiling stuff to a separate file once the AST moves to a header, but I'm fine with doing that in a follow up.

How are you handling the circular dependency problem? Support depends on Demangle, and with this patch Demangle would now depend on Support, right?

lib/Demangle/ItaniumDemangle.cpp
36

nit: this shouldn't be indented.

747

#ifndef STANDALONE_DEMANGLER

rsmith marked 2 inline comments as done.Aug 16 2018, 3:11 PM

How are you handling the circular dependency problem? Support depends on Demangle, and with this patch Demangle would now depend on Support, right?

Good question. So far I've not handled that problem :(

I think the only reasonable way to handle this is for Demangle to move into Support. We could keep the headers separate but move the implementation into lib/Support/Demangle, similar to what we do for AST. Or we could also move the headers to include/llvm/Support/Demangle. My preference is the latter; it seems reasonable to me for Demangle to generally live inside Support.

I think the only reasonable way to handle this is for Demangle to move into Support. We could keep the headers separate but move the implementation into lib/Support/Demangle, similar to what we do for AST. Or we could also move the headers to include/llvm/Support/Demangle. My preference is the latter; it seems reasonable to me for Demangle to generally live inside Support.

Sure, I really don't know why we didn't just do that to start with. The original RFC[1] talked about putting the demangler into Support, but the commit that materialized it (r280732) put Demangle into it's own library. Would be nice to get @zturner's sign-off for a move too, since he owns the microsoft demangler.

[1]: https://groups.google.com/forum/#!topic/llvm-dev/v_7OuWx8n1A

I think the only reasonable way to handle this is for Demangle to move into Support. We could keep the headers separate but move the implementation into lib/Support/Demangle, similar to what we do for AST. Or we could also move the headers to include/llvm/Support/Demangle. My preference is the latter; it seems reasonable to me for Demangle to generally live inside Support.

Sure, I really don't know why we didn't just do that to start with. The original RFC[1] talked about putting the demangler into Support, but the commit that materialized it (r280732) put Demangle into it's own library. Would be nice to get @zturner's sign-off for a move too, since he owns the microsoft demangler.

[1]: https://groups.google.com/forum/#!topic/llvm-dev/v_7OuWx8n1A

My understanding is that Demangle is used by other projects like compiler-rt which can't depend on Support. If I'm wrong, it would be great. For some reason I can't find it when I search the archive, but if you search your inbox for "llvm r328123" you'll find a long discussion thread with various people who were dealing with this and/or may know something about it. Anyway, I'm CC'ing them all here.

Personally if we can get Demangle into support I would be very happy.

I think the only reasonable way to handle this is for Demangle to move into Support. We could keep the headers separate but move the implementation into lib/Support/Demangle, similar to what we do for AST. Or we could also move the headers to include/llvm/Support/Demangle. My preference is the latter; it seems reasonable to me for Demangle to generally live inside Support.

Sure, I really don't know why we didn't just do that to start with. The original RFC[1] talked about putting the demangler into Support, but the commit that materialized it (r280732) put Demangle into it's own library. Would be nice to get @zturner's sign-off for a move too, since he owns the microsoft demangler.

[1]: https://groups.google.com/forum/#!topic/llvm-dev/v_7OuWx8n1A

My understanding is that Demangle is used by other projects like compiler-rt which can't depend on Support. If I'm wrong, it would be great. For some reason I can't find it when I search the archive, but if you search your inbox for "llvm r328123" you'll find a long discussion thread with various people who were dealing with this and/or may know something about it. Anyway, I'm CC'ing them all here.

Personally if we can get Demangle into support I would be very happy.

That's why all of this is behind an #ifdef? It seems to be trying to preserve the ability to build a stand-alone demangler that does not in fact pull in Support or anything else.

I think the only reasonable way to handle this is for Demangle to move into Support. We could keep the headers separate but move the implementation into lib/Support/Demangle, similar to what we do for AST. Or we could also move the headers to include/llvm/Support/Demangle. My preference is the latter; it seems reasonable to me for Demangle to generally live inside Support.

Sure, I really don't know why we didn't just do that to start with. The original RFC[1] talked about putting the demangler into Support, but the commit that materialized it (r280732) put Demangle into it's own library. Would be nice to get @zturner's sign-off for a move too, since he owns the microsoft demangler.

[1]: https://groups.google.com/forum/#!topic/llvm-dev/v_7OuWx8n1A

My understanding is that Demangle is used by other projects like compiler-rt which can't depend on Support. If I'm wrong, it would be great. For some reason I can't find it when I search the archive, but if you search your inbox for "llvm r328123" you'll find a long discussion thread with various people who were dealing with this and/or may know something about it. Anyway, I'm CC'ing them all here.

Personally if we can get Demangle into support I would be very happy.

That's why all of this is behind an #ifdef? It seems to be trying to preserve the ability to build a stand-alone demangler that does not in fact pull in Support or anything else.

The question is, who needs that? Can we just put it *into* Support?

I think the only reasonable way to handle this is for Demangle to move into Support. We could keep the headers separate but move the implementation into lib/Support/Demangle, similar to what we do for AST. Or we could also move the headers to include/llvm/Support/Demangle. My preference is the latter; it seems reasonable to me for Demangle to generally live inside Support.

Sure, I really don't know why we didn't just do that to start with. The original RFC[1] talked about putting the demangler into Support, but the commit that materialized it (r280732) put Demangle into it's own library. Would be nice to get @zturner's sign-off for a move too, since he owns the microsoft demangler.

[1]: https://groups.google.com/forum/#!topic/llvm-dev/v_7OuWx8n1A

My understanding is that Demangle is used by other projects like compiler-rt which can't depend on Support. If I'm wrong, it would be great. For some reason I can't find it when I search the archive, but if you search your inbox for "llvm r328123" you'll find a long discussion thread with various people who were dealing with this and/or may know something about it. Anyway, I'm CC'ing them all here.

Personally if we can get Demangle into support I would be very happy.

That's why all of this is behind an #ifdef? It seems to be trying to preserve the ability to build a stand-alone demangler that does not in fact pull in Support or anything else.

The question is, who needs that? Can we just put it *into* Support?

libc++abi is under a different license than support.

We can't fix this until at a minimum the relicensing is "done".

I think the only reasonable way to handle this is for Demangle to move into Support. We could keep the headers separate but move the implementation into lib/Support/Demangle, similar to what we do for AST. Or we could also move the headers to include/llvm/Support/Demangle. My preference is the latter; it seems reasonable to me for Demangle to generally live inside Support.

Sure, I really don't know why we didn't just do that to start with. The original RFC[1] talked about putting the demangler into Support, but the commit that materialized it (r280732) put Demangle into it's own library. Would be nice to get @zturner's sign-off for a move too, since he owns the microsoft demangler.

[1]: https://groups.google.com/forum/#!topic/llvm-dev/v_7OuWx8n1A

My understanding is that Demangle is used by other projects like compiler-rt which can't depend on Support. If I'm wrong, it would be great. For some reason I can't find it when I search the archive, but if you search your inbox for "llvm r328123" you'll find a long discussion thread with various people who were dealing with this and/or may know something about it. Anyway, I'm CC'ing them all here.

Personally if we can get Demangle into support I would be very happy.

That's why all of this is behind an #ifdef? It seems to be trying to preserve the ability to build a stand-alone demangler that does not in fact pull in Support or anything else.

The question is, who needs that? Can we just put it *into* Support?

libc++abi is under a different license than support.

We can't fix this until at a minimum the relicensing is "done".

But libc++abi doesn't include these headers. It makes a copy of them and checks them in. Does this solve that problem?

libc++abi is under a different license than support.

We can't fix this until at a minimum the relicensing is "done".

Wait why? Do lawyers really care if we put this stuff in LLVMDemangle instead of LLVMSupport??

But libc++abi doesn't include these headers. It makes a copy of them and checks them in. Does this solve that problem?

My understanding was that this macro would *allow* them to make that copy. They would copy the code, set the define, and not need a copy of Support.

We could of course insist that they do some kind of preprocessing of the source code as part of the copy, but not sure what that gains?

libc++abi is under a different license than support.

We can't fix this until at a minimum the relicensing is "done".

Wait why? Do lawyers really care if we put this stuff in LLVMDemangle instead of LLVMSupport??

No, they care that libc++abi doesn't include code from Support which is under a different license.

If you want to move the file from one directory to another (without changing the code), maybe we can do that. But I don't think that discussion really needs to happen befor ethis change can be reviewed?

Changes to move Demangle library under Support are here: https://reviews.llvm.org/D50875

I don't believe there is any compiler-rt / libc++abi impact: libc++abi has its own copy of the demangler that is notionally separately maintained but actually is kept in sync with the one in LLVM.

If we're not going to allow code from Support to be used in the demangler, then I don't think we should move it to support, because it will be too easy for people to mess up and use code they shouldn't.

Perhaps what we need are some hooks into the demangler (e.g. an interface defined in LLVMDemangle that exposes some virtual methods with default implementations), and then LLVM / Clang could override them to inject custom functionality.

If we're not going to allow code from Support to be used in the demangler, then I don't think we should move it to support, because it will be too easy for people to mess up and use code they shouldn't.

We could easily add a unittest (or something similar) that builds and uses the file in the standalone mode without the Support library to ensure that when it gets copied into libc++abi it still works. This should be possible to implement both with a single file in the Support library and for a separate library. And regardless of which design, seem like a more reliable way of preserving the invariant that we want here?

Perhaps what we need are some hooks into the demangler (e.g. an interface defined in LLVMDemangle that exposes some virtual methods with default implementations), and then LLVM / Clang could override them to inject custom functionality.

No strong feelings here, happy to let you and Richard discuss this aspect of the design.

zturner added a subscriber: rsmith.Aug 16 2018, 5:33 PM

Another option is to simply move it into support and say that going forward
merges to libcxxabi might get more difficult. Perhaps this isn’t so bad
though. Isn’t the itanium demangler mostly complete? If the potential for
future changes to it is low, maybe this is an acceptable tradeoff. Then we
could just move it to support, use it and evolve it any way we want with no
restrictions, and when new functionality gets added to the itanium mangler
that needs to be port to libcxxabi, it’s a little more effort than a simple
copy paste because you have to think about it some.

Thoughts?

Another option is to simply move it into support and say that going forward
merges to libcxxabi might get more difficult. Perhaps this isn’t so bad
though. Isn’t the itanium demangler mostly complete?

I think the primary changes we want to share will be bugfixes. There have been a lot of those over the past few years. Not sure how many are left?

If the potential for
future changes to it is low, maybe this is an acceptable tradeoff. Then we
could just move it to support, use it and evolve it any way we want with no
restrictions, and when new functionality gets added to the itanium mangler
that needs to be port to libcxxabi, it’s a little more effort than a simple
copy paste because you have to think about it some.

Thoughts?

FWIW, I think this is mostly a question for the maintainers. I'm happy for them to somewhat dictate what works w.r.t. sharing the logic between the two domains given the (perhaps frustrating) constraints here.

Yea, I agree it’s mostly bugfixes. My hypothesis is that bugfixes are, on
average, pretty small and isolated, so it shouldn’t be too hard to port one
between the two even if they evolve separately. I’m curious what Erik
thinks though.

Another option is to simply move it into support and say that going forward
merges to libcxxabi might get more difficult. Perhaps this isn’t so bad
though. Isn’t the itanium demangler mostly complete? If the potential for
future changes to it is low, maybe this is an acceptable tradeoff.

It's pretty much complete, but there is still going to be a steady stream of updates. Each release of C++ is likely to add a handful more constructs that need manglings, and often new attributes/extensions get custom manglings that need to be handled as well.

Then we
could just move it to support, use it and evolve it any way we want with no
restrictions, and when new functionality gets added to the itanium mangler
that needs to be port to libcxxabi, it’s a little more effort than a simple
copy paste because you have to think about it some.

Thoughts?

-1 from me, large-scale refactorings such as this, or Pavel's plans here: https://reviews.llvm.org/D50599 would make updating/fixing bugs in the demangler a nightmare. I really don't want to have to implement every feature twice. Not to mention its just plain ugly to have two vaguely similar demanglers in the source tree.

But do you actually need those changes that pavel is working on? That looks
like a whole bunch of code that someone who just wants to print a demangled
name won’t care about. How involved are bugs usually, because I’m imagining
they’re a) usually just a couple lines change and b) pretty infrequent.
Once the relicense happens, you just change the include path and include
the real one.

I mean I agree it’s a bit ugly, but I don’t think it’s any more ugly than
moving it to support and hacking up the file with ifdefs so that it both is
and isn’t part of support at the same time. After all, we already have two
vaguely similar demanglers in the tree. One in llvm and one in libcxxabi.
So that doesn’t actually get any worse.

I pretty strongly dislike the alternative of moving demangler to support
and “lying” about it being part of support :-:

Another option is to simply move it into support and say that going forward
merges to libcxxabi might get more difficult. Perhaps this isn’t so bad
though. Isn’t the itanium demangler mostly complete? If the potential for
future changes to it is low, maybe this is an acceptable tradeoff.

It's pretty much complete, but there is still going to be a steady stream of updates. Each release of C++ is likely to add a handful more constructs that need manglings, and often new attributes/extensions get custom manglings that need to be handled as well.

Then we
could just move it to support, use it and evolve it any way we want with no
restrictions, and when new functionality gets added to the itanium mangler
that needs to be port to libcxxabi, it’s a little more effort than a simple
copy paste because you have to think about it some.

Thoughts?

-1 from me, large-scale refactorings such as this, or Pavel's plans here: https://reviews.llvm.org/D50599 would make updating/fixing bugs in the demangler a nightmare. I really don't want to have to implement every feature twice. Not to mention its just plain ugly to have two vaguely similar demanglers in the source tree.

This, combined with the comments on the move of the library IMO make it very clear what our two realistic choices are.

We can either make Demangle part of Support, and if we do so I think Zach is essentially right about how we do this (it'll move, the directory structure and other things will change), or we don't.

If we don't, we can't (even conditionally) use Support.

So the choices become:
a) We truly merge this into Support (and force manual porting of bugfixes between the two versions) and get all of the interesting enhancements we're talking about here such as what Richard is working on.
b) We don't merge this into Support, we keep it truly separate as it is today, and we do the very significant refactorings necessary to support these kinds of facilities with the layering as it is.

I'm worried that (b) will turn into "we don't get the fancy features at all" which worries me. But I'm seeing very little good compromise between (a) and (b). =/

I'm going to try to split this patch so it adds a generic visitation mechanism to the demangler AST, and introduce a separate layer that includes Support and builds profiling off the visitation mechanism. That seems like the only way out that meets everyone's acceptance criteria. However, I don't see any way to do that without moving all of the demangler to a public header in include/llvm (since the whole thing will need to be included from both the Demangle/ demangler and the profiling demangler). That header would only be included (and the templates in it instantiated) in two places in LLVM, but still, it'd be a 5000-line header file. If people are strongly opposed to that approach, please shout now!

Can’t we just use a base class with pure virtual methods, pass it to the
demangle function, implement this in clang, and pass null from libcxxabi?
Moving the whole thing to a header file seems kinda unfortunate.

I'm going to try to split this patch so it adds a generic visitation mechanism to the demangler AST, and introduce a separate layer that includes Support and builds profiling off the visitation mechanism. That seems like the only way out that meets everyone's acceptance criteria. However, I don't see any way to do that without moving all of the demangler to a public header in include/llvm (since the whole thing will need to be included from both the Demangle/ demangler and the profiling demangler). That header would only be included (and the templates in it instantiated) in two places in LLVM, but still, it'd be a 5000-line header file. If people are strongly opposed to that approach, please shout now!

+1 from me! I was just writing an argument to do something like this.

One more thing that I want to mention: If/when we go monorepo, then we can share code between libcxxabi and llvm the old-fashioned way. Tangling in support would just make it impossible to ever de-duplicate the demanglers.

Yea this approach is fine I guess, but I’m not really a fan of gigantic
headers, so if it’s possible to use runtime polymorphism that seems better
to me. Otoh, I’m not the maintainer of the itanium demangler, so feel free
to ignore :)