[objc_direct] fix codegen for mismatched Decl/Impl return types

[MadCoder]

Summary

For non direct methods, the codegen uses the type of the Implementation.
Because Objective-C rules allow some differences between the Declaration
and Implementation return types, when the Implementation is in this
translation unit, the type of the Implementation should be preferred to
emit the Function over the Declaration.

Radar-Id: rdar://problem/58797748

[dexonsmith]

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

[MadCoder]

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

[dexonsmith]

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?

[MadCoder]

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.

[dexonsmith]

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.

[MadCoder]

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.

ah I see, I don't know how to do that, I couldn't figure out how to fix the function declaration, if you want to give it a stab I'd love it.

[dexonsmith]

In D73208#1837909, @MadCoder wrote:

In D73208#1836789, @dexonsmith wrote:

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.

ah I see, I don't know how to do that, I couldn't figure out how to fix the function declaration, if you want to give it a stab I'd love it.

I missed that question until now. This uses llvm::Value::replaceAllUsesWith, if you git grep through clang you'll find examples.

Here's one from CodeGenFunction::AddInitializerToStaticVarDecl:

// The initializer may differ in type from the global. Rewrite
// the global to match the initializer.  (We have to do this
// because some types, like unions, can't be completely represented
// in the LLVM type system.)
if (GV->getType()->getElementType() != Init->getType()) {
  llvm::GlobalVariable *OldGV = GV;

  GV = new llvm::GlobalVariable(CGM.getModule(), Init->getType(),
                                OldGV->isConstant(),
                                OldGV->getLinkage(), Init, "",
                                /*InsertBefore*/ OldGV,
                                OldGV->getThreadLocalMode(),
                         CGM.getContext().getTargetAddressSpace(D.getType()));
  GV->setVisibility(OldGV->getVisibility());
  GV->setDSOLocal(OldGV->isDSOLocal());
  GV->setComdat(OldGV->getComdat());

  // Steal the name of the old global
  GV->takeName(OldGV);

  // Replace all uses of the old global with the new global
  llvm::Constant *NewPtrForOldDecl =
  llvm::ConstantExpr::getBitCast(GV, OldGV->getType());
  OldGV->replaceAllUsesWith(NewPtrForOldDecl);

  // Erase the old global, since it is no longer used.
  OldGV->eraseFromParent();
}

This example is analogous to your case. The pattern is:

1. Create the new thing (with no name, or with a temporary/bad name).
2. Steal the name of the old thing.
3. Bitcast from the new thing to the old type.
4. RAUW to replace all uses of the old thing with the bitcast-of-new-thing from (3).
5. Delete the old thing.

The RAUW isn't going to fix up the DenseMap you have on the side though, so you'll need to handle that explicitly; likely during step (4) makes sense.

[MadCoder]

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

It works with minimap perf impect because the only case we call GenerateDirectMethod with an Implementation is if we're about to codegen the body, so the case when you build against a header without seing the @implementation will not be affected perf-wise.

[MadCoder]

damn you tabs!

[dexonsmith]

LGTM, with one style nitpick.

clang/lib/CodeGen/CGObjCMac.cpp

4049–4051

I think the LLVM style is to leave out these braces.

clang/lib/CodeGen/CGObjCMac.cpp

llvm::Function *CGObjCCommonMac::GenerateMethod(const ObjCMethodDecl *OMD,
   MethodDefinitions.insert(std::make_pair(OMD, Method));
 
   return Method;
 }
 
 llvm::Function *
 CGObjCCommonMac::GenerateDirectMethod(const ObjCMethodDecl *OMD,
                                       const ObjCContainerDecl *CD) {
-  auto I = DirectMethodDefinitions.find(OMD->getCanonicalDecl());
-  if (I != DirectMethodDefinitions.end())
+  auto *COMD = OMD->getCanonicalDecl();
+  auto I = DirectMethodDefinitions.find(COMD);
+  llvm::Function *OldFn = nullptr, *Fn = nullptr;
+
+  if (I != DirectMethodDefinitions.end()) {
+    // Objective-C allows for the declaration and implementation types
+    // to differ slightly.
+    //
+    // If we're being asked for the Function associated for a method
+    // implementation, a previous value might have been cached
+    // based on the type of the canonical declaration.
+    //
+    // If these do not match, then we'll replace this function with
+    // a new one that has the proper type below.
+    if (!OMD->getBody() || COMD->getReturnType() == OMD->getReturnType())
       return I->second;
-
-  SmallString<256> Name;
-  GetNameForMethod(OMD, CD, Name, /*ignoreCategoryNamespace*/true);
+    OldFn = I->second;
+  }
 
   CodeGenTypes &Types = CGM.getTypes();
   llvm::FunctionType *MethodTy =
     Types.GetFunctionType(Types.arrangeObjCMethodDeclaration(OMD));
-  llvm::Function *Method =
-      llvm::Function::Create(MethodTy, llvm::GlobalValue::ExternalLinkage,
+
+  if (OldFn) {
+    Fn = llvm::Function::Create(MethodTy, llvm::GlobalValue::ExternalLinkage,
+                                "", &CGM.getModule());
+    Fn->takeName(OldFn);
+    OldFn->replaceAllUsesWith(
+        llvm::ConstantExpr::getBitCast(Fn, OldFn->getType()));
+    OldFn->eraseFromParent();
+
+    // Replace the cached function in the map.
+    I->second = Fn;
+  } else {
+    SmallString<256> Name;
+    GetNameForMethod(OMD, CD, Name, /*ignoreCategoryNamespace*/ true);
+
+    Fn = llvm::Function::Create(MethodTy, llvm::GlobalValue::ExternalLinkage,
                                 Name.str(), &CGM.getModule());
-  DirectMethodDefinitions.insert(std::make_pair(OMD->getCanonicalDecl(), Method));
+    DirectMethodDefinitions.insert(std::make_pair(COMD, Fn));
+  }
 
-  return Method;
+  return Fn;
 }
 
 void CGObjCCommonMac::GenerateDirectMethodPrologue(
     CodeGenFunction &CGF, llvm::Function *Fn, const ObjCMethodDecl *OMD,
     const ObjCContainerDecl *CD) {
   auto &Builder = CGF.Builder;
   bool ReceiverCanBeNull = true;
   auto selfAddr = CGF.GetAddrOfLocalVar(OMD->getSelfDecl());

clang/test/CodeGenObjC/direct-method-ret-mismatch.m

+// RUN: %clang_cc1 -emit-llvm -fobjc-arc -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
+
+__attribute__((objc_root_class))
+@interface Root
+- (Root *)method __attribute__((objc_direct));
+@end
+
+@implementation Root
+// CHECK-LABEL: define internal i8* @"\01-[Root something]"(
+- (id)something {
+  // CHECK: %{{[^ ]*}} = call {{.*}} @"\01-[Root method]"
+  return [self method];
+}
+
+// CHECK-LABEL: define hidden i8* @"\01-[Root method]"(
+- (id)method {
+  return self;
+}
+@end