Page MenuHomePhabricator

Add support for __attribute__((objc_class_stub))
AcceptedPublic

Authored by slavapestov on Mar 20 2019, 7:47 PM.

Details

Summary

This CL lands the Clang-side support for a new Objective-C runtime feature that adds support for dynamically-allocated metadata.

What follows is a basic description of this mechanism:

In Swift, the class metadata contains an Objective-C class object as a prefix, followed by a vtable, generic parameters, and field offsets. If the inheritance hierarchy of a class crosses a resilience boundary, we cannot statically emit the entire metadata. Instead, it is lazily built at runtime when first needed.

In order for such classes to be accessed from Clang, the Objective-C runtime will support a new mechanism whereby Swift can instead emit a "class stub" object. The class stub object is used by Clang in places where a class reference is normally emitted:

  • Classrefs emitted when messaging the class
  • Categories

A class stub contains an "isa pointer" which is a scalar value 1 followed by a function pointer which realizes the real class metadata. A reference to a class stub from a classref also has its least significant pointer set to 1.

The Objective-C runtime distinguishes a class stub from a real class using this fake "isa pointer". Instead of directly loading from a classref, Clang instead calls objc_loadClassref(); this function checks if the classref contains a class stub, and if so, it calls the function pointer and stores the result back into the classref to fast path future loads.

Note that Clang already supports a objc_runtime_visible attribute. The objc_class_stub attribute can be thought of as a generalization of objc_runtime_visible. Whereas objc_runtime_visible replaces the classref load with a runtime lookup of a class by string, the class stub mechanism is more efficient because the result of the lookup is cached. Also, objc_runtime_visible does not support categories to be defined on the class, whereas objc_class_stub does.

Diff Detail

Event Timeline

slavapestov created this revision.Mar 20 2019, 7:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2019, 7:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dexonsmith added a subscriber: dexonsmith.

+Erik and Alex, can you take a look at this?

