[PATCH] D59628: Add support for __attribute__((objc_class_stub))

[Slava Pestov via Phabricator via cfe-commits]

slavapestov marked 11 inline comments as done.
slavapestov added a comment.

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()`.



================
Comment at: include/clang/Basic/Attr.td:1802
+  let Documentation = [ObjCClassStubDocs];
+}
+
----------------
aaron.ballman wrote:
> 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.
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?


================
Comment at: include/clang/Basic/AttrDocs.td:1116
+def ObjCClassStubDocs : Documentation {
+    let Category = DocCatFunction;
+    let Content = [{
----------------
aaron.ballman wrote:
> This seems like the wrong category -- the attribute doesn't apply to functions.
Would DocCatType make more sense? Would you like me to change ObjCRuntimeVisible and a handful of other miscategorized attributes too?


================
Comment at: lib/CodeGen/CGObjCMac.cpp:7274
+        // Classrefs pointing at Objective-C stub classes have the least
+        // significant bit set to 1.
+        auto *Tag = llvm::ConstantInt::get(CGM.IntPtrTy, 1);
----------------
rjmccall wrote:
> 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".
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.


================
Comment at: lib/CodeGen/CGObjCMac.cpp:7285
 
-    Entry = new llvm::GlobalVariable(CGM.getModule(), ObjCTypes.ClassnfABIPtrTy,
+    Entry = new llvm::GlobalVariable(CGM.getModule(), ClassGV->getType(),
                                      false, llvm::GlobalValue::PrivateLinkage,
----------------
jordan_rose wrote:
> 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].)
You raise a good point. I brought back the assert in the next iteration of the patch.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59628/new/

https://reviews.llvm.org/D59628