[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd32c685e1012: [modules] Merge equivalent extensions and 
diagnose ivar redeclarations for… (authored by vsapsai).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/DeclObjC.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/merge-extension-ivars.m
  clang/test/Modules/redecl-ivars.m

Index: clang/test/Modules/redecl-ivars.m
===
--- /dev/null
+++ clang/test/Modules/redecl-ivars.m
@@ -0,0 +1,166 @@
+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-extension.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-extension.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-ivars-number.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-ivars-number.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-methods-protocols.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-methods-protocols.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-subclass.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-subclass.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-implementation.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-implementation.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Same class extensions with the same ivars but from different modules aren't considered
+// an error and they are merged together. Test that differences in extensions and/or ivars
+// are still reported as errors.
+
+//--- include/Interfaces.h
+@interface NSObject @end
+@interface ObjCInterface : NSObject
+@end
+@interface ObjCInterfaceLevel2 : ObjCInterface
+@end
+
+@protocol Protocol1 @end
+@protocol Protocol2 @end
+
+//--- include/IvarsInExtensions.h
+#import 
+@interface ObjCInterface() {
+  int ivarName;
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 3;
+}
+@end
+
+//--- include/IvarsInExtensionsWithMethodsProtocols.h
+#import 
+@interface ObjCInterface() {
+  int methodRelatedIvar;
+}
+- (void)test;
+@end
+@interface ObjCInterfaceLevel2()  {
+  int protocolRelatedIvar;
+}
+@end
+
+//--- include/IvarInImplementation.h
+#import 
+@implementation ObjCInterface {
+  int ivarName;
+}
+@end
+
+//--- include/module.modulemap
+module Interfaces {
+  header "Interfaces.h"
+  export *
+}
+module IvarsInExtensions {
+  header "IvarsInExtensions.h"
+  export *
+}
+module IvarsInExtensionsWithMethodsProtocols {
+  header "IvarsInExtensionsWithMethodsProtocols.h"
+  export *
+}
+module IvarInImplementation {
+  header "IvarInImplementation.h"
+  export *
+}
+
+
+//--- test-mismatch-in-extension.m
+// Different ivars with the same name aren't mergeable and constitute an error.
+#import 
+@interface ObjCInterface() {
+  float ivarName; // expected-note {{previous definition is here}}
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 5; // expected-note {{previous definition is here}}
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterfaceLevel2
+@end
+
+
+//--- test-mismatch-in-ivars-number.m
+// Extensions with different amount of ivars aren't considered to be the same.
+#import 
+@interface ObjCInterface() {
+  int ivarName; // expected-note {{previous definition is here}}
+  float anotherIvar;
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterface
+@end
+
+
+//--- test-mismatch-in-methods-protocols.m
+// Extensions with different methods or protocols aren't considered to be the same.
+#import 
+@interface

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 425303.
vsapsai added a comment.

Handle in `VisitObjCIvarDecl` only redeclarations in extensions.

Interface+interface redeclarations should be handled in VisitObjCInterfaceDecl
and implementation+implementation in VisitObjCImplementationDecl.

Checked that we don't emit duplicate ivar metadata because unlike extensions,
there can be only a single [non-category] implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/DeclObjC.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/merge-extension-ivars.m
  clang/test/Modules/redecl-ivars.m

Index: clang/test/Modules/redecl-ivars.m
===
--- /dev/null
+++ clang/test/Modules/redecl-ivars.m
@@ -0,0 +1,166 @@
+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-extension.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-extension.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-ivars-number.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-ivars-number.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-methods-protocols.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-methods-protocols.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-subclass.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-subclass.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-implementation.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-implementation.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Same class extensions with the same ivars but from different modules aren't considered
+// an error and they are merged together. Test that differences in extensions and/or ivars
+// are still reported as errors.
+
+//--- include/Interfaces.h
+@interface NSObject @end
+@interface ObjCInterface : NSObject
+@end
+@interface ObjCInterfaceLevel2 : ObjCInterface
+@end
+
+@protocol Protocol1 @end
+@protocol Protocol2 @end
+
+//--- include/IvarsInExtensions.h
+#import 
+@interface ObjCInterface() {
+  int ivarName;
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 3;
+}
+@end
+
+//--- include/IvarsInExtensionsWithMethodsProtocols.h
+#import 
+@interface ObjCInterface() {
+  int methodRelatedIvar;
+}
+- (void)test;
+@end
+@interface ObjCInterfaceLevel2()  {
+  int protocolRelatedIvar;
+}
+@end
+
+//--- include/IvarInImplementation.h
+#import 
+@implementation ObjCInterface {
+  int ivarName;
+}
+@end
+
+//--- include/module.modulemap
+module Interfaces {
+  header "Interfaces.h"
+  export *
+}
+module IvarsInExtensions {
+  header "IvarsInExtensions.h"
+  export *
+}
+module IvarsInExtensionsWithMethodsProtocols {
+  header "IvarsInExtensionsWithMethodsProtocols.h"
+  export *
+}
+module IvarInImplementation {
+  header "IvarInImplementation.h"
+  export *
+}
+
+
+//--- test-mismatch-in-extension.m
+// Different ivars with the same name aren't mergeable and constitute an error.
+#import 
+@interface ObjCInterface() {
+  float ivarName; // expected-note {{previous definition is here}}
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 5; // expected-note {{previous definition is here}}
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterfaceLevel2
+@end
+
+
+//--- test-mismatch-in-ivars-number.m
+// Extensions with different amount of ivars aren't considered to be the same.
+#import 
+@interface ObjCInterface() {
+  int ivarName; // expected-note {{previous definition is here}}
+  float anotherIvar;
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterface
+@

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1261-1263
+  Reader.Diag(IVD->getLocation(), diag::err_duplicate_ivar_declaration)
+  << II;
+  Reader.Diag(PrevIvar->getLocation(), diag::note_previous_definition);

We emit diagnostic for
```lang=objective-c
@implementation TestClass {
@public
  struct { int z; } implementationIvar;
}
@end
```

from merge-anon-record-definition-in-objc.m because we handle only 
`ObjCCategoryDecl` and not `ObjCImplementationDecl`. It is very uncommon to put 
implementation in a header file but I still need to check we don't emit 
duplicate ivar metadata in this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 425093.
vsapsai added a comment.

Rebase and update test to use `ptr` instead of `i64*`. Also now the added test 
is failing for `implementationIvar`, investigating that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/DeclObjC.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/merge-extension-ivars.m
  clang/test/Modules/redecl-ivars.m

Index: clang/test/Modules/redecl-ivars.m
===
--- /dev/null
+++ clang/test/Modules/redecl-ivars.m
@@ -0,0 +1,166 @@
+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-extension.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-extension.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-ivars-number.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-ivars-number.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-methods-protocols.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-methods-protocols.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-subclass.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-subclass.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-implementation.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-implementation.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Same class extensions with the same ivars but from different modules aren't considered
+// an error and they are merged together. Test that differences in extensions and/or ivars
+// are still reported as errors.
+
+//--- include/Interfaces.h
+@interface NSObject @end
+@interface ObjCInterface : NSObject
+@end
+@interface ObjCInterfaceLevel2 : ObjCInterface
+@end
+
+@protocol Protocol1 @end
+@protocol Protocol2 @end
+
+//--- include/IvarsInExtensions.h
+#import 
+@interface ObjCInterface() {
+  int ivarName;
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 3;
+}
+@end
+
+//--- include/IvarsInExtensionsWithMethodsProtocols.h
+#import 
+@interface ObjCInterface() {
+  int methodRelatedIvar;
+}
+- (void)test;
+@end
+@interface ObjCInterfaceLevel2()  {
+  int protocolRelatedIvar;
+}
+@end
+
+//--- include/IvarInImplementation.h
+#import 
+@implementation ObjCInterface {
+  int ivarName;
+}
+@end
+
+//--- include/module.modulemap
+module Interfaces {
+  header "Interfaces.h"
+  export *
+}
+module IvarsInExtensions {
+  header "IvarsInExtensions.h"
+  export *
+}
+module IvarsInExtensionsWithMethodsProtocols {
+  header "IvarsInExtensionsWithMethodsProtocols.h"
+  export *
+}
+module IvarInImplementation {
+  header "IvarInImplementation.h"
+  export *
+}
+
+
+//--- test-mismatch-in-extension.m
+// Different ivars with the same name aren't mergeable and constitute an error.
+#import 
+@interface ObjCInterface() {
+  float ivarName; // expected-note {{previous definition is here}}
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 5; // expected-note {{previous definition is here}}
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterfaceLevel2
+@end
+
+
+//--- test-mismatch-in-ivars-number.m
+// Extensions with different amount of ivars aren't considered to be the same.
+#import 
+@interface ObjCInterface() {
+  int ivarName; // expected-note {{previous definition is here}}
+  float anotherIvar;
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterface
+@end
+
+
+//--- test-mismatch-in-methods-protocols.m
+// Extensions with different methods or protocols aren't considered to be the same.
+#import 
+@interface ObjCInterface() {
+  int methodRelatedIvar; // expec

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

OK, if it is prohibited. It looks good to me. Please wait for a few days before 
landing in case there are other comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 6 inline comments as done.
vsapsai added a comment.

In D121177#3371832 , @ChuanqiXu wrote:

> I see it is guaranteed by `lookupInstanceVariable(Identifier)` now. I don't 
> know much about ObjC. But I feel a little bit strange. Is it possible that 
> two distinct variable with the same identifier in ObjC? If it is impossible, 
> I think it might be OK.

Two distinct ivars with the same name in the same class are prohibited. But 
there are a bunch of situations when ivars don't end up in the same class. For 
example, the most common case (and a huge reason for non-fragile ABI) is you 
have `@interface MyView: NSView` that has ivar `color`. If NSView decides to 
add its own ivar `color` in `@implementation`, it won't break your MyView and 
your binary will work without recompilation. That is roughly achieved by 
treating those ivars as `MyView.color` and `NSView.color` (it is very hand-wavy 
over-simplified explanation). But within the same class ivar redeclarations are 
prohibited. Without splitting everything into modules, we don't allow 
redeclarations in

  @interface TestSubject {
int a;
  }
  @end
  
  @interface TestSubject() {
int a;  // <- error as it is a redeclaration
int b;
int c;
  }
  @end
  
  @interface TestSubject() {
int b;  // <- error as it is a redeclaration
  }
  @end
  
  @implementation TestSubject {
int c;  // <- error as it is a redeclaration
  }
  @end


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 414780.
vsapsai added a comment.

Address comments and make the tests use non-fragile ABI (it's not default 
everywhere).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/DeclObjC.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/merge-extension-ivars.m
  clang/test/Modules/redecl-ivars.m

Index: clang/test/Modules/redecl-ivars.m
===
--- /dev/null
+++ clang/test/Modules/redecl-ivars.m
@@ -0,0 +1,166 @@
+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-extension.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-extension.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-ivars-number.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-ivars-number.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-methods-protocols.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-mismatch-in-methods-protocols.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-subclass.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-subclass.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-implementation.m
+// RUN: %clang_cc1 -fsyntax-only -fobjc-runtime=macosx-10.9 -verify -I%t/include %t/test-redecl-in-implementation.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Same class extensions with the same ivars but from different modules aren't considered
+// an error and they are merged together. Test that differences in extensions and/or ivars
+// are still reported as errors.
+
+//--- include/Interfaces.h
+@interface NSObject @end
+@interface ObjCInterface : NSObject
+@end
+@interface ObjCInterfaceLevel2 : ObjCInterface
+@end
+
+@protocol Protocol1 @end
+@protocol Protocol2 @end
+
+//--- include/IvarsInExtensions.h
+#import 
+@interface ObjCInterface() {
+  int ivarName;
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 3;
+}
+@end
+
+//--- include/IvarsInExtensionsWithMethodsProtocols.h
+#import 
+@interface ObjCInterface() {
+  int methodRelatedIvar;
+}
+- (void)test;
+@end
+@interface ObjCInterfaceLevel2()  {
+  int protocolRelatedIvar;
+}
+@end
+
+//--- include/IvarInImplementation.h
+#import 
+@implementation ObjCInterface {
+  int ivarName;
+}
+@end
+
+//--- include/module.modulemap
+module Interfaces {
+  header "Interfaces.h"
+  export *
+}
+module IvarsInExtensions {
+  header "IvarsInExtensions.h"
+  export *
+}
+module IvarsInExtensionsWithMethodsProtocols {
+  header "IvarsInExtensionsWithMethodsProtocols.h"
+  export *
+}
+module IvarInImplementation {
+  header "IvarInImplementation.h"
+  export *
+}
+
+
+//--- test-mismatch-in-extension.m
+// Different ivars with the same name aren't mergeable and constitute an error.
+#import 
+@interface ObjCInterface() {
+  float ivarName; // expected-note {{previous definition is here}}
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 5; // expected-note {{previous definition is here}}
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterfaceLevel2
+@end
+
+
+//--- test-mismatch-in-ivars-number.m
+// Extensions with different amount of ivars aren't considered to be the same.
+#import 
+@interface ObjCInterface() {
+  int ivarName; // expected-note {{previous definition is here}}
+  float anotherIvar;
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterface
+@end
+
+
+//--- test-mismatch-in-methods-protocols.m
+// Extensions with different methods or protocols aren't considered to be the same.
+#import 
+@interface ObjCInterface() {
+  int methodRelatedIvar; // expected-note {{previous definition is here}}
+}
+- (v

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D121177#3371695 , @vsapsai wrote:

> Thanks for the review, Chuanqi! I believe you were touching recently 
> `isSameEntity`. Do you have any concerns about using 
> `StructuralEquivalenceContext` instead of `isSameEntity`? I've decided to go 
> with `StructuralEquivalenceContext` because at this point we are still 
> dealing with deserialization and deduplication while 
> `ASTContext::isSameEntity` kinda assumes that all deduplication is already 
> done, at least based on the comment
>
>   // Objective-C classes and protocols with the same name always match.
>   if (isa(X) || isa(X))
> return true;

Yeah, I was touching `isSameEntity`. But I don't know about 
`StructuralEquivalenceContext`. I just skipped over the implementation of 
`StructuralEquivalenceContext`. I feel like it would be OK if we could assume 
the input is duplicated indeed. 
I see it is guaranteed by `lookupInstanceVariable(Identifier)` now. I don't 
know much about ObjC. But I feel a little bit strange. Is it possible that two 
distinct variable with the same identifier in ObjC? If it is impossible, I 
think it might be OK.




Comment at: clang/include/clang/Serialization/ASTReader.h:1110
+  template 
+  using DuplicateDecls = std::pair;
+

vsapsai wrote:
> ChuanqiXu wrote:
> > How about change the name to:
> > ```
> >   template 
> >   using DuplicateObjDecls = std::pair;
> > ```
> > 
> > to clarify it is only used for ObjC. So the developer of other languages 
> > would skip this when reading.
> Would `DuplicateObjCDecls` work? `ObjC` part is common enough and I want to 
> avoid just `Obj` because it can imply "Object" which is not the goal. Also we 
> have `getObjCObjectType` which doesn't help with disambiguating the meaning 
> of "Obj".
Yeah, it should be `ObjC`. It's my typo originally.



Comment at: clang/include/clang/Serialization/ASTReader.h:1117
+  llvm::MapVector,
+  llvm::SmallVector, 4>>
+  PendingExtensionIvarRedeclarations;

vsapsai wrote:
> ChuanqiXu wrote:
> > It looks a little bit odd for me to use `Vector` for duplicate vars. 
> > Especially, the structure is `Vector>`. I feel it is better with 
> > `llvm::SmallSet`. If the order is important here, I think 
> > `llvm::SmallSetVector` might be an option.
> I'm not sure the use of `llvm::SmallSet` is warranted here. We deserialize 
> each ObjCIvarDecl from each module (and corresponding DeclContext) only once, 
> so we don't have to ensure there are no repeated pairs of same ObjCIvarDecl. 
> And for stable diagnostic we need to use some kind of ordered container. Also 
> a bunch of other "Pending..." fields don't use sets as uniqueness is enforced 
> in different ways.
Oh, I got it. `llvm::SmallVector` might not be wrong in this case. And for the 
mapping relationship, `llvm::SmallMapVector` is considerable too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review, Chuanqi! I believe you were touching recently 
`isSameEntity`. Do you have any concerns about using 
`StructuralEquivalenceContext` instead of `isSameEntity`? I've decided to go 
with `StructuralEquivalenceContext` because at this point we are still dealing 
with deserialization and deduplication while `ASTContext::isSameEntity` kinda 
assumes that all deduplication is already done, at least based on the comment

  // Objective-C classes and protocols with the same name always match.
  if (isa(X) || isa(X))
return true;




Comment at: clang/include/clang/Serialization/ASTReader.h:1110
+  template 
+  using DuplicateDecls = std::pair;
+

ChuanqiXu wrote:
> How about change the name to:
> ```
>   template 
>   using DuplicateObjDecls = std::pair;
> ```
> 
> to clarify it is only used for ObjC. So the developer of other languages 
> would skip this when reading.
Would `DuplicateObjCDecls` work? `ObjC` part is common enough and I want to 
avoid just `Obj` because it can imply "Object" which is not the goal. Also we 
have `getObjCObjectType` which doesn't help with disambiguating the meaning of 
"Obj".



Comment at: clang/include/clang/Serialization/ASTReader.h:1112-1115
+  /// When resolving duplicate ivars from extensions we don't error out
+  /// immediately but check if can merge identical extensions. Not checking
+  /// extensions for equality immediately because ivar deserialization isn't
+  /// over yet at that point.

ChuanqiXu wrote:
> Similarly, I suggest to add words or change the name to clarify it is used in 
> ObjC only.
Thanks for sharing your perspective, I'll make it more obvious it is 
ObjC-related.



Comment at: clang/include/clang/Serialization/ASTReader.h:1117
+  llvm::MapVector,
+  llvm::SmallVector, 4>>
+  PendingExtensionIvarRedeclarations;

ChuanqiXu wrote:
> It looks a little bit odd for me to use `Vector` for duplicate vars. 
> Especially, the structure is `Vector>`. I feel it is better with 
> `llvm::SmallSet`. If the order is important here, I think 
> `llvm::SmallSetVector` might be an option.
I'm not sure the use of `llvm::SmallSet` is warranted here. We deserialize each 
ObjCIvarDecl from each module (and corresponding DeclContext) only once, so we 
don't have to ensure there are no repeated pairs of same ObjCIvarDecl. And for 
stable diagnostic we need to use some kind of ordered container. Also a bunch 
of other "Pending..." fields don't use sets as uniqueness is enforced in 
different ways.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1249-1251
+if (auto *ParentExt = dyn_cast(IVD->getDeclContext())) {
+  if (auto *PrevParentExt =
+  dyn_cast(PrevIvar->getDeclContext())) {

ChuanqiXu wrote:
> nit:
> ```
> auto *ParentExt = dyn_cast(IVD->getDeclContext());
> auto *PrevParentExt = dyn_cast(PrevIvar->getDeclContext());
> if (ParentExt && PrevParentExt) {
> 
> }
> ```
> 
> We could reduce one identation in this way. Codes with less identation looks 
> better.
Thanks, good point, will change it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I don't know about ObjC.  Comments on style/name only.




Comment at: clang/include/clang/Serialization/ASTReader.h:1110
+  template 
+  using DuplicateDecls = std::pair;
+

How about change the name to:
```
  template 
  using DuplicateObjDecls = std::pair;
```

to clarify it is only used for ObjC. So the developer of other languages would 
skip this when reading.



Comment at: clang/include/clang/Serialization/ASTReader.h:1112-1115
+  /// When resolving duplicate ivars from extensions we don't error out
+  /// immediately but check if can merge identical extensions. Not checking
+  /// extensions for equality immediately because ivar deserialization isn't
+  /// over yet at that point.

Similarly, I suggest to add words or change the name to clarify it is used in 
ObjC only.



Comment at: clang/include/clang/Serialization/ASTReader.h:1117
+  llvm::MapVector,
+  llvm::SmallVector, 4>>
+  PendingExtensionIvarRedeclarations;

It looks a little bit odd for me to use `Vector` for duplicate vars. 
Especially, the structure is `Vector>`. I feel it is better with 
`llvm::SmallSet`. If the order is important here, I think 
`llvm::SmallSetVector` might be an option.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1249-1251
+if (auto *ParentExt = dyn_cast(IVD->getDeclContext())) {
+  if (auto *PrevParentExt =
+  dyn_cast(PrevIvar->getDeclContext())) {

nit:
```
auto *ParentExt = dyn_cast(IVD->getDeclContext());
auto *PrevParentExt = dyn_cast(PrevIvar->getDeclContext());
if (ParentExt && PrevParentExt) {

}
```

We could reduce one identation in this way. Codes with less identation looks 
better.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

The main alternative to the current implementation was to change 
`getRedeclContext` for ObjCCategoryDecl, so we handle ivar redeclarations and 
merges in ObjCInterfaceDecl with `ASTDeclReader::findExisting`. Decided against 
it because of noticeably different merging behavior:

- should merge not any same-name ivars but only those located in equivalent 
extensions;
- should error for incompatible same-name ivars;
- should handle ObjCCategoryDecl too but we should check them not during their 
deserialization but after their ivars are deserialized.

Mostly for these reasons decided not to piggy-back on 
`ASTDeclReader::findExisting` but to have a different implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121177/new/

https://reviews.llvm.org/D121177

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121177: [modules] Merge equivalent extensions and diagnose ivar redeclarations for extensions loaded from different modules.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: aprantl, ChuanqiXu, rsmith, ahatanak.
Herald added a subscriber: ributzka.
Herald added a project: All.
vsapsai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Emitting metadata for the same ivar multiple times can lead to
miscompilations. Objective-C runtime adds offsets to calculate ivar
position in memory and presence of duplicate offsets causes wrong final
position thus overwriting unrelated memory.

Such a situation is impossible with modules disabled as clang diagnoses
ivar redeclarations during sema checks after parsing
(`Sema::ActOnFields`). Fix the case with modules enabled by checking
during deserialization if ivar is already declared. We also support
a use case where the same category ends up in multiple modules. We
don't want to treat this case as ivar redeclaration and instead merge
corresponding ivars.

rdar://83468070


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121177

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/DeclObjC.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/merge-extension-ivars.m
  clang/test/Modules/redecl-ivars.m

Index: clang/test/Modules/redecl-ivars.m
===
--- /dev/null
+++ clang/test/Modules/redecl-ivars.m
@@ -0,0 +1,166 @@
+// UNSUPPORTED: -zos, -aix
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-mismatch-in-extension.m
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-mismatch-in-extension.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-mismatch-in-ivars-number.m
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-mismatch-in-ivars-number.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-mismatch-in-methods-protocols.m
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-mismatch-in-methods-protocols.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-redecl-in-subclass.m
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-redecl-in-subclass.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-redecl-in-implementation.m
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include %t/test-redecl-in-implementation.m \
+// RUN:-fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Same class extensions with the same ivars but from different modules aren't considered
+// an error and they are merged together. Test that differences in extensions and/or ivars
+// are still reported as errors.
+
+//--- include/Interfaces.h
+@interface NSObject @end
+@interface ObjCInterface : NSObject
+@end
+@interface ObjCInterfaceLevel2 : ObjCInterface
+@end
+
+@protocol Protocol1 @end
+@protocol Protocol2 @end
+
+//--- include/IvarsInExtensions.h
+#import 
+@interface ObjCInterface() {
+  int ivarName;
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 3;
+}
+@end
+
+//--- include/IvarsInExtensionsWithMethodsProtocols.h
+#import 
+@interface ObjCInterface() {
+  int methodRelatedIvar;
+}
+- (void)test;
+@end
+@interface ObjCInterfaceLevel2()  {
+  int protocolRelatedIvar;
+}
+@end
+
+//--- include/IvarInImplementation.h
+#import 
+@implementation ObjCInterface {
+  int ivarName;
+}
+@end
+
+//--- include/module.modulemap
+module Interfaces {
+  header "Interfaces.h"
+  export *
+}
+module IvarsInExtensions {
+  header "IvarsInExtensions.h"
+  export *
+}
+module IvarsInExtensionsWithMethodsProtocols {
+  header "IvarsInExtensionsWithMethodsProtocols.h"
+  export *
+}
+module IvarInImplementation {
+  header "IvarInImplementation.h"
+  export *
+}
+
+
+//--- test-mismatch-in-extension.m
+// Different ivars with the same name aren't mergeable and constitute an error.
+#import 
+@interface ObjCInterface() {
+  float ivarName; // expected-note {{previous definition is here}}
+}
+@end
+@interface ObjCInterfaceLevel2() {
+  int bitfieldIvarName: 5; // expected-note {{previous definition is here}}
+}
+@end
+#import 
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+// expected-error@IvarsInExtensions.h:* {{instance variable is already declared}}
+@implementation ObjCInterfaceLevel2
+@end
+
+
+//--- test-mismatch-in-ivars-number.m
+// Extensions with different amount of ivars aren't considered to be the same.
+#import 
+@interface ObjCInterface() {
+  int ivarName; //