Make sure that the implicit arguments for direct methods have been setup

[MadCoder]

Summary

Also use the Canonical Decl to index the set of Direct methods so that
when calls and implementations interleave, the same llvm::Function is
used and the same symbol name emitted.

Add a codegen test, that cover both issues.

Also fix a bunch of code in clang that had wrong assumptions about when
getSelfDecl() would be set:

• CGDebugInfo::getObjCMethodName and AnalysisConsumer::getFunctionName would assume that it was set for method declarations part of a protocol, which they never were, and that self would be a Class type, which it isn't as it is id for a protocol.

• ASTDeclWriter::VisitObjCMethodDecl was assuming that having a SelfDecl meant that it MUST BE a method definition which is no longer true.

Radar-Id: rdar://problem/57661767

[MadCoder]

turns out that I had no codegen check for the call site and that one of the last iteration broke it trivially :'(

[MadCoder]

hmm wait I have an old problem I had fixed creep up again :'(

need to fix it again sigh.

[liuliu]

Did you mean the linkage issue?

[MadCoder]

Fix the fact that the hashmap of direct method was indexed by Declarations instead of names (and depending on code ordering, the declaration used at codegen time may be the one from the @interface or from the @implementation leading to name collisions and llvm "helpfully" adding .1's everywhere

[arphaman]

@MadCoder Indexing by names doesn't seem like the right solution. You should be able to get the canonical declaration (getCanonicalDecl) from the declaration that represents the method in @implementation, which should avoid the problem you're describing.

[MadCoder]

oh hah, thanks :)

[jfb]

You should upload patches with context :)

clang/lib/CodeGen/CGObjCMac.cpp

4030 ↗

