Page MenuHomePhabricator

Remove the unused/undefined _cmd parameter to objc_direct methods.
ClosedPublic

Authored by mwyman on Aug 8 2022, 12:12 PM.

Details

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.

Diff Detail

Event Timeline

mwyman created this revision.Aug 8 2022, 12:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 12:12 PM
mwyman requested review of this revision.Aug 8 2022, 12:12 PM
plotfi added a comment.EditedAug 8 2022, 5:57 PM

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 added inline comments.Aug 8 2022, 6:58 PM
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);
mwyman updated this revision to Diff 451051.Aug 8 2022, 10:55 PM

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

mwyman marked an inline comment as done.Aug 9 2022, 4:04 PM
mwyman added a comment.Wed, Sep 7, 1:38 PM

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

kyulee added a subscriber: kyulee.Mon, Sep 12, 9:03 AM

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

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

This revision is now accepted and ready to land.Wed, Sep 21, 7:43 AM

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?

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

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

Oops, yep I'll send a patch.