[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL371655: Fix -Wnonportable-include-path suppression for
header maps with absolute paths. (authored by vsapsai, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D58094?vs=219194&id=219788#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58094/new/
https://reviews.llvm.org/D58094
Files:
cfe/trunk/include/clang/Lex/DirectoryLookup.h
cfe/trunk/lib/Lex/HeaderSearch.cpp
cfe/trunk/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
cfe/trunk/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Bar.h
cfe/trunk/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Baz.h
cfe/trunk/test/Preprocessor/nonportable-include-with-hmap.c
Index: cfe/trunk/lib/Lex/HeaderSearch.cpp
===
--- cfe/trunk/lib/Lex/HeaderSearch.cpp
+++ cfe/trunk/lib/Lex/HeaderSearch.cpp
@@ -341,9 +341,10 @@
SmallVectorImpl *SearchPath, SmallVectorImpl *RelativePath,
Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule,
bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound,
-bool &HasBeenMapped, SmallVectorImpl &MappedName) const {
+bool &IsInHeaderMap, SmallVectorImpl &MappedName) const {
InUserSpecifiedSystemFramework = false;
- HasBeenMapped = false;
+ IsInHeaderMap = false;
+ MappedName.clear();
SmallString<1024> TmpDir;
if (isNormalDir()) {
@@ -377,6 +378,8 @@
if (Dest.empty())
return None;
+ IsInHeaderMap = true;
+
auto FixupSearchPath = [&]() {
if (SearchPath) {
StringRef SearchPathRef(getName());
@@ -393,10 +396,8 @@
// ("Foo.h" -> "Foo/Foo.h"), in which case continue header lookup using the
// framework include.
if (llvm::sys::path::is_relative(Dest)) {
-MappedName.clear();
MappedName.append(Dest.begin(), Dest.end());
Filename = StringRef(MappedName.begin(), MappedName.size());
-HasBeenMapped = true;
Optional Result = HM->LookupFile(Filename, HS.getFileMgr());
if (Result) {
FixupSearchPath();
@@ -883,18 +884,22 @@
// Check each directory in sequence to see if it contains this file.
for (; i != SearchDirs.size(); ++i) {
bool InUserSpecifiedSystemFramework = false;
-bool HasBeenMapped = false;
+bool IsInHeaderMap = false;
bool IsFrameworkFoundInDir = false;
Optional File = SearchDirs[i].LookupFile(
Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule,
SuggestedModule, InUserSpecifiedSystemFramework, IsFrameworkFoundInDir,
-HasBeenMapped, MappedName);
-if (HasBeenMapped) {
+IsInHeaderMap, MappedName);
+if (!MappedName.empty()) {
+ assert(IsInHeaderMap && "MappedName should come from a header map");
CacheLookup.MappedName =
- copyString(Filename, LookupFileCache.getAllocator());
- if (IsMapped)
-*IsMapped = true;
+ copyString(MappedName, LookupFileCache.getAllocator());
}
+if (IsMapped)
+ // A filename is mapped when a header map remapped it to a relative path
+ // used in subsequent header search or to an absolute path pointing to an
+ // existing file.
+ *IsMapped |= (!MappedName.empty() || (IsInHeaderMap && File));
if (IsFrameworkFound)
// Because we keep a filename remapped for subsequent search directory
// lookups, ignore IsFrameworkFoundInDir after the first remapping and not
Index: cfe/trunk/include/clang/Lex/DirectoryLookup.h
===
--- cfe/trunk/include/clang/Lex/DirectoryLookup.h
+++ cfe/trunk/include/clang/Lex/DirectoryLookup.h
@@ -183,7 +183,7 @@
SmallVectorImpl *RelativePath, Module *RequestingModule,
ModuleMap::KnownHeader *SuggestedModule,
bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound,
- bool &HasBeenMapped, SmallVectorImpl &MappedName) const;
+ bool &IsInHeaderMap, SmallVectorImpl &MappedName) const;
private:
Optional DoFrameworkLookup(
Index: cfe/trunk/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
===
--- cfe/trunk/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
+++ cfe/trunk/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
@@ -1,6 +1,9 @@
{
"mappings" :
{
- "Foo/Foo.h" : "headers/foo/Foo.h"
+ "Foo/Foo.h" : "headers/foo/Foo.h",
+ "Bar.h" : "headers/foo/Bar.h",
+ "Foo/Bar.h" : "INPUTS_DIR/nonportable-hmaps/headers/foo/Bar.h",
+ "headers/Foo/Baz.h" : "/not/existing/absolute/path/Baz.h"
}
}
Index: cfe/trunk/test/Preprocessor/nonportable-include-with-hmap.c
===
--- cfe/trunk/test/Preprocessor/nonporta
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai marked 2 inline comments as done.
vsapsai added a comment.
Thanks for the review.
Comment at: clang/lib/Lex/HeaderSearch.cpp:892-902
+IsInHeaderMap, MappedName);
+if (!MappedName.empty()) {
+ assert(IsInHeaderMap && "MappedName should come from a header map");
CacheLookup.MappedName =
- copyString(Filename, LookupFileCache.getAllocator());
- if (IsMapped)
-*IsMapped = true;
+ copyString(MappedName, LookupFileCache.getAllocator());
}
+if (IsMapped)
dexonsmith wrote:
> I wonder if this would be easier to follow if you refactored like this:
>
> ```
> if (!MappedName.empty()) {
> // other logic.
> if (IsMapped)
> *IsMapped = true;
> } else if (IsInHeaderMap && File) {
> if (IsMapped)
> *IsMapped = true;
> }
> ```
>
> but maybe my aesthetics are being thrown off by all the intervening comments
> in Phab. I'll leave it up to you.
I've tried the suggested approach but don't find it better. It is a personal
preference but I like how `*IsMapped |= ...` conveys the value is an
"aggregate" of the previous file lookups.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58094/new/
https://reviews.llvm.org/D58094
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
LGTM. I have one idea for you to consider inline.
Comment at: clang/lib/Lex/HeaderSearch.cpp:892-902
+IsInHeaderMap, MappedName);
+if (!MappedName.empty()) {
+ assert(IsInHeaderMap && "MappedName should come from a header map");
CacheLookup.MappedName =
- copyString(Filename, LookupFileCache.getAllocator());
- if (IsMapped)
-*IsMapped = true;
+ copyString(MappedName, LookupFileCache.getAllocator());
}
+if (IsMapped)
I wonder if this would be easier to follow if you refactored like this:
```
if (!MappedName.empty()) {
// other logic.
if (IsMapped)
*IsMapped = true;
} else if (IsInHeaderMap && File) {
if (IsMapped)
*IsMapped = true;
}
```
but maybe my aesthetics are being thrown off by all the intervening comments in
Phab. I'll leave it up to you.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58094/new/
https://reviews.llvm.org/D58094
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai marked 4 inline comments as done. vsapsai added a comment. Updated the code. Hope it is easier to understand now. Comment at: clang/lib/Lex/HeaderSearch.cpp:908-909 +if (IsMapped) + *IsMapped = CurrentInHeaderMap || HasBeenMapped; + dexonsmith wrote: > Are you intentionally delaying this until after the `if (!File)` check? If > so, why? Agree it is non-obvious and error-prone when changing the code. Changed it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai updated this revision to Diff 219194.
vsapsai added a comment.
- Add a test for unused absolute path in a header map; simplify code.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58094/new/
https://reviews.llvm.org/D58094
Files:
clang/include/clang/Lex/DirectoryLookup.h
clang/lib/Lex/HeaderSearch.cpp
clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
clang/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Bar.h
clang/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Baz.h
clang/test/Preprocessor/nonportable-include-with-hmap.c
Index: clang/test/Preprocessor/nonportable-include-with-hmap.c
===
--- clang/test/Preprocessor/nonportable-include-with-hmap.c
+++ clang/test/Preprocessor/nonportable-include-with-hmap.c
@@ -1,5 +1,9 @@
+// REQUIRES: shell
+
// RUN: rm -f %t.hmap
-// RUN: %hmaptool write %S/Inputs/nonportable-hmaps/foo.hmap.json %t.hmap
+// RUN: sed -e "s:INPUTS_DIR:%S/Inputs:g" \
+// RUN: %S/Inputs/nonportable-hmaps/foo.hmap.json > %t.hmap.json
+// RUN: %hmaptool write %t.hmap.json %t.hmap
// RUN: %clang_cc1 -Eonly\
// RUN: -I%t.hmap \
// RUN: -I%S/Inputs/nonportable-hmaps \
@@ -15,4 +19,16 @@
// 5. Return.
//
// There is nothing nonportable; -Wnonportable-include-path should not fire.
-#include "Foo/Foo.h" // expected-no-diagnostics
+#include "Foo/Foo.h" // no warning
+
+// Verify files with absolute paths in the header map are handled too.
+// "Bar.h" is included twice to make sure that when we see potentially
+// nonportable path, the file has been already discovered through a relative
+// path which triggers the file to be opened and `FileEntry::RealPathName`
+// to be set.
+#include "Bar.h"
+#include "Foo/Bar.h" // no warning
+
+// But the presence of the absolute path in the header map is not enough. If we
+// didn't use it to discover a file, shouldn't suppress the warning.
+#include "headers/Foo/Baz.h" // expected-warning {{non-portable path}}
Index: clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
===
--- clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
+++ clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
@@ -1,6 +1,9 @@
{
"mappings" :
{
- "Foo/Foo.h" : "headers/foo/Foo.h"
+ "Foo/Foo.h" : "headers/foo/Foo.h",
+ "Bar.h" : "headers/foo/Bar.h",
+ "Foo/Bar.h" : "INPUTS_DIR/nonportable-hmaps/headers/foo/Bar.h",
+ "headers/Foo/Baz.h" : "/not/existing/absolute/path/Baz.h"
}
}
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -341,9 +341,10 @@
SmallVectorImpl *SearchPath, SmallVectorImpl *RelativePath,
Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule,
bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound,
-bool &HasBeenMapped, SmallVectorImpl &MappedName) const {
+bool &IsInHeaderMap, SmallVectorImpl &MappedName) const {
InUserSpecifiedSystemFramework = false;
- HasBeenMapped = false;
+ IsInHeaderMap = false;
+ MappedName.clear();
SmallString<1024> TmpDir;
if (isNormalDir()) {
@@ -377,6 +378,8 @@
if (Dest.empty())
return None;
+ IsInHeaderMap = true;
+
auto FixupSearchPath = [&]() {
if (SearchPath) {
StringRef SearchPathRef(getName());
@@ -393,10 +396,8 @@
// ("Foo.h" -> "Foo/Foo.h"), in which case continue header lookup using the
// framework include.
if (llvm::sys::path::is_relative(Dest)) {
-MappedName.clear();
MappedName.append(Dest.begin(), Dest.end());
Filename = StringRef(MappedName.begin(), MappedName.size());
-HasBeenMapped = true;
Optional Result = HM->LookupFile(Filename, HS.getFileMgr());
if (Result) {
FixupSearchPath();
@@ -883,18 +884,22 @@
// Check each directory in sequence to see if it contains this file.
for (; i != SearchDirs.size(); ++i) {
bool InUserSpecifiedSystemFramework = false;
-bool HasBeenMapped = false;
+bool IsInHeaderMap = false;
bool IsFrameworkFoundInDir = false;
Optional File = SearchDirs[i].LookupFile(
Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule,
SuggestedModule, InUserSpecifiedSystemFramework, IsFrameworkFoundInDir,
-HasBeenMapped, MappedName);
-if (HasBeenMapped) {
+IsInHeaderMap, MappedName);
+if (!MappedName.empty()) {
+ assert(IsInHeaderMap && "MappedName should come from a header map");
CacheLookup.MappedName =
- copyString(Filename, LookupFileCache.getAllocator());
- if (IsMapped)
-*IsMapped = true;
+ copyString(MappedName, LookupFileCache.getAllocator());
}
+if (IsMapped)
+ // A filename is mapped when a header map remapped it to a relative path
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai marked 3 inline comments as done. vsapsai added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:888 bool InUserSpecifiedSystemFramework = false; -bool HasBeenMapped = false; +bool CurrentInHeaderMap = false; bool IsFrameworkFoundInDir = false; dexonsmith wrote: > Why not name this the same way as the parameter? (`IsInHeaderMap`) Wanted to emphasize it applies only to the current search directory. But I think what matters is to distinguish between being mentioned in header map and changing the search name. So I'll drop "Current" and suggestions for better naming are welcome. Comment at: clang/lib/Lex/HeaderSearch.cpp:896-897 + assert(CurrentInHeaderMap && "MappedName should come from a header map"); CacheLookup.MappedName = - copyString(Filename, LookupFileCache.getAllocator()); - if (IsMapped) -*IsMapped = true; + copyString(MappedName, LookupFileCache.getAllocator()); + HasBeenMapped = true; dexonsmith wrote: > It's not obvious to me why this changed from `Filename` to `MappedName`. Can > you explain it? Technically it doesn't matter because in this branch they are equal (would an assertion be useful?). But with my change `IsInHeaderMap` and `MappedName` can be different for absolute paths. So I think it is more natural ```lang=c++ if (!MappedName.empty()) CacheLookup.MappedName = copyString(MappedName); ``` than ```lang=c++ if (!MappedName.empty()) CacheLookup.MappedName = copyString(Filename); ``` In the second case I need to check that `LookupFile` can modify `Filename`. But I don't have a strong opinion about it. Comment at: clang/lib/Lex/HeaderSearch.cpp:898 + copyString(MappedName, LookupFileCache.getAllocator()); + HasBeenMapped = true; } dexonsmith wrote: > Do we need `HasBeenMapped` outside of the loop, or can you just use the > loop-local variable? Yes. Let me add a test case and a comment explaining it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
dexonsmith added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:888 bool InUserSpecifiedSystemFramework = false; -bool HasBeenMapped = false; +bool CurrentInHeaderMap = false; bool IsFrameworkFoundInDir = false; Why not name this the same way as the parameter? (`IsInHeaderMap`) Comment at: clang/lib/Lex/HeaderSearch.cpp:896-897 + assert(CurrentInHeaderMap && "MappedName should come from a header map"); CacheLookup.MappedName = - copyString(Filename, LookupFileCache.getAllocator()); - if (IsMapped) -*IsMapped = true; + copyString(MappedName, LookupFileCache.getAllocator()); + HasBeenMapped = true; It's not obvious to me why this changed from `Filename` to `MappedName`. Can you explain it? Comment at: clang/lib/Lex/HeaderSearch.cpp:898 + copyString(MappedName, LookupFileCache.getAllocator()); + HasBeenMapped = true; } Do we need `HasBeenMapped` outside of the loop, or can you just use the loop-local variable? Comment at: clang/lib/Lex/HeaderSearch.cpp:908-909 +if (IsMapped) + *IsMapped = CurrentInHeaderMap || HasBeenMapped; + Are you intentionally delaying this until after the `if (!File)` check? If so, why? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai updated this revision to Diff 218794.
vsapsai added a comment.
Herald added a subscriber: ributzka.
- Rebase the patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58094/new/
https://reviews.llvm.org/D58094
Files:
clang/include/clang/Lex/DirectoryLookup.h
clang/lib/Lex/HeaderSearch.cpp
clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
clang/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Bar.h
clang/test/Preprocessor/nonportable-include-with-hmap.c
Index: clang/test/Preprocessor/nonportable-include-with-hmap.c
===
--- clang/test/Preprocessor/nonportable-include-with-hmap.c
+++ clang/test/Preprocessor/nonportable-include-with-hmap.c
@@ -1,5 +1,9 @@
+// REQUIRES: shell
+
// RUN: rm -f %t.hmap
-// RUN: %hmaptool write %S/Inputs/nonportable-hmaps/foo.hmap.json %t.hmap
+// RUN: sed -e "s:INPUTS_DIR:%S/Inputs:g" \
+// RUN: %S/Inputs/nonportable-hmaps/foo.hmap.json > %t.hmap.json
+// RUN: %hmaptool write %t.hmap.json %t.hmap
// RUN: %clang_cc1 -Eonly\
// RUN: -I%t.hmap \
// RUN: -I%S/Inputs/nonportable-hmaps \
@@ -16,3 +20,11 @@
//
// There is nothing nonportable; -Wnonportable-include-path should not fire.
#include "Foo/Foo.h" // expected-no-diagnostics
+
+// Verify files with absolute paths in the header map are handled too.
+// "Bar.h" is included twice to make sure that when we see potentially
+// nonportable path, the file has been already discovered through a relative
+// path which triggers the file to be opened and `FileEntry::RealPathName`
+// to be set.
+#include "Bar.h"
+#include "Foo/Bar.h" // expected-no-diagnostics
Index: clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
===
--- clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
+++ clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
@@ -1,6 +1,8 @@
{
"mappings" :
{
- "Foo/Foo.h" : "headers/foo/Foo.h"
+ "Foo/Foo.h" : "headers/foo/Foo.h",
+ "Bar.h" : "headers/foo/Bar.h",
+ "Foo/Bar.h" : "INPUTS_DIR/nonportable-hmaps/headers/foo/Bar.h"
}
}
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -341,9 +341,10 @@
SmallVectorImpl *SearchPath, SmallVectorImpl *RelativePath,
Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule,
bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound,
-bool &HasBeenMapped, SmallVectorImpl &MappedName) const {
+bool &IsInHeaderMap, SmallVectorImpl &MappedName) const {
InUserSpecifiedSystemFramework = false;
- HasBeenMapped = false;
+ IsInHeaderMap = false;
+ MappedName.clear();
SmallString<1024> TmpDir;
if (isNormalDir()) {
@@ -377,6 +378,8 @@
if (Dest.empty())
return None;
+ IsInHeaderMap = true;
+
auto FixupSearchPath = [&]() {
if (SearchPath) {
StringRef SearchPathRef(getName());
@@ -393,10 +396,8 @@
// ("Foo.h" -> "Foo/Foo.h"), in which case continue header lookup using the
// framework include.
if (llvm::sys::path::is_relative(Dest)) {
-MappedName.clear();
MappedName.append(Dest.begin(), Dest.end());
Filename = StringRef(MappedName.begin(), MappedName.size());
-HasBeenMapped = true;
Optional Result = HM->LookupFile(Filename, HS.getFileMgr());
if (Result) {
FixupSearchPath();
@@ -879,21 +880,22 @@
}
SmallString<64> MappedName;
+ bool HasBeenMapped = false;
// Check each directory in sequence to see if it contains this file.
for (; i != SearchDirs.size(); ++i) {
bool InUserSpecifiedSystemFramework = false;
-bool HasBeenMapped = false;
+bool CurrentInHeaderMap = false;
bool IsFrameworkFoundInDir = false;
Optional File = SearchDirs[i].LookupFile(
Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule,
SuggestedModule, InUserSpecifiedSystemFramework, IsFrameworkFoundInDir,
-HasBeenMapped, MappedName);
-if (HasBeenMapped) {
+CurrentInHeaderMap, MappedName);
+if (!MappedName.empty()) {
+ assert(CurrentInHeaderMap && "MappedName should come from a header map");
CacheLookup.MappedName =
- copyString(Filename, LookupFileCache.getAllocator());
- if (IsMapped)
-*IsMapped = true;
+ copyString(MappedName, LookupFileCache.getAllocator());
+ HasBeenMapped = true;
}
if (IsFrameworkFound)
// Because we keep a filename remapped for subsequent search directory
@@ -903,6 +905,9 @@
if (!File)
continue;
+if (IsMapped)
+ *IsMapped = CurrentInHeaderMap || HasBeenMapped;
+
CurDir = &SearchDirs[i];
// This file is a system header or C++ unfriendly if the dir is.
Index: clang/include/clang/Lex/DirectoryLookup.h
===
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai marked an inline comment as done.
vsapsai added inline comments.
Comment at: clang/lib/Lex/HeaderSearch.cpp:388
} else {
Result = HS.getFileMgr().getFile(Dest);
}
I have considered changing this to `.getFile(Dest, /*OpenFile=*/true)` so that
the bug can be triggered without a double include. But decided not to do so
because it seems weird to open a file (and make an extra syscall) to make it
easier to ignore the filename case mismatch later.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58094/new/
https://reviews.llvm.org/D58094
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.
vsapsai created this revision.
vsapsai added reviewers: dexonsmith, bruno.
Herald added a subscriber: jkorous.
In `DirectoryLookup::LookupFile` parameter `HasBeenMapped` doesn't cover
the case when clang finds a file through a header map but doesn't remap
the lookup filename because the target path is an absolute path. As a
result, -Wnonportable-include-path suppression for header maps
introduced in r301592 wasn't triggered.
Change parameter `HasBeenMapped` to `IsInHeaderMap` and use parameter
`MappedName` to track the filename remapping. This way we can handle
both relative and absolute paths in header maps, and account for their
specific properties, like filename remapping being a property preserved
across lookups in multiple directories.
rdar://problem/39516483
https://reviews.llvm.org/D58094
Files:
clang/include/clang/Lex/DirectoryLookup.h
clang/lib/Lex/HeaderSearch.cpp
clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
clang/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Bar.h
clang/test/Preprocessor/nonportable-include-with-hmap.c
Index: clang/test/Preprocessor/nonportable-include-with-hmap.c
===
--- clang/test/Preprocessor/nonportable-include-with-hmap.c
+++ clang/test/Preprocessor/nonportable-include-with-hmap.c
@@ -1,5 +1,9 @@
+// REQUIRES: shell
+
// RUN: rm -f %t.hmap
-// RUN: %hmaptool write %S/Inputs/nonportable-hmaps/foo.hmap.json %t.hmap
+// RUN: sed -e "s:INPUTS_DIR:%S/Inputs:g" \
+// RUN: %S/Inputs/nonportable-hmaps/foo.hmap.json > %t.hmap.json
+// RUN: %hmaptool write %t.hmap.json %t.hmap
// RUN: %clang_cc1 -Eonly\
// RUN: -I%t.hmap \
// RUN: -I%S/Inputs/nonportable-hmaps \
@@ -16,3 +20,11 @@
//
// There is nothing nonportable; -Wnonportable-include-path should not fire.
#include "Foo/Foo.h" // expected-no-diagnostics
+
+// Verify files with absolute paths in the header map are handled too.
+// "Bar.h" is included twice to make sure that when we see potentially
+// nonportable path, the file has been already discovered through a relative
+// path which triggers the file to be opened and `FileEntry::RealPathName`
+// to be set.
+#include "Bar.h"
+#include "Foo/Bar.h" // expected-no-diagnostics
Index: clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
===
--- clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
+++ clang/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json
@@ -1,6 +1,8 @@
{
"mappings" :
{
- "Foo/Foo.h" : "headers/foo/Foo.h"
+ "Foo/Foo.h" : "headers/foo/Foo.h",
+ "Bar.h" : "headers/foo/Bar.h",
+ "Foo/Bar.h" : "INPUTS_DIR/nonportable-hmaps/headers/foo/Bar.h"
}
}
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -335,10 +335,11 @@
ModuleMap::KnownHeader *SuggestedModule,
bool &InUserSpecifiedSystemFramework,
bool &IsFrameworkFound,
-bool &HasBeenMapped,
+bool &IsInHeaderMap,
SmallVectorImpl &MappedName) const {
InUserSpecifiedSystemFramework = false;
- HasBeenMapped = false;
+ IsInHeaderMap = false;
+ MappedName.clear();
SmallString<1024> TmpDir;
if (isNormalDir()) {
@@ -372,16 +373,16 @@
if (Dest.empty())
return nullptr;
+ IsInHeaderMap = true;
+
const FileEntry *Result;
// Check if the headermap maps the filename to a framework include
// ("Foo.h" -> "Foo/Foo.h"), in which case continue header lookup using the
// framework include.
if (llvm::sys::path::is_relative(Dest)) {
-MappedName.clear();
MappedName.append(Dest.begin(), Dest.end());
Filename = StringRef(MappedName.begin(), MappedName.size());
-HasBeenMapped = true;
Result = HM->LookupFile(Filename, HS.getFileMgr());
} else {
Result = HS.getFileMgr().getFile(Dest);
@@ -852,26 +853,30 @@
}
SmallString<64> MappedName;
+ bool HasBeenMapped = false;
// Check each directory in sequence to see if it contains this file.
for (; i != SearchDirs.size(); ++i) {
bool InUserSpecifiedSystemFramework = false;
-bool HasBeenMapped = false;
+bool CurrentInHeaderMap = false;
bool IsFrameworkFoundInDir = false;
const FileEntry *FE = SearchDirs[i].LookupFile(
Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule,
SuggestedModule, InUserSpecifiedSystemFramework, IsFrameworkFoundInDir,
-HasBeenMapped, MappedName);
-if (HasBeenMapped) {
+CurrentInHeaderMap, MappedName);
+if (!MappedName.empty()) {
+ assert(CurrentInHeaderMap && "MappedName should come from a header map");
CacheLookup.MappedName =
- copyString(Filename, LookupFileCache.getAllocator());
- if (IsMapped)
-*IsMapped = true;
+ copyString(MappedName, L
