Page MenuHomePhabricator

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

Authored by MadCoder on Dec 5 2019, 2:24 PM.

Details

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

Diff Detail

Event Timeline

MadCoder created this revision.Dec 5 2019, 2:24 PM

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

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

need to fix it again sigh.

liuliu added a comment.Dec 5 2019, 2:50 PM

Did you mean the linkage issue?

MadCoder edited the summary of this revision. (Show Details)Dec 5 2019, 3:32 PM
MadCoder updated this revision to Diff 232460.Dec 5 2019, 3:35 PM

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

@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.

oh hah, thanks :)

MadCoder updated this revision to Diff 232491.Dec 5 2019, 8:31 PM
MadCoder edited the summary of this revision. (Show Details)
MadCoder added a reviewer: jfb.Dec 5 2019, 8:39 PM
jfb added a comment.Dec 6 2019, 9:07 AM

You should upload patches with context :)

clang/lib/CodeGen/CGObjCMac.cpp
4030

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

arphaman accepted this revision.Dec 6 2019, 9:39 AM

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

This revision is now accepted and ready to land.Dec 6 2019, 9:39 AM
MadCoder updated this revision to Diff 232602.Dec 6 2019, 10:05 AM
MadCoder marked 2 inline comments as done.

just added all the context this time (-W)

clang/lib/CodeGen/CGObjCMac.cpp
4030

this is a pair this doesn't work

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 requested changes to this revision.Dec 6 2019, 11:30 AM

@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.

This revision now requires changes to proceed.Dec 6 2019, 11:30 AM

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 marked an inline comment as done.Dec 6 2019, 12:01 PM
liuliu added inline comments.
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 marked an inline comment as done.Dec 6 2019, 12:25 PM
MadCoder added inline comments.
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 updated this revision to Diff 232638.Dec 6 2019, 1:21 PM
MadCoder edited the summary of this revision. (Show Details)
MadCoder updated this revision to Diff 232641.Dec 6 2019, 1:34 PM
MadCoder marked an inline comment as done.

woops a bogus hunk went in

MadCoder added inline comments.Dec 6 2019, 1:35 PM
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 added inline comments.Dec 6 2019, 1:54 PM
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 updated this revision to Diff 232644.Dec 6 2019, 2:05 PM
MadCoder marked an inline comment as done.

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

MadCoder added inline comments.Dec 6 2019, 2:07 PM
clang/lib/Serialization/ASTWriterDecl.cpp
674 ↗(On Diff #232638)

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

@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 accepted this revision.Dec 6 2019, 2:12 PM

LGTM, thanks

This revision is now accepted and ready to land.Dec 6 2019, 2:12 PM
MadCoder marked an inline comment as done.Dec 6 2019, 2:14 PM
This revision was automatically updated to reflect the committed changes.

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;
}

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 added a comment.Dec 9 2019, 1:39 PM

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.

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