Committed in r348995. (I also took the liberty of adding a [Support] tag to the title and wrapping the commit message, in line with LLVM conventions; see https://llvm.org/docs/DeveloperPolicy.html#commit-messages). Thanks for the patch!

That's the intent ... it uploads the entire file to Phabricator, so that it can be viewed through the web interface. The arcanist tool does that for you automatically, as an alternative to have to copy and paste a giant block of text around.

This LGTM, but before I approve, I'd like to look around the surrounding code to confirm things are how I expect, but this patch is uploaded without context :( Could you reupload with context? See https://llvm.org/docs/Phabricator.html (the part about using -U999999 in your git commands to generate full context).

https://reviews.llvm.org/D55617

I started playing with this:

https://reviews.llvm.org/D55543 does the same thing

I'd tried this exact same patch back in https://reviews.llvm.org/D44619, but I was running into a bunch of check-clang failures with it, and I was never able to figure them out. It looks like it works now though, so I'm glad :) Richard's comment from that diff might still be relevant:

Testing

Correct patch

Straightforward fix, so LGTM.

In D52674#1298115, @rjmccall wrote:

In D52674#1297893, @smeenai wrote:

In D52674#1297879, @rjmccall wrote:

I'm not worried about the mangler being re-used for multiple declarations, I'm worried about a global flag changing how we mangle all components of a type when we only mean to change it at the top level.

Hmm, but don't we want it to affect all components? For example, consider something like:

@interface I
@end

template <class T> class C {};

void f();
void g() {
  try {
    f();
  } catch (C<I> *) {
  }
}

I would say that we want the RTTI for C<I> * to have the discriminator for I.

Why? IIUC, you're adding the discriminator to distinguish between two different RTTI objects. It's not like there are two different types. You should mangle C<I> normally here.

check-cxx-abilist passes for me with this applied (Mojave, Xcode 10.1).

In D52674#1297879, @rjmccall wrote:

I'm not worried about the mangler being re-used for multiple declarations, I'm worried about a global flag changing how we mangle all components of a type when we only mean to change it at the top level.

@rnk pointed out on IRC that the MicrosoftCXXNameMangler is actually specifically designed to manage the mangling of only a single name, in which case adding state to it for handling RTTI seems like a natural approach. @rjmccall, what do you think? I think this is much cleaner than having to thread through the RTTI state to every individual method. The ForRTTI_t enum is modeled after the ForDefinition_t enum used in CGM, but I'm happy to switch to a more general struct (as you'd mentioned before) if you prefer.

Stateful approach

LGTM, though you may wanna also wait for Zach.

In D52674#1293180, @rjmccall wrote:

In D52674#1291284, @smeenai wrote:

In D52674#1281747, @rjmccall wrote:

In D52674#1271814, @smeenai wrote:

@rjmccall I prototyped the ForRTTI parameter approach in D53546. It could definitely be cleaned up a bit, but it demonstrates the problems I saw with the added parameter. Namely, mangleType(QualType, SourceRange, QualifierMangleMode) has a bunch of additional logic which is needed for correctness, so we need to thread the parameter through the entire chain, and the only way I could think of doing that without adding the parameter to every single mangleType overload was to have an additional switch to dispatch to the overloads with the added ForRTTI parameter, which is pretty ugly.

As I see it, there are a few ways to proceed here:

