@erik.pilkington review feedback

In D73755#1874962, @plotfi wrote:

@MadCoder Could there be a way to mark objc_direct methods as something like attribute((visibility("default"))) or __declspec(dllexport) to denote that a given method should be directly callable but still retain the metadata to be called through an objc_msgSend from an external dylib? Could that help to boost adoption?

damn you tabs!

@dexonsmith here, I still hook the same method but do it in a more LLVM-approved way ;)

In D73208#1836789, @dexonsmith wrote:

In D73208#1836722, @MadCoder wrote:

In D73208#1836704, @dexonsmith wrote:

In D73208#1835264, @MadCoder wrote:

In D73208#1835051, @dexonsmith wrote:

Why isn't a similar dance needed for non-direct methods?

because non direct methods do not need an llvm::Function to be synthesized at the call-site. direct methods do, and they form one with the type of the declaration they see. Then that same llvm::Function is used when you CodeGen the Implementation, so if there's a mismatch, sadness ensues because the LLVM IR verifier will notice the discrepancy between the declared return type of the function and the actual types coming out of the ret codepaths.

Regular obj-C methods use the _implementation_ types for the codegen (the declaration(s) aren't even consulted) and I want to stick at what obj-c does as much as I can.

(as a data point: If you use obj-C types with C functions, the type of the first declaration seen is used instead).

Okay, that makes sense to me.

Another solution would be to change IRGen for the implementation: if the declaration already exists (getFunction), do a bitcast + RAUW dance to fix it up (and update the DirectMethodDefinitions table). WDYT?

I didn't want to do that because that would mean that the type used for the implementation would depart from dynamic Objective-C methods, and it feels that it shouldn't. hence I took this option.

I think we're talking across each other. The idea is check the type when generating the implementation; if it's not correct, you fix it (need to update existing uses to bitcast). So the type used for the implementation would match dynamic methods.

In D73208#1836704, @dexonsmith wrote:

In D73208#1835264, @MadCoder wrote:

In D73208#1835051, @dexonsmith wrote:

Why isn't a similar dance needed for non-direct methods?

because non direct methods do not need an llvm::Function to be synthesized at the call-site. direct methods do, and they form one with the type of the declaration they see. Then that same llvm::Function is used when you CodeGen the Implementation, so if there's a mismatch, sadness ensues because the LLVM IR verifier will notice the discrepancy between the declared return type of the function and the actual types coming out of the ret codepaths.

Regular obj-C methods use the _implementation_ types for the codegen (the declaration(s) aren't even consulted) and I want to stick at what obj-c does as much as I can.

(as a data point: If you use obj-C types with C functions, the type of the first declaration seen is used instead).

Okay, that makes sense to me.

Another solution would be to change IRGen for the implementation: if the declaration already exists (getFunction), do a bitcast + RAUW dance to fix it up (and update the DirectMethodDefinitions table). WDYT?

In D73208#1835051, @dexonsmith wrote:

Why isn't a similar dance needed for non-direct methods?

with the test for real

adding the non ARC test I forgot to add

fixed @ahatanak feedback

Added some more tests to make sure it works along inheritance chains as expected

Removed the category from the direct method name mangling as discussed with @aprantl

I had forgotten to clang format and left some tabs here... some day I'll fix my vim config

Addressed @aprantl review feedback.

Small update because I uploaded before the full completion of the test-suite and somehow it's sentient and punished me... (some diagnostics were too verbose and stuttering things with Sema::CheckObjCMethodOverrides).

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.

In D71226#1776038, @rjmccall wrote:

It'd be nice if we didn't have to synthesize parameters for all declarations when we're just handling an @interface, but we can improve that in follow-ups.

@liuliu that fixes your test case (which I reproduced in the CG test)

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

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

woops a bogus hunk went in

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.

just added all the context this time (-W)

oh hah, thanks :)

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

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

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

Diff against previous is:

when running the full test-suite before sending the patch, and it broke tests because some loads are now ordered differently :(
yay.

reverted the hunk about "direct methods" note.

Updated for the new round of comments, with added tests and Sema checks errors for:

• messaging super
• messaging a nullable Class expression

Implemented all the tests @rjmccall wanted (and then some)

In D69991#1744979, @aprantl wrote:

Is it intentional that the direct method names use the exact same symbol namespace (\01-[class message]) as "real" Objective-C methods? Could that be a problem? Should we use a slightly different naming scheme?

Beefed up the tests, addressed the selfValue related issue.

Updated clang/test/Misc/pragma-attribute-supported-attributes-list.test that I had forgotten.

Updated the patch to restrict objc_direct to methods and use objc_direct_members for containers, and several diagnostics improvements (especially in the vicinity of properties and the GNU runtime + tests).