Remove the unused/undefined _cmd parameter to objc_direct methods.

[mwyman]

Summary

When objc_direct methods were implemented, the implicit _cmd parameter was left as an argument to the method implementation function, but was unset by callers; if the method body referenced the _cmd variable, a selector load would be emitted inside the body. However, this leaves an unused argument in the ABI, and is unnecessary.

This change removes the empty/unset argument, and if _cmd is referenced inside an objc_direct method it will emit local storage for the implicit variable. From the ABI perspective, objc_direct methods will have the implicit self parameter, immediately followed by whatever explicit arguments are defined on the method, rather than having one unset/undefined register in the middle.

[plotfi]

I tried running the following on some example code and got a stacktrace:

// RUN: clang -cc1 -no-opaque-pointers -emit-llvm -fobjc-arc -triple x86_64-apple-darwin10 -o - %s  -O0

@interface C
- (int)getInt __attribute__((objc_direct));
@end

@implementation C
- (int)getInt __attribute__((objc_direct)) {
}
@end

void f() {
  C *c;
  [c getInt];
}

7  clang-16                 0x00000001054ee160 clang::CodeGen::CGFunctionInfo::create(unsigned int, bool, bool, clang::FunctionType::ExtInfo const&, llvm::ArrayRef<clang::FunctionType::ExtParameterInfo>, clang::CanQual<clang::Type>, llvm::ArrayRef<clang::CanQual<clang::Type>>, clang::CodeGen::RequiredArgs) (.cold.2) + 0
8  clang-16                 0x000000010291f50c clang::CodeGen::CGFunctionInfo::create(unsigned int, bool, bool, clang::FunctionType::ExtInfo const&, llvm::ArrayRef<clang::FunctionType::ExtParameterInfo>, clang::CanQual<clang::Type>, llvm::ArrayRef<clang::CanQual<clang::Type>>, clang::CodeGen::RequiredArgs) + 608

Looks like the assert is on the arg length not patching the param info length due to dropping the leading arg:

assert(paramInfos.empty() || paramInfos.size() == argTypes.size());

[plotfi]

clang/lib/CodeGen/CGCall.cpp

487–488

In order to fix the assert try this instead on line 487:

SmallVector<FunctionProtoType::ExtParameterInfo, 4> extParamInfos(
    MD->isDirectMethod() ? 1 : 2);

[plotfi]

Updated D86049.

[mwyman]

Fixed assert due to mis-matched number of expected parameters.

[mwyman]

This is related to changes for https://reviews.llvm.org/D86049.

[mwyman]

Hi Akira,

I'd reached out to John offline and he'd mentioned you might be able to help on some of these objc_direct reviews; if so, that would be wonderful!

-Michael

[plotfi]

In D131424#3803190, @mwyman wrote:

Hi Akira,

I'd reached out to John offline and he'd mentioned you might be able to help on some of these objc_direct reviews; if so, that would be wonderful!

-Michael

I would also be open to review for https://reviews.llvm.org/D86049 Akira.

-Puyan

[ahatanak]

LGTM

[ahatanak]

It looks like this patch causes an assertion to fail (Assertion failed: (it != LocalDeclMap.end() && "Invalid argument to GetAddrOfLocalVar(), no decl!")) when the following code is compiled.

__attribute__((objc_direct_members))
@interface C
@property id prop;
-(void)m0;
@end

@implementation C
-(void)m0 {
}
@end

Do we need to synthesize _cmd in generateObjCGetterBody and generateObjCSetterBody if the property is direct too?

[ahatanak]

-fobjc-arc is needed to see the assertion fail.

[mwyman]

In D131424#3828647, @ahatanak wrote:

-fobjc-arc is needed to see the assertion fail.

Oops, yep I'll send a patch.

[mwyman]

In D131424#3828656, @mwyman wrote:

In D131424#3828647, @ahatanak wrote:

-fobjc-arc is needed to see the assertion fail.

Oops, yep I'll send a patch.