rjmccall requested changes to this revision.Mar 20 2019, 9:46 PM
rjmccall added inline comments.
include/clang/Basic/AttrDocs.td
1118 ↗(On Diff #191626)

You should probably check that the user doesn't try to subclass classes annotated with this attribute, then. :)

lib/CodeGen/CGObjCMac.cpp
735 ↗(On Diff #191626)

It should be safe to make it readnone, which could theoretically optimize something like doing a class message-send in a loop.

7274 ↗(On Diff #191626)

This isn't for an arbitrary class ref, it's for the global class list. I'd say something like "the global class list sets the LSB to 1 on any class stubs".

7278 ↗(On Diff #191626)

It's more typical to do this with a GEP, although the add should still work.

lib/Sema/SemaDeclAttr.cpp
7126 ↗(On Diff #191626)

You should emit an error if !getLangOpts().ObjCRuntime.allowsClassStubs(), which should be straightforward to implement.

This revision now requires changes to proceed.Mar 20 2019, 9:46 PM

You should also add Sema tests that ensure the attribute applies to the expected AST nodes, is diagnosed appropriately when applied to something it shouldn't be applied to, accepts no args, etc. Basically, all of the semantic places we could warn on should have test coverage, as well as demonstrations of correct usage.

include/clang/Basic/Attr.td
1798 ↗(On Diff #191626)

Formatting is a smidge off here -- should be ObjCClassStub : Attr with the extra whitespace.

1802 ↗(On Diff #191626)

Does this attribute make sense outside of ObjC? e.g., should it require an ObjC compiland? If it should only be used in ObjC, then it should have a LangOpts field.

include/clang/Basic/AttrDocs.td
1116 ↗(On Diff #191626)

This seems like the wrong category -- the attribute doesn't apply to functions.

1118 ↗(On Diff #191626)

Try to keep the docs wrapped to the usual 80-col limit.

I think this could use a bit more exposition, or links to explain stuff. Based on the small amount here, I still have no idea why I would use this attribute.

jordan_rose added inline comments.
lib/CodeGen/CGObjCMac.cpp
7285 ↗(On Diff #191626)

Is there a concern here in the non-stub case if GetClassGlobal no longer produces a ObjCTypes.ClassnfABIPtrTy? (Probably not, but thought I'd check [again].)

slavapestov marked 11 inline comments as done.Mar 26 2019, 3:33 PM

Thanks for the comments everyone. In the next version of the patch, I fixed most of the review feedback and also addressed an issue where super message sends were not calling objc_loadClassRef().

include/clang/Basic/Attr.td
1802 ↗(On Diff #191626)

The other ObjC attributes, such as ObjCSubclassingRestricted, do not have a LangOpts. Do you want me to add a LangOpts field to those too? Or is it unnecessary since they're already restricted to InterfaceDecls?

include/clang/Basic/AttrDocs.td
1116 ↗(On Diff #191626)

Would DocCatType make more sense? Would you like me to change ObjCRuntimeVisible and a handful of other miscategorized attributes too?

lib/CodeGen/CGObjCMac.cpp
7274 ↗(On Diff #191626)

Can you explain what the difference is? My understanding is that the thing you pass to objc_loadClassref() (or load from directly) is a "classref". This is for classes you reference, not classes you define.

7285 ↗(On Diff #191626)

You raise a good point. I brought back the assert in the next iteration of the patch.

slavapestov marked 2 inline comments as done.

Second revision adds the following:

  • Validation of the attribute in Sema
  • Sema tests for the above
  • Correct behavior of super method calls of a class with the attribute
  • More comprehensive CodeGen tests
rjmccall added inline comments.Mar 26 2019, 3:38 PM
lib/CodeGen/CGObjCMac.cpp
7274 ↗(On Diff #191626)

Oh, I see what you're saying, nevermind.

I don't know what the etiquette is around here about pinging reviewers for a re-review, but this CL is ready for another look. Your feedback is much appreciated!

I don't know what the etiquette is around here about pinging reviewers for a re-review, but this CL is ready for another look. Your feedback is much appreciated!

Thanks for letting us know you think you've addressed all the feedback. We usually recommend pinging once a week if there's no activity on a review thread, but it's no problem to ping sooner when you have more information.

include/clang/Basic/Attr.td
1802 ↗(On Diff #191626)

The other ObjC attributes, such as ObjCSubclassingRestricted, do not have a LangOpts. Do you want me to add a LangOpts field to those too? Or is it unnecessary since they're already restricted to InterfaceDecls?

Don't feel obligated, but if you wanted to post a follow-up patch that adds this to the other ObjC attribute, I think it would be a good change to make. It's not strictly required, but I think the diagnostics are a bit more clear when we restrict the language options.

include/clang/Basic/AttrDocs.td
1116 ↗(On Diff #191626)

Oye, it looks like we have a lot of inconsistent categories currently. It's not really a type attribute, it's a declaration attribute, but we don't have such a notion yet. Go ahead and leave this as DocCatType for now; I'll see about cleaning this up in a follow-up.

include/clang/Basic/ObjCRuntime.h
437–443 ↗(On Diff #192378)

I'd prefer if you grouped these cases and did it like:

switch (getKind()) {
case FragileMacOSX:
case GCC:
case GNUstep:
case ObjFW:
  return false;
case MacOSX:
case iOS:
case WatchOS:
  return true;
}
lib/CodeGen/CGObjCMac.cpp
7269 ↗(On Diff #192378)

Please don't use auto here, the type is not spelled out in the initialization.

7347 ↗(On Diff #192378)

Don't use auto here either.

lib/Sema/SemaDeclObjC.cpp
4097 ↗(On Diff #192378)

Elide braces.

4131–4133 ↗(On Diff #192378)

This should be done in Attr.td. You'll need to add a new LangOpt subclass (around line 298 or so are examples), and then add it to the LangOpts array when defining the attribute. Then you can remove this code as well as the new diagnostic.

4134 ↗(On Diff #192378)

Elide braces.

test/CodeGenObjC/class-stubs.m
2 ↗(On Diff #192378)

Hmm, this looks suspicious -- I think it's more clear to just bump this to the previous line rather than wonder if the RUN: command will confuse things.

rjmccall added inline comments.Apr 1 2019, 11:53 AM
lib/Sema/SemaDeclObjC.cpp
4131–4133 ↗(On Diff #192378)

I don't think there's a way to run arbitrary code in a LangOpt right now, but it should be relatively straightforward to generalize ClangAttrEmitter to handle this. Just add an optional Code property to LangOpt that's expressed in terms of an assumed variable LangOpts, so that Attr.td could have a line like:

def ObjCClassStubsAllowed : LangOpt<"ObjCClassStubsAllowed", "LangOpts.ObjCRuntime.allowsClassStubs()">;

ClangAttrEmitter would take that expression, parenthesize it, and use it where it currently expands to "LangOpts." + Name. It should be possible to remove the Negated field in favor of this.

I guess that's probably worth doing vs. just having some hard-coded logic.

slavapestov marked 8 inline comments as done.
aaron.ballman added inline comments.Apr 3 2019, 5:53 AM
include/clang/Basic/Attr.td
293 ↗(On Diff #193407)

I think the type here should be code instead of string since the user is passing in code snippets, no?

lib/Sema/SemaDeclObjC.cpp
4129–4130 ↗(On Diff #193407)

Combine these if statements?

aaron.ballman added inline comments.Apr 3 2019, 7:26 AM
include/clang/Basic/AttrDocs.td
1116 ↗(On Diff #191626)

I added DocCatDecl in r357585, which I think would be the correct category to use here.

rjmccall added inline comments.Apr 4 2019, 8:29 AM
include/clang/Basic/Attr.td
290 ↗(On Diff #193407)

I think there's a grand total of one use of negated, so you might as well rewrite it to use customCode; see below.

306 ↗(On Diff #193407)

This is not an appropriate name for this LangOpt. How about ObjCAllowsClassStubs?

include/clang/Basic/AttrDocs.td
1100 ↗(On Diff #193407)

We should add something here to make it clear what the evolution story with this attribute is. Presumably you can't add it to an existing class without breaking ABI. Are there circumstances under which we're willing to guarantee that it can be removed (under a sufficiently-recent deployment target)?

utils/TableGen/ClangAttrEmitter.cpp
3442 ↗(On Diff #193407)

I think the contract with CustomCode ought to be that LangOpts will be bound as an unqualified name, so that the custom code can be an arbitrary expression in terms of that. That will allow e.g. compound and negated LangOpts to be defined, like LangOpts.OpenCL && LangOpts.CPlusPlus. So you need to generate auto &LangOpts = S.LangOpts; at the top of the function, at least when custom code is present; and you should probably add parens around the code just to be safe.

Alternatively, you could make the contract that CustomCode should contain %0 or some other string that'll be expanded to an expression for the language options, but that sounds pretty complex compared to just binding LangOpts in the context.

slavapestov marked 3 inline comments as done.May 16 2019, 12:53 PM

Updated patch to address review feedback.

Thanks! Down to minor stuff now.

clang/include/clang/Basic/Attr.td
293

Please add a comment here explaining how to use CustomCode.

Can we just remove Negated and define COnly as !LangOpts.CPlusPlus?

slavapestov marked an inline comment as done.
aaron.ballman added inline comments.May 17 2019, 6:44 AM
clang/include/clang/Basic/Attr.td
292

If this is code, should it be using a code type rather than a string type?

clang/include/clang/Basic/AttrDocs.td
1101

Swift generated -> Swift-generated

clang/lib/CodeGen/CGObjCMac.cpp
736

Please spell the type out explicitly rather than use auto here.

7325

Can you add a message to this assertion so that when it triggers, users get something more helpful to report?

7367

Can you spell out the type here, since you're changing the initialization already?

7411–7412

I'm not opposed to this change, but it seems unrelated to the patch. Feel free to commit separately as an NFC.

slavapestov marked 6 inline comments as done.
rjmccall accepted this revision.May 22 2019, 1:49 PM

LGTM, thanks! You should wait for Aaron's approval, too.

This revision is now accepted and ready to land.May 22 2019, 1:49 PM
aaron.ballman accepted this revision.May 23 2019, 7:54 AM

LGTM!

clang/include/clang/Basic/Attr.td
297

This compiles? I would have assumed that it would have to be def COnly : LangOpt<"COnly", [{!LangOpts.CPlusPlus}]>;

(If it compiles as-is, that's fine, I just wasn't aware that you could use string syntax for code blocks.)

slavapestov marked 2 inline comments as done.
slavapestov added inline comments.
clang/include/clang/Basic/Attr.td
297

It appears that both work! Going with your suggested version.