• The approach in D53546 (cleaned up a bit). I think it's pretty ugly, but you may have suggestions on how to do it better.
• In the mangleType overload for ObjCObjectPointerType, skipping the generic mangleType dispatch and going directly to either the ObjCObjectType or ObjCInterfaceType overloads. That avoids the ugliness in the generic mangleType, but it also requires us to duplicate some logic from it in the ObjCObjectPointerType overload, which doesn't seem great either.
• Maintaining ForRTTI state in the mangler instead of threading a parameter through (I'm generally not a fan of state like that, but it might be cleaner than the threading in this case?)
• Just sticking with what I'm doing in this patch.
  
  What do you think?

Sorry for the delay. I think duplicating the dispatch logic for ObjCObjectPointerType is probably the best approach; the pointee type will always an ObjCObjectType of some sort, and there are only two kinds of those.

Sorry, I'm still not sure how this will work.

Duplicating the dispatch logic for ObjCObjectPointerType ends up looking like P8114, which is fine. However, when we're actually mangling RTTI or RTTI names, we're still going through the main mangleType(QualType, SourceRange, QualifierMangleMode) overload, which still requires us to thread ForRTTI through that function, which still requires us to duplicate the switch in that function (to handle the ForRTTI case, since the other switch is generated via TypeNodes.def and not easily modifiable), which is the main ugliness in my opinion. Do you also want me to add special dispatching to mangleCXXRTTI and mangleCXXRTTIName to just call the ObjCObjectPointerType function directly when they're dealing with that type? That's certainly doable, but at that point just keeping some state around in the demangler starts to feel cleaner, at least to me.

Well, you could check for an ObjCObjectPointerType in the top-level routine.

But yeah, probably the cleanest thing to do would to be push the state through the main dispatch. Don't push a single bool through, though; make a TypeManglingOptions struct to encapsulate it, in case we have a similar problem elsewhere. It should have a method for getting options that should be propagated down to component type manglings, and that method can drop the "for RTTI" bit; that's the part that ObjCObjectPointerType can handle differently.

In D54125#1291935, @pirama wrote:

In D54125#1291931, @smeenai wrote:

From the summary,

"In ELF, symbols non-weak symbols"

The first "symbols" seems extraneous?

I fixed it in the commit message, but Phabricator seems to showing the commit message from the first patch. I'll manually update the summary.

From the summary,

Sorry, had a leftover draft which I forgot to clean up. Edited in Phabricator now.

In D52674#1281747, @rjmccall wrote:

In D52674#1271814, @smeenai wrote:

@rjmccall I prototyped the ForRTTI parameter approach in D53546. It could definitely be cleaned up a bit, but it demonstrates the problems I saw with the added parameter. Namely, mangleType(QualType, SourceRange, QualifierMangleMode) has a bunch of additional logic which is needed for correctness, so we need to thread the parameter through the entire chain, and the only way I could think of doing that without adding the parameter to every single mangleType overload was to have an additional switch to dispatch to the overloads with the added ForRTTI parameter, which is pretty ugly.

As I see it, there are a few ways to proceed here:

• The approach in D53546 (cleaned up a bit). I think it's pretty ugly, but you may have suggestions on how to do it better.
• In the mangleType overload for ObjCObjectPointerType, skipping the generic mangleType dispatch and going directly to either the ObjCObjectType or ObjCInterfaceType overloads. That avoids the ugliness in the generic mangleType, but it also requires us to duplicate some logic from it in the ObjCObjectPointerType overload, which doesn't seem great either.
• Maintaining ForRTTI state in the mangler instead of threading a parameter through (I'm generally not a fan of state like that, but it might be cleaner than the threading in this case?)
• Just sticking with what I'm doing in this patch.
  
  What do you think?

Sorry for the delay. I think duplicating the dispatch logic for ObjCObjectPointerType is probably the best approach; the pointee type will always an ObjCObjectType of some sort, and there are only two kinds of those.

That's fair. I can just change the policy for clang explicitly in a separate diff then.

Somewhat orthogonal to this diff, but should we change clang so that it only does its own cmake_minimum_required call when it's being built standalone? There's potential for inconsistent policies between building clang in-tree and standalone if we do that, but it also seems strange to just wholesale ignore LLVM's policy settings.

I'm not familiar enough with this part of the code to feel comfortable accepting, sorry.

I notice that you're using LLVM_BUILD_INSTRUMENTED=IR, which corresponds to -fprofile-generate (IR-level profiling), instead of -fprofile-instr-generate (clang-level profiling). Did you play around with both and observe that IR-level profiling gave you better results?

LLVM's CMake has built in support for PGO; see https://llvm.org/docs/AdvancedBuilds.html#multi-stage-pgo. I haven't looked at the script in detail, but does it function similarly?

Ping @rjmccall. Let me know if the approach in D53546 is what you'd been envisioning, or if I'm just doing something completely brain-dead :)

Awesome, I don't have to maintain an internal patch for this anymore :)

@rjmccall I prototyped the ForRTTI parameter approach in D53546. It could definitely be cleaned up a bit, but it demonstrates the problems I saw with the added parameter. Namely, mangleType(QualType, SourceRange, QualifierMangleMode) has a bunch of additional logic which is needed for correctness, so we need to thread the parameter through the entire chain, and the only way I could think of doing that without adding the parameter to every single mangleType overload was to have an additional switch to dispatch to the overloads with the added ForRTTI parameter, which is pretty ugly.

Do you have real world performance numbers (both time and memory) for this? Computational complexity isn't always the best predictor of actual performance because of e.g. cache characteristics (as @Bigcheese mentioned).

In D52674#1253408, @rjmccall wrote:

In D52674#1253401, @smeenai wrote:

Actually, I take that back ... I just misread the stack trace.

There are a bunch of hops between the mangleCXXRTTI call and the ultimate mangling function:

MicrosoftMangleContextImpl::mangleCXXRTTI(QualType, raw_ostream &)
MicrosoftCXXNameMangler::mangleType(QualType, SourceRange, QualifierMangleMode)
MicrosoftCXXNameMangler::mangleType(const ObjCObjectPointerType *, Qualifiers, SourceRange)
MicrosoftCXXNameMangler::mangleType(QualType, SourceRange, QualifierMangleMode)
MicrosoftCXXNameMangler::mangleType(const ObjCObjectType *, Qualifiers, SourceRange)

(the last one will be ObjCInterfaceType instead of ObjCObjectType if catching anything other than id)

Threading a ForRTTI flag or similar all the way to the final call seems pretty tricky. I can add an optional paramater to mangleType(QualType, SourceRange, QualifierMangleMode), but that function uses a generated switch case to call the specific mangleType functions, and I don't know how to special-case certain types in that switch case (to pass the extra parameter along) without doing something super ugly. Adding the extra parameter to every single mangleType overload seems highly non-ideal, which is why I was thinking of maintaining some internal state instead.

Well, that's why I was talking about how the pointee type of an ObjCObjectPointerType is always an ObjCObjectType (of which ObjCInterfaceType is a subclass) — the implication being that you don't actually have to do the switch to dispatch to one of those two cases.

In D52662#1254561, @ldionne wrote:

In D52662#1254545, @smeenai wrote:

In D52662#1251942, @ldionne wrote:

In D52662#1251867, @smeenai wrote:

In D52662#1251866, @ldionne wrote:

We'd need to give default visibility to the vtable and the RTTI, which requires using an attribute.

Isn't that what __attribute__((type_visibility("default"))) accomplishes?

The problem is that it requires applying the attribute to the class template itself as opposed to a single instantiation of it. Unless I am wrong?

Thinking about this more, I guess you can't really have a key function for a class template anyway, since you need to define all your methods inline for clients to be able to use your class.

You're right. I guess we _might_ be able to come up with a twisted way to achieve this with type_visibility, for example: declare (but not define) the key function in the template, then define it out-of-line, but add an explicit instantiation declaration of the key function for the specific instantiation of the class template we're interested in. And then apply the type_visibility("default") attribute to an explicit instantiation declaration of the instantiation of the class template we're interested in. IDK whether this would work, and I don't care. It's messy and too complicated, and this area of the language is already crazy enough.

In D52662#1251942, @ldionne wrote:

In D52662#1251867, @smeenai wrote:

In D52662#1251866, @ldionne wrote:

In D52662#1251857, @smeenai wrote:

Hmm, why do we need to be able to explicitly instantiate vtables and RTTI ... doesn't the implied emission of those in the object file containing the key function work? I've been aware of a few -dev threads flying back and forth about various visibility issues for libc++, and I admit I haven't been keeping up with them fully, so I apologize if this has already been covered in one of those.

We'd need to give default visibility to the vtable and the RTTI, which requires using an attribute.

Isn't that what __attribute__((type_visibility("default"))) accomplishes?

The problem is that it requires applying the attribute to the class template itself as opposed to a single instantiation of it. Unless I am wrong?