[PATCH] D91874: [GNU ObjC] Fix a regression listing methods twice.
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd1ed67037de6: [GNU ObjC] Fix a regression listing methods
twice. (authored by theraven).
Changed prior to commit:
https://reviews.llvm.org/D91874?vs=306704=308583#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91874/new/
https://reviews.llvm.org/D91874
Files:
clang/lib/CodeGen/CGObjCGNU.cpp
clang/test/CodeGenObjC/gnu-method-only-once.m
Index: clang/test/CodeGenObjC/gnu-method-only-once.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/gnu-method-only-once.m
@@ -0,0 +1,23 @@
+// 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
Index: clang/lib/CodeGen/CGObjCGNU.cpp
===
--- clang/lib/CodeGen/CGObjCGNU.cpp
+++ clang/lib/CodeGen/CGObjCGNU.cpp
@@ -3512,19 +3512,6 @@
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
Index: clang/test/CodeGenObjC/gnu-method-only-once.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/gnu-method-only-once.m
@@ -0,0 +1,23 @@
+// 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
Index: clang/lib/CodeGen/CGObjCGNU.cpp
===
--- clang/lib/CodeGen/CGObjCGNU.cpp
+++ clang/lib/CodeGen/CGObjCGNU.cpp
@@ -3512,19 +3512,6 @@
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);
- };
-
[PATCH] D91874: [GNU ObjC] Fix a regression listing methods twice.
rjmccall added a comment. Functionally LGTM. I don't know if 11 is still taking changes, but if it is, you have code-owner approval. Comment at: clang/test/CodeGenObjC/gnu-method-only-once.m:8 +// merging happened was missed in the move and so we ended up emitting two +// copies of method metadata for everything that appeared in the + Comment trails off. Not sure the bug history is necessary here; you could just make this part of the commit message. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91874/new/ https://reviews.llvm.org/D91874 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91874: [GNU ObjC] Fix a regression listing methods twice.
lanza accepted this revision. lanza added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91874/new/ https://reviews.llvm.org/D91874 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91874: [GNU ObjC] Fix a regression listing methods twice.
theraven added a comment. I'd like to get this into the 11 point release, so it would be good to have a review... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91874/new/ https://reviews.llvm.org/D91874 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91874: [GNU ObjC] Fix a regression listing methods twice.
theraven added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91874/new/ https://reviews.llvm.org/D91874 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D91874: [GNU ObjC] Fix a regression listing methods twice.
theraven created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
theraven requested review of this revision.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D91874
Files:
clang/lib/CodeGen/CGObjCGNU.cpp
clang/test/CodeGenObjC/gnu-method-only-once.m
Index: clang/test/CodeGenObjC/gnu-method-only-once.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/gnu-method-only-once.m
@@ -0,0 +1,23 @@
+// 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 everything that appeared in the
+
+// 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
Index: clang/lib/CodeGen/CGObjCGNU.cpp
===
--- clang/lib/CodeGen/CGObjCGNU.cpp
+++ clang/lib/CodeGen/CGObjCGNU.cpp
@@ -3512,19 +3512,6 @@
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
Index: clang/test/CodeGenObjC/gnu-method-only-once.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/gnu-method-only-once.m
@@ -0,0 +1,23 @@
+// 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 everything that appeared in the
+
+// 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
Index: clang/lib/CodeGen/CGObjCGNU.cpp
===
--- clang/lib/CodeGen/CGObjCGNU.cpp
+++ clang/lib/CodeGen/CGObjCGNU.cpp
@@ -3512,19 +3512,6 @@
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);
- };
-