Sent https://reviews.llvm.org/D135091

[stephanemoore]

LGTM after fixing the assertion ✅

clang/lib/CodeGen/CGCall.cpp

 /// using the given receiver type.  The receiver type is not always
 /// the 'self' type of the method or even an Objective-C pointer type.
 /// This is *not* the right method for actually performing such a
 /// message send, due to the possibility of optional arguments.
 const CGFunctionInfo &
 CodeGenTypes::arrangeObjCMessageSendSignature(const ObjCMethodDecl *MD,
                                               QualType receiverType) {
   SmallVector<CanQualType, 16> argTys;
-  SmallVector<FunctionProtoType::ExtParameterInfo, 4> extParamInfos(2);
+  SmallVector<FunctionProtoType::ExtParameterInfo, 4> extParamInfos(
+      MD->isDirectMethod() ? 1 : 2);
   argTys.push_back(Context.getCanonicalParamType(receiverType));
+  if (!MD->isDirectMethod())
     argTys.push_back(Context.getCanonicalParamType(Context.getObjCSelType()));
   // FIXME: Kill copy?
   for (const auto *I : MD->parameters()) {
     argTys.push_back(Context.getCanonicalParamType(I->getType()));
     auto extParamInfo = FunctionProtoType::ExtParameterInfo().withIsNoEscape(
         I->hasAttr<NoEscapeAttr>());
     extParamInfos.push_back(extParamInfo);
   }
 

clang/lib/CodeGen/CGObjC.cpp

if (OMD->isDirectMethod()) {
     Fn->setVisibility(llvm::Function::HiddenVisibility);
     CGM.SetLLVMFunctionAttributes(OMD, FI, Fn, /*IsThunk=*/false);
     CGM.SetLLVMFunctionAttributesForDefinition(OMD, Fn);
   } else {
     CGM.SetInternalFunctionAttributes(OMD, Fn, FI);
   }
 
   args.push_back(OMD->getSelfDecl());
+  if (!OMD->isDirectMethod())
     args.push_back(OMD->getCmdDecl());
 
   args.append(OMD->param_begin(), OMD->param_end());
 
   CurGD = OMD;
   CurEHLocation = OMD->getEndLoc();
 
   StartFunction(OMD, OMD->getReturnType(), Fn, FI, args,
                 OMD->getLocation(), StartLoc);

clang/lib/CodeGen/CGObjCMac.cpp

CGObjCCommonMac::EmitMessageSend(CodeGen::CodeGenFunction &CGF,
   CodeGenTypes &Types = CGM.getTypes();
   auto selTy = CGF.getContext().getObjCSelType();
   llvm::Value *SelValue = llvm::UndefValue::get(Types.ConvertType(selTy));
 
   CallArgList ActualArgs;
   if (!IsSuper)
     Arg0 = CGF.Builder.CreateBitCast(Arg0, ObjCTypes.ObjectPtrTy);
   ActualArgs.add(RValue::get(Arg0), Arg0Ty);
+  if (!Method || !Method->isDirectMethod())
     ActualArgs.add(RValue::get(SelValue), selTy);
   ActualArgs.addFrom(CallArgs);
 
   // If we're calling a method, use the formal signature.
   MessageSendInfo MSI = getMessageSendInfo(Method, ResultType, ActualArgs);
 
   if (Method)
     assert(CGM.getContext().getCanonicalType(Method->getReturnType()) ==
                CGM.getContext().getCanonicalType(ResultType) &&
if (ReceiverCanBeNull) {
 
     // rest of the body
     CGF.EmitBlock(ContBlock);
     Builder.SetInsertPoint(ContBlock);
   }
 
   // only synthesize _cmd if it's referenced
   if (OMD->getCmdDecl()->isUsed()) {
+    // `_cmd` is not a parameter to direct methods, so storage must be
+    // explicitly declared for it.
+    CGF.EmitVarDecl(*OMD->getCmdDecl());
     Builder.CreateStore(GetSelector(CGF, OMD),
                         CGF.GetAddrOfLocalVar(OMD->getCmdDecl()));
   }
 }
 
 llvm::GlobalVariable *CGObjCCommonMac::CreateMetadataVar(Twine Name,
                                                ConstantStructBuilder &Init,
                                                          StringRef Section,

clang/test/CodeGenObjC/direct-method.m

 }
 
 // CHECK-LABEL: define hidden i32 @"\01-[Root getInt]"(
 - (int)getInt __attribute__((objc_direct)) {
   // loading parameters
   // CHECK-LABEL: entry:
   // CHECK-NEXT: [[RETVAL:%.*]] = alloca
   // CHECK-NEXT: [[SELFADDR:%.*]] = alloca %0*,
-  // CHECK-NEXT: [[_CMDADDR:%.*]] = alloca i8*,
   // CHECK-NEXT: store %0* %{{.*}}, %0** [[SELFADDR]],
-  // CHECK-NEXT: store i8* %{{.*}}, i8** [[_CMDADDR]],
 
   // self nil-check
   // CHECK-NEXT: [[SELF:%.*]] = load %0*, %0** [[SELFADDR]],
   // CHECK-NEXT: [[NILCHECK:%.*]] = icmp eq %0* [[SELF]], null
   // CHECK-NEXT: br i1 [[NILCHECK]],
 
   // setting return value to nil
   // CHECK-LABEL: objc_direct_method.self_is_nil:
- (int)getInt __attribute__((objc_direct)) {
   return 42;
 }
 
 // CHECK-LABEL: define hidden i32 @"\01+[Root classGetInt]"(
 + (int)classGetInt __attribute__((objc_direct)) {
   // loading parameters
   // CHECK-LABEL: entry:
   // CHECK-NEXT: [[SELFADDR:%.*]] = alloca i8*,
-  // CHECK-NEXT: [[_CMDADDR:%.*]] = alloca i8*,
   // CHECK-NEXT: store i8* %{{.*}}, i8** [[SELFADDR]],
-  // CHECK-NEXT: store i8* %{{.*}}, i8** [[_CMDADDR]],
 
   // [self self]
   // CHECK-NEXT: [[SELF:%.*]] = load i8*, i8** [[SELFADDR]],
   // CHECK-NEXT: [[SELFSEL:%.*]] = load i8*, i8** @OBJC_SELECTOR_REFERENCES_
   // CHECK-NEXT: [[SELF0:%.*]] = call {{.*}} @objc_msgSend
   // CHECK-NEXT: store i8* [[SELF0]], i8** [[SELFADDR]],
 
   // return
   // CHECK-NEXT: ret
   return 42;
 }
 
 // CHECK-LABEL: define hidden i64 @"\01-[Root getComplex]"(
 - (struct my_complex_struct)getComplex __attribute__((objc_direct)) {
   // loading parameters
   // CHECK-LABEL: entry:
   // CHECK-NEXT: [[RETVAL:%.*]] = alloca
   // CHECK-NEXT: [[SELFADDR:%.*]] = alloca %0*,
-  // CHECK-NEXT: [[_CMDADDR:%.*]] = alloca i8*,
   // CHECK-NEXT: store %0* %{{.*}}, %0** [[SELFADDR]],
-  // CHECK-NEXT: store i8* %{{.*}}, i8** [[_CMDADDR]],
 
   // self nil-check
   // CHECK-NEXT: [[SELF:%.*]] = load %0*, %0** [[SELFADDR]],
   // CHECK-NEXT: [[NILCHECK:%.*]] = icmp eq %0* [[SELF]], null
   // CHECK-NEXT: br i1 [[NILCHECK]],
 
   // setting return value to nil
   // CHECK-LABEL: objc_direct_method.self_is_nil:

 
 // CHECK-LABEL: define hidden void @"\01-[Root getAggregate]"(
 - (struct my_aggregate_struct)getAggregate __attribute__((objc_direct)) {
   // CHECK: %struct.my_aggregate_struct* noalias sret(%struct.my_aggregate_struct) align 4 [[RETVAL:%[^,]*]],
 
   // loading parameters
   // CHECK-LABEL: entry:
   // CHECK-NEXT: [[SELFADDR:%.*]] = alloca %0*,
-  // CHECK-NEXT: [[_CMDADDR:%.*]] = alloca i8*,
   // CHECK-NEXT: store %0* %{{.*}}, %0** [[SELFADDR]],
-  // CHECK-NEXT: store i8* %{{.*}}, i8** [[_CMDADDR]],
 
   // self nil-check
   // CHECK-NEXT: [[SELF:%.*]] = load %0*, %0** [[SELFADDR]],
   // CHECK-NEXT: [[NILCHECK:%.*]] = icmp eq %0* [[SELF]], null
   // CHECK-NEXT: br i1 [[NILCHECK]],
 
   // setting return value to nil
   // CHECK-LABEL: objc_direct_method.self_is_nil:

 
 // CHECK-LABEL: define hidden void @"\01+[Root classGetAggregate]"(
 + (struct my_aggregate_struct)classGetAggregate __attribute__((objc_direct)) {
   struct my_aggregate_struct st = {.a = 42};
   return st;
   // CHECK: ret void
 }
 
+// CHECK-LABEL: define hidden void @"\01-[Root accessCmd]"(
+- (void)accessCmd __attribute__((objc_direct)) {
+  // CHECK-LABEL: entry:
+  // CHECK-NEXT: [[SELFADDR:%.*]] = alloca %0*,
+  // CHECK-NEXT: [[CMDVAL:%_cmd]] = alloca i8*,
+
+  // loading the _cmd selector
+  // CHECK-LABEL: objc_direct_method.cont:
+  // CHECK-NEXT: [[CMD1:%.*]] = load i8*, i8** @OBJC_SELECTOR_REFERENCES_
+  // CHECK-NEXT: store i8* [[CMD1]], i8** [[CMDVAL]],
+  SEL sel = _cmd;
+}
+
 @end
 // CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
 
 @interface Foo : Root {
   id __strong _cause_cxx_destruct;
 }
 @property(nonatomic, readonly, direct) int getDirect_setDynamic;
 @property(nonatomic, readonly) int getDynamic_setDirect;

 // CHECK-LABEL: define hidden i32 @"\01-[Foo directMethodInCategoryNoDecl]"(
 - (int)directMethodInCategoryNoDecl __attribute__((objc_direct)) {
   return 42;
 }
 @end
 
 int useRoot(Root *r) {
   // CHECK-LABEL: define{{.*}} i32 @useRoot
-  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root getInt]"
-  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root intProperty]"
-  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root intProperty2]"
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root getInt]" to i32 (i8*)
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root intProperty]" to i32 (i8*)
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Root intProperty2]" to i32 (i8*)
   return [r getInt] + [r intProperty] + [r intProperty2];
 }
 
 int useFoo(Foo *f) {
   // CHECK-LABEL: define{{.*}} i32 @useFoo
-  // CHECK: call void bitcast {{.*}} @"\01-[Foo setGetDynamic_setDirect:]"
-  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo getDirect_setDynamic]"
-  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInExtension]"
-  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInCategory]"
-  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInCategoryNoDecl]"
+  // CHECK: call void bitcast {{.*}} @"\01-[Foo setGetDynamic_setDirect:]" to void (i8*, i32)
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo getDirect_setDynamic]" to i32 (i8*)
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInExtension]" to i32 (i8*)
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInCategory]" to i32 (i8*)
+  // CHECK: %{{[^ ]*}} = call i32 bitcast {{.*}} @"\01-[Foo directMethodInCategoryNoDecl]" to i32 (i8*)
   [f setGetDynamic_setDirect:1];
   return [f getDirect_setDynamic] +
          [f directMethodInExtension] +
          [f directMethodInCategory] +
          [f directMethodInCategoryNoDecl];
 }
 
 __attribute__((objc_root_class))