bruno created this revision.
bruno added reviewers: ributzka, vsapsai, dexonsmith.
Framework vendors usually layout their framework headers in the following way:
Foo.framework/Headers -> "public" headers
Foo.framework/PrivateHeader -> "private" headers
Since both headers in both directories can be found with #import
<Foo/some-header.h>, it's easy to make mistakes and include headers in
Foo.framework/PrivateHeader from headers in Foo.framework/Headers, which
usually configures a layering violation on Darwin ecosystems. One of the
problem this causes is dep cycles when modules are used, since it's very common
for "private" modules to include from the "public" ones; adding an edge the
other way around will trigger cycles.
Add a warning to catch those cases such that:
In file included from test.m:1:
./A.framework/Headers/A.h:2:10: warning: public framework header includes
private framework header 'A/APriv.h' [-Wframework-include-private-from-public]
^
This only works for the layering violation within a framework.
rdar://problem/38712182
Depends on https://reviews.llvm.org/D47157
Repository:
rC Clang
https://reviews.llvm.org/D47301
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticLexKinds.td
lib/Lex/HeaderSearch.cpp
test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h
test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap
test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap
test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h
test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h
test/Modules/Inputs/framework-public-includes-private/a.hmap.json
test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h
test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap
test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap
test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h
test/Modules/Inputs/framework-public-includes-private/z.hmap.json
test/Modules/Inputs/framework-public-includes-private/z.yaml
test/Modules/framework-public-includes-private.m
Index: test/Modules/framework-public-includes-private.m
===================================================================
--- /dev/null
+++ test/Modules/framework-public-includes-private.m
@@ -0,0 +1,37 @@
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// RUN: hmaptool write %S/Inputs/framework-public-includes-private/a.hmap.json %t/a.hmap
+// RUN: hmaptool write %S/Inputs/framework-public-includes-private/z.hmap.json %t/z.hmap
+
+// RUN: sed -e "s:TEST_DIR:%S/Inputs/framework-public-includes-private:g" \
+// RUN: %S/Inputs/framework-public-includes-private/z.yaml > %t/z.yaml
+
+// The output with and without modules should be the same, without modules first.
+// RUN: %clang_cc1 \
+// RUN: -iquote %t/z.hmap -iquote %t/a.hmap -ivfsoverlay %t/z.yaml \
+// RUN: -F%S/Inputs/framework-public-includes-private \
+// RUN: -fsyntax-only %s -verify
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN: -iquote %t/z.hmap -iquote %t/a.hmap -ivfsoverlay %t/z.yaml \
+// RUN: -F%S/Inputs/framework-public-includes-private \
+// RUN: -fsyntax-only %s \
+// RUN: 2>%t/stderr
+
+// The same warnings show up when modules is on but -verify doesn't get it
+// because they only show up under the module A building context.
+// RUN: FileCheck --input-file=%t/stderr %s
+// CHECK: public framework header includes private framework header 'A/APriv.h'
+// CHECK: public framework header includes private framework header 'A/APriv2.h'
+// CHECK: public framework header includes private framework header 'Z/ZPriv.h'
+
+#import "A.h"
+
+int bar() { return foo(); }
+
+// expected-warning@Inputs/framework-public-includes-private/A.framework/Headers/A.h:2{{public framework header includes private framework header 'A/APriv.h'}}
+// expected-warning@Inputs/framework-public-includes-private/A.framework/Headers/A.h:3{{public framework header includes private framework header 'A/APriv2.h'}}
+// expected-warning@Inputs/framework-public-includes-private/flat-header-path/Z.h:1{{public framework header includes private framework header 'Z/ZPriv.h'}}
Index: test/Modules/Inputs/framework-public-includes-private/z.yaml
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/z.yaml
@@ -0,0 +1,45 @@
+{
+ 'version': 0,
+ 'case-sensitive': 'false',
+ 'use-external-names' : 'false',
+ 'roots': [
+ {
+ 'type': 'directory',
+ 'name': "TEST_DIR/Z.framework/Headers",
+ 'contents': [
+ {
+ 'type': 'file',
+ 'name': "Z.h",
+ 'external-contents': "TEST_DIR/flat-header-path/Z.h"
+ }
+ ]
+ },
+ {
+ 'type': 'directory',
+ 'name': "TEST_DIR/Z.framework/PrivateHeaders",
+ 'contents': [
+ {
+ 'type': 'file',
+ 'name': "ZPriv.h",
+ 'external-contents': "TEST_DIR/flat-header-path/ZPriv.h"
+ }
+ ]
+ },
+ {
+ 'type': 'directory',
+ 'name': "TEST_DIR/Z.framework/Modules",
+ 'contents': [
+ {
+ 'type': 'file',
+ 'name': "module.modulemap",
+ 'external-contents': "TEST_DIR/flat-header-path/Z.modulemap"
+ },
+ {
+ 'type': 'file',
+ 'name': "module.private.modulemap",
+ 'external-contents': "TEST_DIR/flat-header-path/Z.private.modulemap"
+ }
+ ]
+ }
+ ]
+}
Index: test/Modules/Inputs/framework-public-includes-private/z.hmap.json
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/z.hmap.json
@@ -0,0 +1,7 @@
+
+{
+ "mappings" :
+ {
+ "ZPriv.h" : "Z/ZPriv.h"
+ }
+}
Index: test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h
@@ -0,0 +1 @@
+// ZPriv.h
Index: test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap
@@ -0,0 +1,3 @@
+framework module Z_Private {
+ header "ZPriv.h"
+}
Index: test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap
@@ -0,0 +1,3 @@
+framework module Z {
+ header "Z.h"
+}
Index: test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h
@@ -0,0 +1 @@
+#import "ZPriv.h"
Index: test/Modules/Inputs/framework-public-includes-private/a.hmap.json
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/a.hmap.json
@@ -0,0 +1,8 @@
+
+{
+ "mappings" :
+ {
+ "A.h" : "A/A.h",
+ "APriv2.h" : "A/APriv2.h"
+ }
+}
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h
@@ -0,0 +1 @@
+// APriv.h
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h
@@ -0,0 +1 @@
+// APriv.h
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap
@@ -0,0 +1,3 @@
+framework module A_Private {
+ header "APriv.h"
+}
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+
+framework module A {
+ header "A.h"
+}
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h
@@ -0,0 +1,5 @@
+
+#include <A/APriv.h>
+#include "APriv2.h"
+#include <Z/Z.h>
+int foo();
Index: lib/Lex/HeaderSearch.cpp
===================================================================
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -621,10 +621,12 @@
return CopyStr;
}
-static bool isFrameworkStylePath(StringRef Path) {
+static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader,
+ SmallVectorImpl<char> &FrameworkName) {
using namespace llvm::sys;
path::const_iterator I = path::begin(Path);
path::const_iterator E = path::end(Path);
+ IsPrivateHeader = false;
// Detect different types of framework style paths:
//
@@ -636,16 +638,28 @@
// and some other variations among these lines.
int FoundComp = 0;
while (I != E) {
- if (I->endswith(".framework"))
+ if (*I == "Headers")
+ ++FoundComp;
+ if (I->endswith(".framework")) {
+ FrameworkName.append(I->begin(), I->end());
++FoundComp;
- if (*I == "Headers" || *I == "PrivateHeaders")
+ }
+ if (*I == "PrivateHeaders") {
++FoundComp;
+ IsPrivateHeader = true;
+ }
++I;
}
return FoundComp >= 2;
}
+static bool isFrameworkStylePath(StringRef Path) {
+ bool IsPrivateHdr = false;
+ SmallString<128> FrameworkName;
+ return isFrameworkStylePath(Path, IsPrivateHdr, FrameworkName);
+}
+
/// LookupFile - Given a "foo" or \<foo> reference, look up the indicated file,
/// return null on failure. isAngled indicates whether the file reference is
/// for system \#include's or not (i.e. using <> instead of ""). Includers, if
@@ -864,12 +878,30 @@
return MSFE;
}
+ SmallString<128> FromFramework;
bool FoundByHeaderMap = !IsMapped ? false : *IsMapped;
- if (!Includers.empty() && !isAngled &&
- isFrameworkStylePath(Includers.front().second->getName()) &&
- !FoundByHeaderMap)
- Diags.Report(IncludeLoc, diag::warn_quoted_include_in_framework_header)
- << Filename;
+ bool IsFrameworkPrivateHeader = false;
+ bool IsIncluderFromFramework =
+ isFrameworkStylePath(Includers.front().second->getName(),
+ IsFrameworkPrivateHeader, FromFramework);
+ if (!Includers.empty() && IsIncluderFromFramework) {
+ if (!isAngled && !FoundByHeaderMap)
+ Diags.Report(IncludeLoc, diag::warn_quoted_include_in_framework_header)
+ << Filename;
+ // Headers in Foo.framework/Headers should not include headers
+ // from Foo.framework/PrivateHeaders, since this violates public/private
+ // api boundaries and can cause modular dependency cycles.
+ SmallString<128> ToFramework;
+ bool IncludeIsFrameworkPrivateHeader = false;
+ if (!IsFrameworkPrivateHeader &&
+ isFrameworkStylePath(FE->getName(),
+ IncludeIsFrameworkPrivateHeader, ToFramework) &&
+ IncludeIsFrameworkPrivateHeader && FromFramework == ToFramework) {
+ Diags.Report(IncludeLoc,
+ diag::warn_framework_include_private_from_public)
+ << Filename;
+ }
+ }
// Remember this location for the next lookup we do.
CacheLookup.HitIdx = i;
Index: include/clang/Basic/DiagnosticLexKinds.td
===================================================================
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -712,6 +712,9 @@
def warn_quoted_include_in_framework_header : Warning<
"double-quoted include \"%0\" in framework header, expected system style <angled> include"
>, InGroup<FrameworkHdrQuotedInclude>, DefaultIgnore;
+def warn_framework_include_private_from_public : Warning<
+ "public framework header includes private framework header '%0'"
+ >, InGroup<FrameworkIncludePrivateFromPublic>;
def warn_auto_module_import : Warning<
"treating #%select{include|import|include_next|__include_macros}0 as an "
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -32,6 +32,7 @@
def Section : DiagGroup<"section">;
def AutoImport : DiagGroup<"auto-import">;
def FrameworkHdrQuotedInclude : DiagGroup<"quoted-include-in-framework-header">;
+def FrameworkIncludePrivateFromPublic : DiagGroup<"framework-include-private-from-public">;
def CXX14BinaryLiteral : DiagGroup<"c++14-binary-literal">;
def CXXPre14CompatBinaryLiteral : DiagGroup<"c++98-c++11-compat-binary-literal">;
def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