(On Diff #232491)

I'd rather have auto * or Decl * here.

[arphaman]

LGTM, after fixing JF's comments. Please upload patches with full context as JF mentioned, as it helps with the review.

[MadCoder]

just added all the context this time (-W)

clang/lib/CodeGen/CGObjCMac.cpp

4030 ↗

(On Diff #232491)

this is a pair this doesn't work

[liuliu]

With this latest fix applied on top of b220662a45c8067a2ae485ae34c1138d93506df9, in our company's internal code, I still encounter the crash.

Showing All Messages
/Users/liuliu/Snapchat/Dev/phantom/3.	Libraries/Storage/DocObject/Implementations/SCSQLiteDocObjectContext/Tests/Generated/SCTestMainEntityChangeRequest.mm:338:25: LLVM IR generation of compound statement ('{}')
/Users/liuliu/Snapchat/Dev/phantom/0  clang-10                 0x000000010b9edf0c llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
1  clang-10                 0x000000010b9ee4c9 PrintStackTraceSignalHandler(void*) + 25
/Users/liuliu/Snapchat/Dev/phantom/2  clang-10                 0x000000010b9ec086 llvm::sys::RunSignalHandlers() + 118
3  clang-10                 0x000000010b9f1e6c SignalHandler(int) + 252
4  libsystem_platform.dylib 0x00007fff65d8eb5d _sigtramp + 29
5  libsystem_platform.dylib 0x00007ffee7e9b790 _sigtramp + 18446744071596723280
/Users/liuliu/Snapchat/Dev/phantom/6  clang-10                 0x000000010bfdff14 clang::CodeGen::CodeGenTypes::arrangeObjCMethodDeclaration(clang::ObjCMethodDecl const*) + 52
/Users/liuliu/Snapchat/Dev/phantom/7  clang-10                 0x000000010c1f6bcd (anonymous namespace)::CGObjCCommonMac::GenerateDirectMethod(clang::ObjCMethodDecl const*, clang::ObjCContainerDecl const*) + 285
/Users/liuliu/Snapchat/Dev/phantom/8  clang-10                 0x000000010c1f63e9 (anonymous namespace)::CGObjCCommonMac::EmitMessageSend(clang::CodeGen::CodeGenFunction&, clang::CodeGen::ReturnValueSlot, clang::QualType, clang::Selector, llvm::Value*, clang::QualType, bool, clang::CodeGen::CallArgList const&, clang::ObjCMethodDecl const*, clang::ObjCInterfaceDecl const*, (anonymous namespace)::ObjCCommonTypesHelper const&) + 1401
/Users/liuliu/Snapchat/Dev/phantom/9  clang-10                 0x000000010c205b14 (anonymous namespace)::CGObjCNonFragileABIMac::GenerateMessageSend(clang::CodeGen::CodeGenFunction&, clang::CodeGen::ReturnValueSlot, clang::QualType, clang::Selector, llvm::Value*, clang::CodeGen::CallArgList const&, clang::ObjCInterfaceDecl const*, clang::ObjCMethodDecl const*) + 644
/Users/liuliu/Snapchat/Dev/phantom/10 clang-10                 0x000000010c1931b1 clang::CodeGen::CGObjCRuntime::GeneratePossiblySpecializedMessageSend(clang::CodeGen::CodeGenFunction&, clang::CodeGen::ReturnValueSlot, clang::QualType, clang::Selector, llvm::Value*, clang::CodeGen::CallArgList const&, clang::ObjCInterfaceDecl const*, clang::ObjCMethodDecl const*, bool) + 433
/Users/liuliu/Snapchat/Dev/phantom/11 clang-10                 0x000000010c1942d2 clang::CodeGen::CodeGenFunction::EmitObjCMessageExpr(clang::ObjCMessageExpr const*, clang::CodeGen::ReturnValueSlot) + 2818
/Users/liuliu/Snapchat/Dev/phantom/12 clang-10                 0x000000010c15cda8 (anonymous namespace)::ScalarExprEmitter::VisitObjCMessageExpr(clang::ObjCMessageExpr*) + 184
/Users/liuliu/Snapchat/Dev/phantom/13 clang-10                 0x000000010c15522b clang::StmtVisitorBase<std::__1::add_pointer, (anonymous namespace)::ScalarExprEmitter, llvm::Value*>::Visit(clang::Stmt*) + 6523
/Users/liuliu/Snapchat/Dev/phantom/14 clang-10                 0x000000010c14a309 (anonymous namespace)::ScalarExprEmitter::Visit(clang::Expr*) + 73
/Users/liuliu/Snapchat/Dev/phantom/15 clang-10                 0x000000010c14a26d clang::CodeGen::CodeGenFunction::EmitScalarExpr(clang::Expr const*, bool) + 189
/Users/liuliu/Snapchat/Dev/phantom/16 clang-10                 0x000000010c1a209f emitARCRetainCallResult(clang::CodeGen::CodeGenFunction&, clang::Expr const*) + 31
/Users/liuliu/Snapchat/Dev/phantom/17 clang-10                 0x000000010c1a1eb4 clang::CodeGen::CodeGenFunction::EmitARCReclaimReturnedObject(clang::Expr const*, bool) + 116
/Users/liuliu/Snapchat/Dev/phantom/18 clang-10                 0x000000010c1650ff (anonymous namespace)::ScalarExprEmitter::VisitCastExpr(clang::CastExpr*) + 4511
/Users/liuliu/Snapchat/Dev/phantom/19 clang-10                 0x000000010c15a5d8 clang::StmtVisitorBase<std::__1::add_pointer, (anonymous namespace)::ScalarExprEmitter, llvm::Value*>::VisitImplicitCastExpr(clang::ImplicitCastExpr*) + 40
/Users/liuliu/Snapchat/Dev/phantom/20 clang-10                 0x000000010c154d93 clang::StmtVisitorBase<std::__1::add_pointer, (anonymous namespace)::ScalarExprEmitter, llvm::Value*>::Visit(clang::Stmt*) + 5347
/Users/liuliu/Snapchat/Dev/phantom/21 clang-10                 0x000000010c14a309 (anonymous namespace)::ScalarExprEmitter::Visit(clang::Expr*) + 73
/Users/liuliu/Snapchat/Dev/phantom/22 clang-10                 0x000000010c14a26d clang::CodeGen::CodeGenFunction::EmitScalarExpr(clang::Expr const*, bool) + 189
/Users/liuliu/Snapchat/Dev/phantom/23 clang-10                 0x000000010c0c1c37 clang::CodeGen::CodeGenFunction::EmitAnyExpr(clang::Expr const*, clang::CodeGen::AggValueSlot, bool) + 167
/Users/liuliu/Snapchat/Dev/phantom/24 clang-10                 0x000000010c0e6543 emitPseudoObjectExpr(clang::CodeGen::CodeGenFunction&, clang::PseudoObjectExpr const*, bool, clang::CodeGen::AggValueSlot) + 1379
/Users/liuliu/Snapchat/Dev/phantom/25 clang-10                 0x000000010c0e5fb0 clang::CodeGen::CodeGenFunction::EmitPseudoObjectRValue(clang::PseudoObjectExpr const*, clang::CodeGen::AggValueSlot) + 112
/Users/liuliu/Snapchat/Dev/phantom/26 clang-10                 0x000000010c15d999 (anonymous namespace)::ScalarExprEmitter::VisitPseudoObjectExpr(clang::PseudoObjectExpr*) + 89

It indeed won't crash any more in trivial examples. I need to have some other time to reduce my local example to a reasonable size.

[arphaman]

@MadCoder unfortunately this change causes several test failures when check-clang runs:

Clang :: Analysis/analyzeOneFunction.m
Clang :: Index/c-index-api-loadTU-test.m
Clang :: SemaObjC/dealloc.m

Please fix them before committing this.

[MadCoder]

In D71091#1773066, @liuliu wrote:

With this latest fix applied on top of b220662a45c8067a2ae485ae34c1138d93506df9, in our company's internal code, I still encounter the crash.

Showing All Messages
/Users/liuliu/Snapchat/Dev/phantom/3.	Libraries/Storage/DocObject/Implementations/SCSQLiteDocObjectContext/Tests/Generated/SCTestMainEntityChangeRequest.mm:338:25: LLVM IR generation of compound statement ('{}')
/Users/liuliu/Snapchat/Dev/phantom/0  clang-10                 0x000000010b9edf0c llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
1  clang-10                 0x000000010b9ee4c9 PrintStackTraceSignalHandler(void*) + 25
/Users/liuliu/Snapchat/Dev/phantom/2  clang-10                 0x000000010b9ec086 llvm::sys::RunSignalHandlers() + 118
3  clang-10                 0x000000010b9f1e6c SignalHandler(int) + 252
4  libsystem_platform.dylib 0x00007fff65d8eb5d _sigtramp + 29
5  libsystem_platform.dylib 0x00007ffee7e9b790 _sigtramp + 18446744071596723280
/Users/liuliu/Snapchat/Dev/phantom/6  clang-10                 0x000000010bfdff14 clang::CodeGen::CodeGenTypes::arrangeObjCMethodDeclaration(clang::ObjCMethodDecl const*) + 52
/Users/liuliu/Snapchat/Dev/phantom/7  clang-10                 0x000000010c1f6bcd (anonymous namespace)::CGObjCCommonMac::GenerateDirectMethod(clang::ObjCMethodDecl const*, clang::ObjCContainerDecl const*) + 285
/Users/liuliu/Snapchat/Dev/phantom/8  clang-10                 0x000000010c1f63e9 (anonymous namespace)::CGObjCCommonMac::EmitMessageSend(clang::CodeGen::CodeGenFunction&, clang::CodeGen::ReturnValueSlot, clang::QualType, clang::Selector, llvm::Value*, clang::QualType, bool, clang::CodeGen::CallArgList const&, clang::ObjCMethodDecl const*, clang::ObjCInterfaceDecl const*, (anonymous namespace)::ObjCCommonTypesHelper const&) + 1401
/Users/liuliu/Snapchat/Dev/phantom/9  clang-10                 0x000000010c205b14 (anonymous namespace)::CGObjCNonFragileABIMac::GenerateMessageSend(clang::CodeGen::CodeGenFunction&, clang::CodeGen::ReturnValueSlot, clang::QualType, clang::Selector, llvm::Value*, clang::CodeGen::CallArgList const&, clang::ObjCInterfaceDecl const*, clang::ObjCMethodDecl const*) + 644

It indeed won't crash any more in trivial examples. I need to have some other time to reduce my local example to a reasonable size.

This seems like the problem I just fixed, did you use the latest patch? because the problem was that we didn't bother to emit the implicit arguments for just prototypes before as it was only used by CodeGen of the implementations. This patch is supposed to add it to all declarations now, so I fail to understand how it can be missing anywhere ?

[liuliu]

clang/lib/Sema/SemaDeclObjC.cpp

4839

Yeah, I applied this change. As I said, for my trivial example it does work. But for my more thorough example with @property (direct) etc, it doesn't. I can dig into more later today or Monday. I just got everything work (with another more hackier fix) and would first to evaluate this as a viable path first before commit more time to debug this.

We probably can also just have a follow-up fix for that, depends on if reviewers thought this is blocking or not. I personally think not.

[MadCoder]

clang/lib/Sema/SemaDeclObjC.cpp

4839

Huh I could have seen why property methods would have an issue but they seem to have gotten their directness properly anyway (I'll add them to the CG test for good measure while I debug the tests I broke).

so I'll need a reduced test case from you I can't seem to find an easy way to break it indeed.

[MadCoder]

woops a bogus hunk went in

[MadCoder]

clang/lib/Sema/SemaDeclObjC.cpp

4839

I just added a test using a direct property with both a synthesized and a manual one, and it seems to work for me (?)

[arphaman]

clang/lib/Serialization/ASTWriterDecl.cpp

674 ↗

(On Diff #232638)

@MadCoder Given the fact that you're now setting SelfDecl and CmdDecl for declarations, they need to be serialized properly. I would suggest the following change to the writer:

bool HasBody = D->getBody() != nullptr;
Record.push_back(HasBody);
if (HasBody)
  Record.AddStmt(D->getBody());
Record.AddDeclRef(D->getSelfDecl());
Record.AddDeclRef(D-> getCmdDecl());

The AST reader should be updated accordingly.

[MadCoder]

@liuliu I wouldn't be surprised this addresses your issues if snapchat is using PCH

[MadCoder]

clang/lib/Serialization/ASTWriterDecl.cpp

674 ↗

(On Diff #232638)

hah was the source of the last etst failure I was chasing, thanks!

[arphaman]

In D71091#1773494, @MadCoder wrote:

@liuliu I wouldn't be surprised this addresses your issues if snapchat is using PCH

This also impacts modules, so builds that use could've been affected.

[arphaman]

LGTM, thanks

[liuliu]

We don't use PCH :) I arc patched the latest diff, still have that crash. More strangely, I tried this latest diff on previously simplistic test case and it crashes now:

#import <Foundation/Foundation.h>

@interface SCTestMainEntity : NSObject

@property (nonatomic, readonly, direct) int date;

- (instancetype)initWithDate:(int)date;

@end

int main(void)
{
    SCTestMainEntity *entity = [[SCTestMainEntity alloc] initWithDate:10];
    printf("%d\n", entity.date);
    return 0;
}

[MadCoder]

In D71091#1775722, @liuliu wrote:

We don't use PCH :) I arc patched the latest diff, still have that crash. More strangely, I tried this latest diff on previously simplistic test case and it crashes now:

#import <Foundation/Foundation.h>

@interface SCTestMainEntity : NSObject

@property (nonatomic, readonly, direct) int date;

- (instancetype)initWithDate:(int)date;

@end

int main(void)
{
    SCTestMainEntity *entity = [[SCTestMainEntity alloc] initWithDate:10];
    printf("%d\n", entity.date);
    return 0;
}

Ugh, you're right, and that's because my test is missing one case. doh.

you can work-around by re-redefining -(int)date __attribute__((objc_direct)); next to the property. it works in the test beacuse the call site picks up the implementation to form the call. sigh.

[liuliu]

No problem! I have a custom patch that isn't as nice but get the job done. Let me know when you put new diff out and I can test it on our codebase again.

[MadCoder]

In D71091#1776009, @liuliu wrote:

No problem! I have a custom patch that isn't as nice but get the job done. Let me know when you put new diff out and I can test it on our codebase again.

https://reviews.llvm.org/D71226/new/ is the incremental patch as this one is already merged

clang/lib/Sema/SemaDeclObjC.cpp

     cast<DeclContext>(ClassDecl)->addDecl(ObjCMethod);
   }
 
+  if (ObjCMethod->isDirectMethod()) {
+    ObjCMethod->createImplicitParams(Context, ObjCMethod->getClassInterface());
+  }
+
   if (PrevMethod) {
     // You can never have two method definitions with the same name.
     Diag(ObjCMethod->getLocation(), diag::err_duplicate_method_decl)

clang/test/CodeGenObjC/direct-method.m

 // CHECK-LABEL: define hidden void @"\01-[Foo setGetDynamic_setDirect:]"(
 // CHECK-LABEL: define internal void @"\01-[Foo .cxx_destruct]"(
 @end
+
+int useRoot(Root *r) {
+  // CHEC-LABEL: define i32 @useRoot
+  // CHECK: %call = call i32 bitcast {{.*}} @"\01-[Root getInt]"
+  return [r getInt];
+}