[GNU ObjC] Fix a regression listing methods twice.

[theraven]

Summary

Methods synthesized from declared properties were being added to the
method lists twice. This came from the change to list them in the
class's method list, which missed removing the place in CGObjCGNU that
added them again.

[theraven]

This was caught with the GNUstep runtime's test suite, which apparently had not been run with anything newer than clang 8 until recently. With this patch, all of the runtime's tests now pass again (a few others failed in 10 but appear to have been fixed in 11 or 12). We've now configured our CI to test with the nightly builds on Linux, so should catch these things sooner in the future.

[theraven]

I'd like to get this into the 11 point release, so it would be good to have a review...

[lanza]

LGTM!

[rjmccall]

Functionally LGTM. I don't know if 11 is still taking changes, but if it is, you have code-owner approval.

clang/test/CodeGenObjC/gnu-method-only-once.m

9

Comment trails off. Not sure the bug history is necessary here; you could just make this part of the commit message.

clang/lib/CodeGen/CGObjCGNU.cpp

void CGObjCGNU::GenerateClass(const ObjCImplementationDecl *OID) {
   SmallVector<const ObjCMethodDecl*, 16> InstanceMethods;
   InstanceMethods.insert(InstanceMethods.begin(), OID->instmeth_begin(),
       OID->instmeth_end());
 
   SmallVector<const ObjCMethodDecl*, 16> ClassMethods;
   ClassMethods.insert(ClassMethods.begin(), OID->classmeth_begin(),
       OID->classmeth_end());
 
-  // Collect the same information about synthesized properties, which don't
-  // show up in the instance method lists.
-  for (auto *propertyImpl : OID->property_impls())
-    if (propertyImpl->getPropertyImplementation() ==
-        ObjCPropertyImplDecl::Synthesize) {
-      auto addPropertyMethod = [&](const ObjCMethodDecl *accessor) {
-        if (accessor)
-          InstanceMethods.push_back(accessor);
-      };
-      addPropertyMethod(propertyImpl->getGetterMethodDecl());
-      addPropertyMethod(propertyImpl->getSetterMethodDecl());
-    }
-
   llvm::Constant *Properties = GeneratePropertyList(OID, ClassDecl);
 
   // Collect the names of referenced protocols
   auto RefProtocols = ClassDecl->protocols();
   auto RuntimeProtocols =
       GetRuntimeProtocolList(RefProtocols.begin(), RefProtocols.end());
   SmallVector<std::string, 16> Protocols;
   for (const auto *I : RuntimeProtocols)

clang/test/CodeGenObjC/gnu-method-only-once.m

+// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -S -emit-llvm -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s -check-prefix=CHECK-NEW
+// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -S -emit-llvm -fobjc-runtime=gnustep-1.8 -o - %s | FileCheck %s -check-prefix=CHECK-OLD
+
+// Clang 9 or 10 changed the handling of method lists so that methods provided
+// from synthesised properties showed up in the method list, where previously
+// CGObjCGNU had to collect them and merge them.  One of the places where this
+// merging happened was missed in the move and so we ended up emitting two
+// copies of method metadata for declared properties.
+
+// This class has only instance properties and only one pair of synthesized
+// methods from the property and so we should synthesize only one method list,
+// with precisely two methods on it.
+@interface X
+@property (retain) id iProp;
+@end
+
+@implementation X
+@synthesize iProp;
+@end
+
+// Check that the method list has precisely 2 methods.
+// CHECK-NEW: @.objc_method_list = internal global { i8*, i32, i64, [2 x
+// CHECK-OLD: @.objc_method_list = internal global { i8*, i32, [2 x