[PATCH] D34788: [ASTReader] Add test for previous change r306583 / 145692e.

[Graydon Hoare via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL306732: [ASTReader] Add test for previous change r306583 / 
145692e. (authored by graydon).

Repository:
  rL LLVM

https://reviews.llvm.org/D34788

Files:
  cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Base.h
  cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Derive.h
  cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/H3.h
  cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/module.map
  cfe/trunk/test/Modules/lookup-assert-protocol.m


Index: cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/module.map
===
--- cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/module.map
+++ cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/module.map
@@ -0,0 +1,4 @@
+module X {
+  header "H3.h"
+  export *
+}
Index: cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/H3.h
===
--- cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/H3.h
+++ cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/H3.h
@@ -0,0 +1 @@
+#include "Base.h"
Index: cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Base.h
===
--- cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Base.h
+++ cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Base.h
@@ -0,0 +1,3 @@
+@protocol BaseProtocol
+- (void) test;
+@end
Index: cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Derive.h
===
--- cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Derive.h
+++ cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Derive.h
@@ -0,0 +1,4 @@
+#include "Base.h"
+@protocol DerivedProtocol
+- (void) test2;
+@end
Index: cfe/trunk/test/Modules/lookup-assert-protocol.m
===
--- cfe/trunk/test/Modules/lookup-assert-protocol.m
+++ cfe/trunk/test/Modules/lookup-assert-protocol.m
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I 
%S/Inputs/lookup-assert-protocol %s -verify
+// expected-no-diagnostics
+
+#include "Derive.h"
+#import 
+
+__attribute__((objc_root_class))
+@interface Thing
+@end
+
+@implementation Thing
+- (void)test {
+}
+- (void)test2 {
+}
+@end


Index: cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/module.map
===
--- cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/module.map
+++ cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/module.map
@@ -0,0 +1,4 @@
+module X {
+  header "H3.h"
+  export *
+}
Index: cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/H3.h
===
--- cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/H3.h
+++ cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/H3.h
@@ -0,0 +1 @@
+#include "Base.h"
Index: cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Base.h
===
--- cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Base.h
+++ cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Base.h
@@ -0,0 +1,3 @@
+@protocol BaseProtocol
+- (void) test;
+@end
Index: cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Derive.h
===
--- cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Derive.h
+++ cfe/trunk/test/Modules/Inputs/lookup-assert-protocol/Derive.h
@@ -0,0 +1,4 @@
+#include "Base.h"
+@protocol DerivedProtocol
+- (void) test2;
+@end
Index: cfe/trunk/test/Modules/lookup-assert-protocol.m
===
--- cfe/trunk/test/Modules/lookup-assert-protocol.m
+++ cfe/trunk/test/Modules/lookup-assert-protocol.m
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/lookup-assert-protocol %s -verify
+// expected-no-diagnostics
+
+#include "Derive.h"
+#import 
+
+__attribute__((objc_root_class))
+@interface Thing
+@end
+
+@implementation Thing
+- (void)test {
+}
+- (void)test2 {
+}
+@end
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34788: [ASTReader] Add test for previous change r306583 / 145692e.

[Graydon Hoare via Phabricator via cfe-commits]

graydon created this revision.

Add a test for the change to ASTReader that reproduces the
logic for consolidating multiple ObjC interface definitions to the
case of multiple ObjC protocol definitions.

This test is a modified copy of the test that accompanied the original
change to interfaces, in 2ba1979.


https://reviews.llvm.org/D34788

Files:
  test/Modules/Inputs/lookup-assert-protocol/Base.h
  test/Modules/Inputs/lookup-assert-protocol/Derive.h
  test/Modules/Inputs/lookup-assert-protocol/H3.h
  test/Modules/Inputs/lookup-assert-protocol/module.map
  test/Modules/lookup-assert-protocol.m


Index: test/Modules/lookup-assert-protocol.m
===
--- /dev/null
+++ test/Modules/lookup-assert-protocol.m
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I 
%S/Inputs/lookup-assert-protocol %s -verify
+// expected-no-diagnostics
+
+#include "Derive.h"
+#import 
+
+__attribute__((objc_root_class))
+@interface Thing
+@end
+
+@implementation Thing
+- (void)test {
+}
+- (void)test2 {
+}
+@end
Index: test/Modules/Inputs/lookup-assert-protocol/module.map
===
--- /dev/null
+++ test/Modules/Inputs/lookup-assert-protocol/module.map
@@ -0,0 +1,4 @@
+module X {
+  header "H3.h"
+  export *
+}
Index: test/Modules/Inputs/lookup-assert-protocol/H3.h
===
--- /dev/null
+++ test/Modules/Inputs/lookup-assert-protocol/H3.h
@@ -0,0 +1 @@
+#include "Base.h"
Index: test/Modules/Inputs/lookup-assert-protocol/Derive.h
===
--- /dev/null
+++ test/Modules/Inputs/lookup-assert-protocol/Derive.h
@@ -0,0 +1,4 @@
+#include "Base.h"
+@protocol DerivedProtocol
+- (void) test2;
+@end
Index: test/Modules/Inputs/lookup-assert-protocol/Base.h
===
--- /dev/null
+++ test/Modules/Inputs/lookup-assert-protocol/Base.h
@@ -0,0 +1,3 @@
+@protocol BaseProtocol
+- (void) test;
+@end


Index: test/Modules/lookup-assert-protocol.m
===
--- /dev/null
+++ test/Modules/lookup-assert-protocol.m
@@ -0,0 +1,17 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -I %S/Inputs/lookup-assert-protocol %s -verify
+// expected-no-diagnostics
+
+#include "Derive.h"
+#import 
+
+__attribute__((objc_root_class))
+@interface Thing
+@end
+
+@implementation Thing
+- (void)test {
+}
+- (void)test2 {
+}
+@end
Index: test/Modules/Inputs/lookup-assert-protocol/module.map
===
--- /dev/null
+++ test/Modules/Inputs/lookup-assert-protocol/module.map
@@ -0,0 +1,4 @@
+module X {
+  header "H3.h"
+  export *
+}
Index: test/Modules/Inputs/lookup-assert-protocol/H3.h
===
--- /dev/null
+++ test/Modules/Inputs/lookup-assert-protocol/H3.h
@@ -0,0 +1 @@
+#include "Base.h"
Index: test/Modules/Inputs/lookup-assert-protocol/Derive.h
===
--- /dev/null
+++ test/Modules/Inputs/lookup-assert-protocol/Derive.h
@@ -0,0 +1,4 @@
+#include "Base.h"
+@protocol DerivedProtocol
+- (void) test2;
+@end
Index: test/Modules/Inputs/lookup-assert-protocol/Base.h
===
--- /dev/null
+++ test/Modules/Inputs/lookup-assert-protocol/Base.h
@@ -0,0 +1,3 @@
+@protocol BaseProtocol
+- (void) test;
+@end
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34741: [ASTReader] Treat multiple defns of ObjC protocols the same as interfaces.

[Graydon Hoare via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL306583: [ASTReader] Treat multiple defns of ObjC protocols 
the same as interfaces. (authored by graydon).

Repository:
  rL LLVM

https://reviews.llvm.org/D34741

Files:
  cfe/trunk/include/clang/AST/Redeclarable.h
  cfe/trunk/lib/Serialization/ASTReaderDecl.cpp

Index: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
===
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
@@ -126,6 +126,9 @@
 void ReadObjCDefinitionData(struct ObjCInterfaceDecl::DefinitionData );
 void MergeDefinitionData(ObjCInterfaceDecl *D,
  struct ObjCInterfaceDecl::DefinitionData &);
+void ReadObjCDefinitionData(struct ObjCProtocolDecl::DefinitionData );
+void MergeDefinitionData(ObjCProtocolDecl *D,
+ struct ObjCProtocolDecl::DefinitionData &);
 
 static NamedDecl *getAnonymousDeclForMerging(ASTReader ,
  DeclContext *DC,
@@ -1045,18 +1048,8 @@
   IVD->setSynthesize(synth);
 }
 
-void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) {
-  RedeclarableResult Redecl = VisitRedeclarable(PD);
-  VisitObjCContainerDecl(PD);
-  mergeRedeclarable(PD, Redecl);
-
-  if (Record.readInt()) {
-// Read the definition.
-PD->allocateDefinitionData();
-
-// Set the definition data of the canonical declaration, so other
-// redeclarations will see it.
-PD->getCanonicalDecl()->Data = PD->Data;
+void ASTDeclReader::ReadObjCDefinitionData(
+ struct ObjCProtocolDecl::DefinitionData ) {
 
 unsigned NumProtoRefs = Record.readInt();
 SmallVector ProtoRefs;
@@ -1067,9 +1060,37 @@
 ProtoLocs.reserve(NumProtoRefs);
 for (unsigned I = 0; I != NumProtoRefs; ++I)
   ProtoLocs.push_back(ReadSourceLocation());
-PD->setProtocolList(ProtoRefs.data(), NumProtoRefs, ProtoLocs.data(),
-Reader.getContext());
+Data.ReferencedProtocols.set(ProtoRefs.data(), NumProtoRefs,
+ ProtoLocs.data(), Reader.getContext());
+}
+
+void ASTDeclReader::MergeDefinitionData(ObjCProtocolDecl *D,
+ struct ObjCProtocolDecl::DefinitionData &) {
+  // FIXME: odr checking?
+}
 
+void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) {
+  RedeclarableResult Redecl = VisitRedeclarable(PD);
+  VisitObjCContainerDecl(PD);
+  mergeRedeclarable(PD, Redecl);
+
+  if (Record.readInt()) {
+// Read the definition.
+PD->allocateDefinitionData();
+
+ReadObjCDefinitionData(PD->data());
+
+ObjCProtocolDecl *Canon = PD->getCanonicalDecl();
+if (Canon->Data.getPointer()) {
+  // If we already have a definition, keep the definition invariant and
+  // merge the data.
+  MergeDefinitionData(Canon, std::move(PD->data()));
+  PD->Data = Canon->Data;
+} else {
+  // Set the definition data of the canonical declaration, so other
+  // redeclarations will see it.
+  PD->getCanonicalDecl()->Data = PD->Data;
+}
 // Note that we have deserialized a definition.
 Reader.PendingDefinitions.insert(PD);
   } else {
Index: cfe/trunk/include/clang/AST/Redeclarable.h
===
--- cfe/trunk/include/clang/AST/Redeclarable.h
+++ cfe/trunk/include/clang/AST/Redeclarable.h
@@ -21,6 +21,60 @@
 namespace clang {
 class ASTContext;
 
+// Some notes on redeclarables:
+//
+//  - Every redeclarable is on a circular linked list.
+//
+//  - Every decl has a pointer to the first element of the chain _and_ a
+//DeclLink that may point to one of 3 possible states:
+//  - the "previous" (temporal) element in the chain
+//  - the "latest" (temporal) element in the chain
+//  - the an "uninitialized-latest" value (when newly-constructed)
+//
+//  - The first element is also often called the canonical element. Every
+//element has a pointer to it so that "getCanonical" can be fast.
+//
+//  - Most links in the chain point to previous, except the link out of
+//the first; it points to latest.
+//
+//  - Elements are called "first", "previous", "latest" or
+//"most-recent" when referring to temporal order: order of addition
+//to the chain.
+//
+//  - To make matters confusing, the DeclLink type uses the term "next"
+//for its pointer-storage internally (thus functions like
+//NextIsPrevious). It's easiest to just ignore the implementation of
+//DeclLink when making sense of the redeclaration chain.
+//
+//  - There's also a "definition" link for several types of
+//redeclarable, where only one definition should exist at any given
+//time (and the defn pointer is stored in the decl's "data" which
+//is copied to every element on the chain when it's changed).
+//
+//Here is some ASCII art:
+//
+//  "first"

[PATCH] D34741: [ASTReader] Treat multiple defns of ObjC protocols the same as interfaces.

[Graydon Hoare via Phabricator via cfe-commits]

graydon created this revision.

In change 2ba19793512, the ASTReader logic for ObjC interfaces was modified to
preserve the first definition-data read, "merging" later definitions into it
rather than overwriting it (though this "merging" is, in practice, a no-op that
discards the later definition-data).

Unfortunately this change was only made to ObjC interfaces, not protocols; this
means that when (for example) loading a protocol that references an interface,
if both the protocol and interface are multiply defined (as can easily happen
if the same header is read from multiple contexts), an _inconsistent_ pair of
definitions is loaded: first-read for the interface and last-read for the
protocol.

This in turn causes very subtle downstream bugs in the Swift ClangImporter,
which filters the results of name lookups based on the owning module of a
definition; inconsistency between a pair of related definitions causes name
lookup failures at various stages of compilation.

To fix these downstream issues, this change replicates the logic applied to
interfaces in change 2ba19793512, but for ObjC protocols.

rdar://30851899


https://reviews.llvm.org/D34741

Files:
  include/clang/AST/Redeclarable.h
  lib/Serialization/ASTReaderDecl.cpp

Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -126,6 +126,9 @@
 void ReadObjCDefinitionData(struct ObjCInterfaceDecl::DefinitionData );
 void MergeDefinitionData(ObjCInterfaceDecl *D,
  struct ObjCInterfaceDecl::DefinitionData &);
+void ReadObjCDefinitionData(struct ObjCProtocolDecl::DefinitionData );
+void MergeDefinitionData(ObjCProtocolDecl *D,
+ struct ObjCProtocolDecl::DefinitionData &);
 
 static NamedDecl *getAnonymousDeclForMerging(ASTReader ,
  DeclContext *DC,
@@ -1045,18 +1048,8 @@
   IVD->setSynthesize(synth);
 }
 
-void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) {
-  RedeclarableResult Redecl = VisitRedeclarable(PD);
-  VisitObjCContainerDecl(PD);
-  mergeRedeclarable(PD, Redecl);
-
-  if (Record.readInt()) {
-// Read the definition.
-PD->allocateDefinitionData();
-
-// Set the definition data of the canonical declaration, so other
-// redeclarations will see it.
-PD->getCanonicalDecl()->Data = PD->Data;
+void ASTDeclReader::ReadObjCDefinitionData(
+ struct ObjCProtocolDecl::DefinitionData ) {
 
 unsigned NumProtoRefs = Record.readInt();
 SmallVector ProtoRefs;
@@ -1067,9 +1060,37 @@
 ProtoLocs.reserve(NumProtoRefs);
 for (unsigned I = 0; I != NumProtoRefs; ++I)
   ProtoLocs.push_back(ReadSourceLocation());
-PD->setProtocolList(ProtoRefs.data(), NumProtoRefs, ProtoLocs.data(),
-Reader.getContext());
+Data.ReferencedProtocols.set(ProtoRefs.data(), NumProtoRefs,
+ ProtoLocs.data(), Reader.getContext());
+}
+
+void ASTDeclReader::MergeDefinitionData(ObjCProtocolDecl *D,
+ struct ObjCProtocolDecl::DefinitionData &) {
+  // FIXME: odr checking?
+}
 
+void ASTDeclReader::VisitObjCProtocolDecl(ObjCProtocolDecl *PD) {
+  RedeclarableResult Redecl = VisitRedeclarable(PD);
+  VisitObjCContainerDecl(PD);
+  mergeRedeclarable(PD, Redecl);
+
+  if (Record.readInt()) {
+// Read the definition.
+PD->allocateDefinitionData();
+
+ReadObjCDefinitionData(PD->data());
+
+ObjCProtocolDecl *Canon = PD->getCanonicalDecl();
+if (Canon->Data.getPointer()) {
+  // If we already have a definition, keep the definition invariant and
+  // merge the data.
+  MergeDefinitionData(Canon, std::move(PD->data()));
+  PD->Data = Canon->Data;
+} else {
+  // Set the definition data of the canonical declaration, so other
+  // redeclarations will see it.
+  PD->getCanonicalDecl()->Data = PD->Data;
+}
 // Note that we have deserialized a definition.
 Reader.PendingDefinitions.insert(PD);
   } else {
Index: include/clang/AST/Redeclarable.h
===
--- include/clang/AST/Redeclarable.h
+++ include/clang/AST/Redeclarable.h
@@ -21,6 +21,60 @@
 namespace clang {
 class ASTContext;
 
+// Some notes on redeclarables:
+//
+//  - Every redeclarable is on a circular linked list.
+//
+//  - Every decl has a pointer to the first element of the chain _and_ a
+//DeclLink that may point to one of 3 possible states:
+//  - the "previous" (temporal) element in the chain
+//  - the "latest" (temporal) element in the chain
+//  - the an "uninitialized-latest" value (when newly-constructed)
+//
+//  - The first element is also often called the canonical element. Every
+//element has a pointer to it so that "getCanonical" can be fast.
+//
+//  - Most links in the chain point to 

[PATCH] D31378: [PCH] Attach instance's dependency collectors to PCH external AST sources.

[Graydon Hoare via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL299009: [PCH] Attach instance's dependency collectors to PCH 
external AST sources. (authored by graydon).

Changed prior to commit:
  https://reviews.llvm.org/D31378?vs=93089=93387#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D31378

Files:
  cfe/trunk/include/clang/Frontend/CompilerInstance.h
  cfe/trunk/lib/Frontend/CompilerInstance.cpp
  cfe/trunk/test/PCH/emit-dependencies.c


Index: cfe/trunk/lib/Frontend/CompilerInstance.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInstance.cpp
+++ cfe/trunk/lib/Frontend/CompilerInstance.cpp
@@ -497,6 +497,8 @@
   AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(),
   getPCHContainerReader(),
   getFrontendOpts().ModuleFileExtensions,
+  TheDependencyFileGenerator.get(),
+  DependencyCollectors,
   DeserializationListener,
   OwnDeserializationListener, Preamble,
   getFrontendOpts().UseGlobalModuleIndex);
@@ -507,6 +509,8 @@
 bool AllowPCHWithCompilerErrors, Preprocessor , ASTContext ,
 const PCHContainerReader ,
 ArrayRef Extensions,
+DependencyFileGenerator *DependencyFile,
+ArrayRef DependencyCollectors,
 void *DeserializationListener, bool OwnDeserializationListener,
 bool Preamble, bool UseGlobalModuleIndex) {
   HeaderSearchOptions  = PP.getHeaderSearchInfo().getHeaderSearchOpts();
@@ -524,6 +528,12 @@
   Reader->setDeserializationListener(
   static_cast(DeserializationListener),
   /*TakeOwnership=*/OwnDeserializationListener);
+
+  if (DependencyFile)
+DependencyFile->AttachToASTReader(*Reader);
+  for (auto  : DependencyCollectors)
+Listener->attachToASTReader(*Reader);
+
   switch (Reader->ReadAST(Path,
   Preamble ? serialization::MK_Preamble
: serialization::MK_PCH,
Index: cfe/trunk/include/clang/Frontend/CompilerInstance.h
===
--- cfe/trunk/include/clang/Frontend/CompilerInstance.h
+++ cfe/trunk/include/clang/Frontend/CompilerInstance.h
@@ -662,6 +662,8 @@
   bool AllowPCHWithCompilerErrors, Preprocessor , ASTContext ,
   const PCHContainerReader ,
   ArrayRef Extensions,
+  DependencyFileGenerator *DependencyFile,
+  ArrayRef DependencyCollectors,
   void *DeserializationListener, bool OwnDeserializationListener,
   bool Preamble, bool UseGlobalModuleIndex);
 
Index: cfe/trunk/test/PCH/emit-dependencies.c
===
--- cfe/trunk/test/PCH/emit-dependencies.c
+++ cfe/trunk/test/PCH/emit-dependencies.c
@@ -0,0 +1,9 @@
+// RUN: rm -f %t.pch
+// RUN: %clang_cc1 -emit-pch -o %t.pch %S/Inputs/chain-decls1.h
+// RUN: %clang_cc1 -include-pch %t.pch -fsyntax-only -MT %s.o -dependency-file 
- %s | FileCheck %s
+// CHECK: Inputs/chain-decls1.h
+
+int main() {
+  f();
+  return 0;
+}


Index: cfe/trunk/lib/Frontend/CompilerInstance.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInstance.cpp
+++ cfe/trunk/lib/Frontend/CompilerInstance.cpp
@@ -497,6 +497,8 @@
   AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(),
   getPCHContainerReader(),
   getFrontendOpts().ModuleFileExtensions,
+  TheDependencyFileGenerator.get(),
+  DependencyCollectors,
   DeserializationListener,
   OwnDeserializationListener, Preamble,
   getFrontendOpts().UseGlobalModuleIndex);
@@ -507,6 +509,8 @@
 bool AllowPCHWithCompilerErrors, Preprocessor , ASTContext ,
 const PCHContainerReader ,
 ArrayRef Extensions,
+DependencyFileGenerator *DependencyFile,
+ArrayRef DependencyCollectors,
 void *DeserializationListener, bool OwnDeserializationListener,
 bool Preamble, bool UseGlobalModuleIndex) {
   HeaderSearchOptions  = PP.getHeaderSearchInfo().getHeaderSearchOpts();
@@ -524,6 +528,12 @@
   Reader->setDeserializationListener(
   static_cast(DeserializationListener),
   /*TakeOwnership=*/OwnDeserializationListener);
+
+  if (DependencyFile)
+DependencyFile->AttachToASTReader(*Reader);
+  for (auto  : DependencyCollectors)
+Listener->attachToASTReader(*Reader);
+
   switch (Reader->ReadAST(Path,
   Preamble ? serialization::MK_Preamble
: serialization::MK_PCH,
Index: cfe/trunk/include/clang/Frontend/CompilerInstance.h
===
--- cfe/trunk/include/clang/Frontend/CompilerInstance.h
+++ cfe/trunk/include/clang/Frontend/CompilerInstance.h
@@ -662,6 +662,8 @@
   bool AllowPCHWithCompilerErrors, Preprocessor , ASTContext ,
   const PCHContainerReader ,
   

[PATCH] D31378: [PCH] Attach instance's dependency collectors to PCH external AST sources.

[Graydon Hoare via Phabricator via cfe-commits]

graydon created this revision.

When a PCH is included via -include-pch, clang should treat the
current TU as dependent on the sourcefile that the PCH was generated from.

This is currently _partly_ accomplished by InitializePreprocessor calling
AddImplicitIncludePCH to synthesize an implicit #include of the sourcefile,
into the preprocessor's Predefines buffer.

For FrontendActions such as PreprocessOnlyAction (which is, curiously, what the
driver winds up running one of in response to a plain clang -M) this is
sufficient: the preprocessor cranks over its Predefines and emits a dependency
reference to the initial sourcefile.

For other FrontendActions (for example -emit-obj or -fsyntax-only) the
Predefines buffer is reset to the suggested predefines buffer from the PCH, so
the dependency edge is lost. The result is that clang emits a .d file in those
cases that lacks a reference to the .h file responsible for the input (and in
Swift's case, our .swiftdeps file winds up not including a reference to the
source file for a PCH bridging header.)

This patch fixes the problem by taking a different tack: ignoring the
Predefines buffer (which seems a bit like a hack anyways) and directly
attaching the CompilerInstance's DependencyCollectors (and legacy
DependencyFileGenerator) to the ASTReader for the external AST.

This approach is similar to the one chosen in earlier consultation with Bruno
and Ben, and I think it's the least-bad solution, given several options.


https://reviews.llvm.org/D31378

Files:
  include/clang/Frontend/CompilerInstance.h
  lib/Frontend/CompilerInstance.cpp
  test/PCH/emit-dependencies.c


Index: test/PCH/emit-dependencies.c
===
--- /dev/null
+++ test/PCH/emit-dependencies.c
@@ -0,0 +1,9 @@
+// RUN: rm -f %t.pch
+// RUN: %clang_cc1 -emit-pch -o %t.pch %S/Inputs/chain-decls1.h
+// RUN: %clang_cc1 -include-pch %t.pch -fsyntax-only -MT %s.o -dependency-file 
- %s | FileCheck %s
+// CHECK: Inputs/chain-decls1.h
+
+int main() {
+  f();
+  return 0;
+}
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -497,6 +497,8 @@
   AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(),
   getPCHContainerReader(),
   getFrontendOpts().ModuleFileExtensions,
+  TheDependencyFileGenerator.get(),
+  DependencyCollectors,
   DeserializationListener,
   OwnDeserializationListener, Preamble,
   getFrontendOpts().UseGlobalModuleIndex);
@@ -507,6 +509,8 @@
 bool AllowPCHWithCompilerErrors, Preprocessor , ASTContext ,
 const PCHContainerReader ,
 ArrayRef Extensions,
+DependencyFileGenerator *DependencyFile,
+ArrayRef DependencyCollectors,
 void *DeserializationListener, bool OwnDeserializationListener,
 bool Preamble, bool UseGlobalModuleIndex) {
   HeaderSearchOptions  = PP.getHeaderSearchInfo().getHeaderSearchOpts();
@@ -524,6 +528,12 @@
   Reader->setDeserializationListener(
   static_cast(DeserializationListener),
   /*TakeOwnership=*/OwnDeserializationListener);
+
+  if (DependencyFile)
+DependencyFile->AttachToASTReader(*Reader);
+  for (auto  : DependencyCollectors)
+Listener->attachToASTReader(*Reader);
+
   switch (Reader->ReadAST(Path,
   Preamble ? serialization::MK_Preamble
: serialization::MK_PCH,
Index: include/clang/Frontend/CompilerInstance.h
===
--- include/clang/Frontend/CompilerInstance.h
+++ include/clang/Frontend/CompilerInstance.h
@@ -662,6 +662,8 @@
   bool AllowPCHWithCompilerErrors, Preprocessor , ASTContext ,
   const PCHContainerReader ,
   ArrayRef Extensions,
+  DependencyFileGenerator *DependencyFile,
+  ArrayRef DependencyCollectors,
   void *DeserializationListener, bool OwnDeserializationListener,
   bool Preamble, bool UseGlobalModuleIndex);
 


Index: test/PCH/emit-dependencies.c
===
--- /dev/null
+++ test/PCH/emit-dependencies.c
@@ -0,0 +1,9 @@
+// RUN: rm -f %t.pch
+// RUN: %clang_cc1 -emit-pch -o %t.pch %S/Inputs/chain-decls1.h
+// RUN: %clang_cc1 -include-pch %t.pch -fsyntax-only -MT %s.o -dependency-file - %s | FileCheck %s
+// CHECK: Inputs/chain-decls1.h
+
+int main() {
+  f();
+  return 0;
+}
Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -497,6 +497,8 @@
   AllowPCHWithCompilerErrors, getPreprocessor(), getASTContext(),
   getPCHContainerReader(),
   getFrontendOpts().ModuleFileExtensions,
+  TheDependencyFileGenerator.get(),
+  DependencyCollectors,
 

[PATCH] D28779: [ASTReader] Add a DeserializationListener callback for IMPORTED_MODULES

[Graydon Hoare via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292436: [ASTReader] Add a DeserializationListener callback 
for IMPORTED_MODULES (authored by graydon).

Changed prior to commit:
  https://reviews.llvm.org/D28779?vs=84594=84881#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28779

Files:
  cfe/trunk/include/clang/Serialization/ASTDeserializationListener.h
  cfe/trunk/lib/Serialization/ASTReader.cpp


Index: cfe/trunk/include/clang/Serialization/ASTDeserializationListener.h
===
--- cfe/trunk/include/clang/Serialization/ASTDeserializationListener.h
+++ cfe/trunk/include/clang/Serialization/ASTDeserializationListener.h
@@ -26,6 +26,7 @@
 class MacroDefinitionRecord;
 class MacroInfo;
 class Module;
+class SourceLocation;
 
 class ASTDeserializationListener {
 public:
@@ -52,6 +53,9 @@
MacroDefinitionRecord *MD) {}
   /// \brief A module definition was read from the AST file.
   virtual void ModuleRead(serialization::SubmoduleID ID, Module *Mod) {}
+  /// \brief A module import was read from the AST file.
+  virtual void ModuleImportRead(serialization::SubmoduleID ID,
+SourceLocation ImportLoc) {}
 };
 }
 
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -3251,8 +3251,11 @@
 for (unsigned I = 0, N = Record.size(); I != N; /**/) {
   unsigned GlobalID = getGlobalSubmoduleID(F, Record[I++]);
   SourceLocation Loc = ReadSourceLocation(F, Record, I);
-  if (GlobalID)
+  if (GlobalID) {
 ImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
+if (DeserializationListener)
+  DeserializationListener->ModuleImportRead(GlobalID, Loc);
+  }
 }
   }
   break;


Index: cfe/trunk/include/clang/Serialization/ASTDeserializationListener.h
===
--- cfe/trunk/include/clang/Serialization/ASTDeserializationListener.h
+++ cfe/trunk/include/clang/Serialization/ASTDeserializationListener.h
@@ -26,6 +26,7 @@
 class MacroDefinitionRecord;
 class MacroInfo;
 class Module;
+class SourceLocation;
 
 class ASTDeserializationListener {
 public:
@@ -52,6 +53,9 @@
MacroDefinitionRecord *MD) {}
   /// \brief A module definition was read from the AST file.
   virtual void ModuleRead(serialization::SubmoduleID ID, Module *Mod) {}
+  /// \brief A module import was read from the AST file.
+  virtual void ModuleImportRead(serialization::SubmoduleID ID,
+SourceLocation ImportLoc) {}
 };
 }
 
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -3251,8 +3251,11 @@
 for (unsigned I = 0, N = Record.size(); I != N; /**/) {
   unsigned GlobalID = getGlobalSubmoduleID(F, Record[I++]);
   SourceLocation Loc = ReadSourceLocation(F, Record, I);
-  if (GlobalID)
+  if (GlobalID) {
 ImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
+if (DeserializationListener)
+  DeserializationListener->ModuleImportRead(GlobalID, Loc);
+  }
 }
   }
   break;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28790: [Modules] Correct test comment from obsolete earlier version of code. NFC

[Graydon Hoare via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292435: [Modules] Correct test comment from obsolete earlier 
version of code. NFC (authored by graydon).

Changed prior to commit:
  https://reviews.llvm.org/D28790?vs=84616=84879#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28790

Files:
  cfe/trunk/test/Modules/implicit-private-with-different-name.m


Index: cfe/trunk/test/Modules/implicit-private-with-different-name.m
===
--- cfe/trunk/test/Modules/implicit-private-with-different-name.m
+++ cfe/trunk/test/Modules/implicit-private-with-different-name.m
@@ -3,7 +3,7 @@
 // Build PCH using A, with adjacent private module APrivate, which winds up 
being implicitly referenced
 // RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name 
-emit-pch -o %t-A.pch %s
 
-// Use the PCH with no explicit way to resolve PrivateA, still pick it up 
through MODULE_DIRECTORY reference in PCH control block
+// Use the PCH with no explicit way to resolve APrivate, still pick it up by 
automatic second-chance search for "A" with "Private" appended
 // RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name 
-include-pch %t-A.pch %s -fsyntax-only
 
 // Check the fixit


Index: cfe/trunk/test/Modules/implicit-private-with-different-name.m
===
--- cfe/trunk/test/Modules/implicit-private-with-different-name.m
+++ cfe/trunk/test/Modules/implicit-private-with-different-name.m
@@ -3,7 +3,7 @@
 // Build PCH using A, with adjacent private module APrivate, which winds up being implicitly referenced
 // RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -emit-pch -o %t-A.pch %s
 
-// Use the PCH with no explicit way to resolve PrivateA, still pick it up through MODULE_DIRECTORY reference in PCH control block
+// Use the PCH with no explicit way to resolve APrivate, still pick it up by automatic second-chance search for "A" with "Private" appended
 // RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only
 
 // Check the fixit
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28790: [Modules] Correct test comment from obsolete earlier version of code. NFC

[Graydon Hoare via Phabricator via cfe-commits]

graydon created this revision.

Code committed in https://reviews.llvm.org/rL290219 went through a few 
iterations; test wound up with
stale comment.


https://reviews.llvm.org/D28790

Files:
  test/Modules/implicit-private-with-different-name.m


Index: test/Modules/implicit-private-with-different-name.m
===
--- test/Modules/implicit-private-with-different-name.m
+++ test/Modules/implicit-private-with-different-name.m
@@ -3,7 +3,7 @@
 // Build PCH using A, with adjacent private module APrivate, which winds up 
being implicitly referenced
 // RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name 
-emit-pch -o %t-A.pch %s
 
-// Use the PCH with no explicit way to resolve PrivateA, still pick it up 
through MODULE_DIRECTORY reference in PCH control block
+// Use the PCH with no explicit way to resolve APrivate, still pick it up by 
automatic second-chance search for "A" with "Private" appended
 // RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name 
-include-pch %t-A.pch %s -fsyntax-only
 
 // Check the fixit


Index: test/Modules/implicit-private-with-different-name.m
===
--- test/Modules/implicit-private-with-different-name.m
+++ test/Modules/implicit-private-with-different-name.m
@@ -3,7 +3,7 @@
 // Build PCH using A, with adjacent private module APrivate, which winds up being implicitly referenced
 // RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -emit-pch -o %t-A.pch %s
 
-// Use the PCH with no explicit way to resolve PrivateA, still pick it up through MODULE_DIRECTORY reference in PCH control block
+// Use the PCH with no explicit way to resolve APrivate, still pick it up by automatic second-chance search for "A" with "Private" appended
 // RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only
 
 // Check the fixit
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28779: [ASTReader] Add a DeserializationListener callback for IMPORTED_MODULES

[Graydon Hoare via Phabricator via cfe-commits]

graydon created this revision.

Add a callback from ASTReader to DeserializationListener when the former
reads an IMPORTED_MODULES block. This supports Swift in using PCH for
bridging headers.


https://reviews.llvm.org/D28779

Files:
  include/clang/Serialization/ASTDeserializationListener.h
  lib/Serialization/ASTReader.cpp


Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -3245,8 +3245,11 @@
 for (unsigned I = 0, N = Record.size(); I != N; /**/) {
   unsigned GlobalID = getGlobalSubmoduleID(F, Record[I++]);
   SourceLocation Loc = ReadSourceLocation(F, Record, I);
-  if (GlobalID)
+  if (GlobalID) {
 ImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
+if (DeserializationListener)
+  DeserializationListener->ModuleImportRead(GlobalID, Loc);
+  }
 }
   }
   break;
Index: include/clang/Serialization/ASTDeserializationListener.h
===
--- include/clang/Serialization/ASTDeserializationListener.h
+++ include/clang/Serialization/ASTDeserializationListener.h
@@ -26,6 +26,7 @@
 class MacroDefinitionRecord;
 class MacroInfo;
 class Module;
+class SourceLocation;
 
 class ASTDeserializationListener {
 public:
@@ -52,6 +53,9 @@
MacroDefinitionRecord *MD) {}
   /// \brief A module definition was read from the AST file.
   virtual void ModuleRead(serialization::SubmoduleID ID, Module *Mod) {}
+  /// \brief A module import was read from the AST file.
+  virtual void ModuleImportRead(serialization::SubmoduleID ID,
+SourceLocation ImportLoc) {}
 };
 }
 


Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -3245,8 +3245,11 @@
 for (unsigned I = 0, N = Record.size(); I != N; /**/) {
   unsigned GlobalID = getGlobalSubmoduleID(F, Record[I++]);
   SourceLocation Loc = ReadSourceLocation(F, Record, I);
-  if (GlobalID)
+  if (GlobalID) {
 ImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
+if (DeserializationListener)
+  DeserializationListener->ModuleImportRead(GlobalID, Loc);
+  }
 }
   }
   break;
Index: include/clang/Serialization/ASTDeserializationListener.h
===
--- include/clang/Serialization/ASTDeserializationListener.h
+++ include/clang/Serialization/ASTDeserializationListener.h
@@ -26,6 +26,7 @@
 class MacroDefinitionRecord;
 class MacroInfo;
 class Module;
+class SourceLocation;
 
 class ASTDeserializationListener {
 public:
@@ -52,6 +53,9 @@
MacroDefinitionRecord *MD) {}
   /// \brief A module definition was read from the AST file.
   virtual void ModuleRead(serialization::SubmoduleID ID, Module *Mod) {}
+  /// \brief A module import was read from the AST file.
+  virtual void ModuleImportRead(serialization::SubmoduleID ID,
+SourceLocation ImportLoc) {}
 };
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL290219: [modules] Handle modules with nonstandard names in 
module.private.modulemaps (authored by graydon).

Changed prior to commit:
  https://reviews.llvm.org/D27852?vs=82161=82177#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27852

Files:
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
  cfe/trunk/include/clang/Lex/HeaderSearch.h
  cfe/trunk/lib/Lex/HeaderSearch.cpp
  cfe/trunk/lib/Lex/ModuleMap.cpp
  
cfe/trunk/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
  
cfe/trunk/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
  
cfe/trunk/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
  
cfe/trunk/test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
  cfe/trunk/test/Modules/implicit-private-with-different-name.m

Index: cfe/trunk/lib/Lex/ModuleMap.cpp
===
--- cfe/trunk/lib/Lex/ModuleMap.cpp
+++ cfe/trunk/lib/Lex/ModuleMap.cpp
@@ -1490,6 +1490,42 @@
 ActiveModule->NoUndeclaredIncludes = true;
   ActiveModule->Directory = Directory;
 
+  if (!ActiveModule->Parent) {
+StringRef MapFileName(ModuleMapFile->getName());
+if (MapFileName.endswith("module.private.modulemap") ||
+MapFileName.endswith("module_private.map")) {
+  // Adding a top-level module from a private modulemap is likely a
+  // user error; we check to see if there's another top-level module
+  // defined in the non-private map in the same dir, and if so emit a
+  // warning.
+  for (auto E = Map.module_begin(); E != Map.module_end(); ++E) {
+auto const *M = E->getValue();
+if (!M->Parent &&
+M->Directory == ActiveModule->Directory &&
+M->Name != ActiveModule->Name) {
+  Diags.Report(ActiveModule->DefinitionLoc,
+   diag::warn_mmap_mismatched_top_level_private)
+<< ActiveModule->Name << M->Name;
+  // The pattern we're defending against here is typically due to
+  // a module named FooPrivate which is supposed to be a submodule
+  // called Foo.Private. Emit a fixit in that case.
+  auto D =
+Diags.Report(ActiveModule->DefinitionLoc,
+ diag::note_mmap_rename_top_level_private_as_submodule);
+  D << ActiveModule->Name << M->Name;
+  StringRef Bad(ActiveModule->Name);
+  if (Bad.consume_back("Private")) {
+SmallString<128> Fixed = Bad;
+Fixed.append(".Private");
+D << FixItHint::CreateReplacement(ActiveModule->DefinitionLoc,
+  Fixed);
+  }
+  break;
+}
+  }
+}
+  }
+
   bool Done = false;
   do {
 switch (Tok.Kind) {
Index: cfe/trunk/lib/Lex/HeaderSearch.cpp
===
--- cfe/trunk/lib/Lex/HeaderSearch.cpp
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp
@@ -194,16 +194,36 @@
   Module *Module = ModMap.findModule(ModuleName);
   if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps)
 return Module;
-  
+
+  StringRef SearchName = ModuleName;
+  Module = lookupModule(ModuleName, SearchName);
+
+  // The facility for "private modules" -- adjacent, optional module maps named
+  // module.private.modulemap that are supposed to define private submodules --
+  // is sometimes misused by frameworks that name their associated private
+  // module FooPrivate, rather than as a submodule named Foo.Private as
+  // intended. Here we compensate for such cases by looking in directories named
+  // Foo.framework, when we previously looked and failed to find a
+  // FooPrivate.framework.
+  if (!Module && SearchName.consume_back("Private"))
+Module = lookupModule(ModuleName, SearchName);
+  return Module;
+}
+
+Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) {
+  Module *Module = nullptr;
+
   // Look through the various header search paths to load any available module
   // maps, searching for a module map that describes this module.
   for (unsigned Idx = 0, N = SearchDirs.size(); Idx != N; ++Idx) {
 if (SearchDirs[Idx].isFramework()) {
-  // Search for or infer a module map for a framework.
+  // Search for or infer a module map for a framework. Here we use
+  // SearchName rather than ModuleName, to permit finding private modules
+  // named FooPrivate in buggy frameworks named Foo.
   SmallString<128> FrameworkDirName;
   FrameworkDirName += SearchDirs[Idx].getFrameworkDir()->getName();
-  llvm::sys::path::append(FrameworkDirName, ModuleName + ".framework");
-  if (const DirectoryEntry *FrameworkDir 
+  

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

graydon added inline comments.



Comment at: test/Modules/implicit-private-with-different-name.m:13
+// 
expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{top-level
 module 'APrivate' in private module map, expected a submodule of 'A'}}
+// 
expected-note@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{make
 'APrivate' a submodule of 'A' to ensure it can be found by name}}
+// CHECK: 
fix-it:"{{.*}}/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap":{1:18-1:26}:"A.Private"

bruno wrote:
> I think we can enhance the usability a bit here, how about:
> 
> warning: top-level module 'APrivate' in private module map, expected a 
> submodule of 'A'
> note: make 'APrivate' a submodule of 'A' to ensure it can be found by name
>   framework module APrivate {
>^
> A.Private
> 
> 
I'm not sure what you're requesting; as far as I can see that is exactly the 
diagnostic I'm emitting (along with the fixit)


https://reviews.llvm.org/D27852



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

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

graydon updated this revision to Diff 82161.
graydon added a comment.

Updates to address review comments


https://reviews.llvm.org/D27852

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticLexKinds.td
  include/clang/Lex/HeaderSearch.h
  lib/Lex/HeaderSearch.cpp
  lib/Lex/ModuleMap.cpp
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
  test/Modules/implicit-private-with-different-name.m

Index: test/Modules/implicit-private-with-different-name.m
===
--- /dev/null
+++ test/Modules/implicit-private-with-different-name.m
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t
+
+// Build PCH using A, with adjacent private module APrivate, which winds up being implicitly referenced
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -emit-pch -o %t-A.pch %s
+
+// Use the PCH with no explicit way to resolve PrivateA, still pick it up through MODULE_DIRECTORY reference in PCH control block
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only
+
+// Check the fixit
+// RUN: %clang_cc1  -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{top-level module 'APrivate' in private module map, expected a submodule of 'A'}}
+// expected-note@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{make 'APrivate' a submodule of 'A' to ensure it can be found by name}}
+// CHECK: fix-it:"{{.*}}/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap":{1:18-1:26}:"A.Private"
+
+#ifndef HEADER
+#define HEADER
+#import "A/aprivate.h"
+const int *y = 
+#endif
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
@@ -0,0 +1,4 @@
+framework module APrivate {
+  header "aprivate.h"
+  export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module A {
+  header "a.h"
+  export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
@@ -0,0 +1 @@
+extern int APRIVATE;
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
@@ -0,0 +1 @@
+extern int APUBLIC;
Index: lib/Lex/ModuleMap.cpp
===
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -1490,6 +1490,42 @@
 ActiveModule->NoUndeclaredIncludes = true;
   ActiveModule->Directory = Directory;
 
+  if (!ActiveModule->Parent) {
+StringRef MapFileName(ModuleMapFile->getName());
+if (MapFileName.endswith("module.private.modulemap") ||
+MapFileName.endswith("module_private.map")) {
+  // Adding a top-level module from a private modulemap is likely a
+  // user error; we check to see if there's another top-level module
+  // defined in the non-private map in the same dir, and if so emit a
+  // warning.
+  for (auto E = Map.module_begin(); E != Map.module_end(); ++E) {
+auto const *M = E->getValue();
+if (!M->Parent &&
+M->Directory == ActiveModule->Directory &&
+M->Name != ActiveModule->Name) {
+  Diags.Report(ActiveModule->DefinitionLoc,
+   diag::warn_mmap_mismatched_top_level_private)
+<< ActiveModule->Name << M->Name;
+  // The pattern we're defending against here is typically due to
+  // a module named FooPrivate which is supposed to be a submodule
+ 

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

graydon marked 5 inline comments as done.
graydon added a comment.

Addressed review comments


https://reviews.llvm.org/D27852



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

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Graydon Hoare via Phabricator via cfe-commits]

graydon created this revision.
graydon added reviewers: manmanren, doug.gregor.
graydon added a subscriber: cfe-commits.

The module system supports accompanying a primary module (say Foo) with
an auxiliary "private" module (defined in an adjacent module.private.modulemap
file) that augments the primary module when associated private headers are
available. The feature is intended to be used to augment the primary
module with a submodule (say Foo.Private), however some users in the wild
are choosing to augment the primary module with an additional top-level module
with a "similar" name (in all cases so far: FooPrivate).

This "works" when a user of the module initially imports a private header,
such as '#import "Foo/something_private.h"' since the Foo import winds up
importing FooPrivate in passing. But if the import is subsequently recorded
in a PCH file, reloading the PCH will fail to validate because of a cross-check
that attempts to find the module.modulemap (or module.private.modulemap) using
HeaderSearch algorithm, applied to the "FooPrivate" name. Since it's stored in
Foo.framework/Modules, not FooPrivate.framework/Modules, the check fails and
the PCH is rejected.

This patch adds a compensatory workaround in the HeaderSearch algorithm
when searching (and failing to find) a module of the form FooPrivate: the
name used to derive filesystem paths is decoupled from the module name
being searched for, and if the initial search fails and the module is
named "FooPrivate", the filesystem search name is altered to remove the
"Private" suffix, and the algorithm is run a second time (still looking for
a module named FooPrivate, but looking in directories derived from Foo).

Accompanying this change is a new warning that triggers when a user loads
a module.private.modulemap that defines a top-level module with a different
name from the top-level module defined in its adjacent module.modulemap.


https://reviews.llvm.org/D27852

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  include/clang/Lex/HeaderSearch.h
  lib/Lex/HeaderSearch.cpp
  lib/Lex/ModuleMap.cpp
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
  
test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
  test/Modules/implicit-private-with-different-name.m

Index: test/Modules/implicit-private-with-different-name.m
===
--- /dev/null
+++ test/Modules/implicit-private-with-different-name.m
@@ -0,0 +1,15 @@
+// RUN: rm -rf %t
+
+// Build PCH using A, with adjacent private module APrivate, which winds up being implicitly referenced
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -emit-pch -o %t-A.pch %s
+
+// Use the PCH with no explicit way to resolve PrivateA, still pick it up through MODULE_DIRECTORY reference in PCH control block
+// RUN: %clang_cc1 -verify -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs/implicit-private-with-different-name -include-pch %t-A.pch %s -fsyntax-only
+
+// expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{expected a submodule}}
+
+#ifndef HEADER
+#define HEADER
+#import "A/aprivate.h"
+const int *y = 
+#endif
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap
@@ -0,0 +1,4 @@
+framework module APrivate {
+  header "aprivate.h"
+  export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+framework module A {
+  header "a.h"
+  export *
+}
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/aprivate.h
@@ -0,0 +1 @@
+extern int APRIVATE;
Index: test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
===
--- /dev/null
+++ test/Modules/Inputs/implicit-private-with-different-name/A.framework/Headers/a.h
@@ -0,0 +1 @@
+extern int APUBLIC;
Index: lib/Lex/ModuleMap.cpp
===
--- 

[PATCH] D27580: [modules] Add optional out-param to ASTReader::ReadAST for imported submodules.

[Graydon Hoare via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL289276: [modules] Add optional out-param to 
ASTReader::ReadAST for imported submodules. (authored by graydon).

Changed prior to commit:
  https://reviews.llvm.org/D27580?vs=80782=80952#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27580

Files:
  cfe/trunk/include/clang/Serialization/ASTReader.h
  cfe/trunk/lib/Serialization/ASTReader.cpp


Index: cfe/trunk/include/clang/Serialization/ASTReader.h
===
--- cfe/trunk/include/clang/Serialization/ASTReader.h
+++ cfe/trunk/include/clang/Serialization/ASTReader.h
@@ -821,14 +821,16 @@
   // \brief A list of late parsed template function data.
   SmallVector LateParsedTemplates;
 
+public:
   struct ImportedSubmodule {
 serialization::SubmoduleID ID;
 SourceLocation ImportLoc;
 
 ImportedSubmodule(serialization::SubmoduleID ID, SourceLocation ImportLoc)
   : ID(ID), ImportLoc(ImportLoc) {}
   };
 
+private:
   /// \brief A list of modules that were imported by precompiled headers or
   /// any other non-module AST file.
   SmallVector ImportedModules;
@@ -1404,9 +1406,13 @@
   /// \param ClientLoadCapabilities The set of client load-failure
   /// capabilities, represented as a bitset of the enumerators of
   /// LoadFailureCapabilities.
+  ///
+  /// \param Imported optional out-parameter to append the list of modules
+  /// that were imported by precompiled headers or any other non-module AST 
file
   ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
 SourceLocation ImportLoc,
-unsigned ClientLoadCapabilities);
+unsigned ClientLoadCapabilities,
+SmallVectorImpl *Imported = 
nullptr);
 
   /// \brief Make the entities in the given module and any of its 
(non-explicit)
   /// submodules visible to name lookup.
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -3578,7 +3578,8 @@
 ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
 ModuleKind Type,
 SourceLocation ImportLoc,
-unsigned ClientLoadCapabilities) {
+unsigned ClientLoadCapabilities,
+SmallVectorImpl 
*Imported) {
   llvm::SaveAndRestore
 SetCurImportLocRAII(CurrentImportLoc, ImportLoc);
 
@@ -3744,6 +3745,10 @@
   }
   UnresolvedModuleRefs.clear();
 
+  if (Imported)
+Imported->append(ImportedModules.begin(),
+ ImportedModules.end());
+
   // FIXME: How do we load the 'use'd modules? They may not be submodules.
   // Might be unnecessary as use declarations are only used to build the
   // module itself.


Index: cfe/trunk/include/clang/Serialization/ASTReader.h
===
--- cfe/trunk/include/clang/Serialization/ASTReader.h
+++ cfe/trunk/include/clang/Serialization/ASTReader.h
@@ -821,14 +821,16 @@
   // \brief A list of late parsed template function data.
   SmallVector LateParsedTemplates;
 
+public:
   struct ImportedSubmodule {
 serialization::SubmoduleID ID;
 SourceLocation ImportLoc;
 
 ImportedSubmodule(serialization::SubmoduleID ID, SourceLocation ImportLoc)
   : ID(ID), ImportLoc(ImportLoc) {}
   };
 
+private:
   /// \brief A list of modules that were imported by precompiled headers or
   /// any other non-module AST file.
   SmallVector ImportedModules;
@@ -1404,9 +1406,13 @@
   /// \param ClientLoadCapabilities The set of client load-failure
   /// capabilities, represented as a bitset of the enumerators of
   /// LoadFailureCapabilities.
+  ///
+  /// \param Imported optional out-parameter to append the list of modules
+  /// that were imported by precompiled headers or any other non-module AST file
   ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
 SourceLocation ImportLoc,
-unsigned ClientLoadCapabilities);
+unsigned ClientLoadCapabilities,
+SmallVectorImpl *Imported = nullptr);
 
   /// \brief Make the entities in the given module and any of its (non-explicit)
   /// submodules visible to name lookup.
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -3578,7 +3578,8 @@
 ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
 

[PATCH] D27580: [modules] Add optional out-param to ASTReader::ReadAST for imported submodules.

[Graydon Hoare via Phabricator via cfe-commits]

graydon created this revision.
graydon added reviewers: doug.gregor, manmanren, rsmith.
graydon added a subscriber: cfe-commits.

The Swift frontend is acquiring the ability to load non-module PCH files 
containing
bridging definitions from C/ObjC. As part of this work, it needs to know which 
submodules
were imported by a PCH in order to wrap them in local Swift modules. This 
information
is collected by ASTReader::ReadAST in a local vector, but is currently kept 
private.

The change here is just to make the type of the vector elements public, and 
provide
an optional out-parameter to the ReadAST method to provide the vector's 
contents to
a caller after a successful read.


https://reviews.llvm.org/D27580

Files:
  include/clang/Serialization/ASTReader.h
  lib/Serialization/ASTReader.cpp


Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -3578,7 +3578,8 @@
 ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
 ModuleKind Type,
 SourceLocation ImportLoc,
-unsigned ClientLoadCapabilities) {
+unsigned ClientLoadCapabilities,
+SmallVectorImpl 
*Imported) {
   llvm::SaveAndRestore
 SetCurImportLocRAII(CurrentImportLoc, ImportLoc);
 
@@ -3744,6 +3745,10 @@
   }
   UnresolvedModuleRefs.clear();
 
+  if (Imported)
+Imported->append(ImportedModules.begin(),
+ ImportedModules.end());
+
   // FIXME: How do we load the 'use'd modules? They may not be submodules.
   // Might be unnecessary as use declarations are only used to build the
   // module itself.
Index: include/clang/Serialization/ASTReader.h
===
--- include/clang/Serialization/ASTReader.h
+++ include/clang/Serialization/ASTReader.h
@@ -821,14 +821,16 @@
   // \brief A list of late parsed template function data.
   SmallVector LateParsedTemplates;
 
+public:
   struct ImportedSubmodule {
 serialization::SubmoduleID ID;
 SourceLocation ImportLoc;
 
 ImportedSubmodule(serialization::SubmoduleID ID, SourceLocation ImportLoc)
   : ID(ID), ImportLoc(ImportLoc) {}
   };
 
+private:
   /// \brief A list of modules that were imported by precompiled headers or
   /// any other non-module AST file.
   SmallVector ImportedModules;
@@ -1404,9 +1406,13 @@
   /// \param ClientLoadCapabilities The set of client load-failure
   /// capabilities, represented as a bitset of the enumerators of
   /// LoadFailureCapabilities.
+  ///
+  /// \param Imported optional out-parameter to append the list of modules
+  /// that were imported by precompiled headers or any other non-module AST 
file
   ASTReadResult ReadAST(StringRef FileName, ModuleKind Type,
 SourceLocation ImportLoc,
-unsigned ClientLoadCapabilities);
+unsigned ClientLoadCapabilities,
+SmallVectorImpl *Imported = 
nullptr);
 
   /// \brief Make the entities in the given module and any of its 
(non-explicit)
   /// submodules visible to name lookup.


Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -3578,7 +3578,8 @@
 ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
 ModuleKind Type,
 SourceLocation ImportLoc,
-unsigned ClientLoadCapabilities) {
+unsigned ClientLoadCapabilities,
+SmallVectorImpl *Imported) {
   llvm::SaveAndRestore
 SetCurImportLocRAII(CurrentImportLoc, ImportLoc);
 
@@ -3744,6 +3745,10 @@
   }
   UnresolvedModuleRefs.clear();
 
+  if (Imported)
+Imported->append(ImportedModules.begin(),
+ ImportedModules.end());
+
   // FIXME: How do we load the 'use'd modules? They may not be submodules.
   // Might be unnecessary as use declarations are only used to build the
   // module itself.
Index: include/clang/Serialization/ASTReader.h
===
--- include/clang/Serialization/ASTReader.h
+++ include/clang/Serialization/ASTReader.h
@@ -821,14 +821,16 @@
   // \brief A list of late parsed template function data.
   SmallVector LateParsedTemplates;
 
+public:
   struct ImportedSubmodule {
 serialization::SubmoduleID ID;
 SourceLocation ImportLoc;
 
 ImportedSubmodule(serialization::SubmoduleID ID, SourceLocation ImportLoc)
   : ID(ID),