From f5d449a52774ee2e43614ab6cffd5408247a598b Mon Sep 17 00:00:00 2001 From: Volodymyr Sapsai <vsapsai@apple.com> Date: Wed, 11 Sep 2019 20:39:04 +0000 Subject: [PATCH] Fix -Wnonportable-include-path suppression for header maps with absolute paths. 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 Reviewers: dexonsmith, bruno Reviewed By: dexonsmith Subscribers: jkorous, cfe-commits, ributzka Differential Revision: https://reviews.llvm.org/D58094 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@371655 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Lex/DirectoryLookup.h | 2 +- lib/Lex/HeaderSearch.cpp | 25 +++++++++++-------- .../Inputs/nonportable-hmaps/foo.hmap.json | 5 +++- .../nonportable-hmaps/headers/foo/Bar.h | 0 .../nonportable-hmaps/headers/foo/Baz.h | 0 .../nonportable-include-with-hmap.c | 20 +++++++++++++-- 6 files changed, 38 insertions(+), 14 deletions(-) create mode 100644 test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Bar.h create mode 100644 test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Baz.h diff --git a/include/clang/Lex/DirectoryLookup.h b/include/clang/Lex/DirectoryLookup.h index 8f321b5b7a..d526319a68 100644 --- a/include/clang/Lex/DirectoryLookup.h +++ b/include/clang/Lex/DirectoryLookup.h @@ -183,7 +183,7 @@ public: SmallVectorImpl<char> *RelativePath, Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound, - bool &HasBeenMapped, SmallVectorImpl<char> &MappedName) const; + bool &IsInHeaderMap, SmallVectorImpl<char> &MappedName) const; private: Optional<FileEntryRef> DoFrameworkLookup( diff --git a/lib/Lex/HeaderSearch.cpp b/lib/Lex/HeaderSearch.cpp index ddbb4d4b42..79677de936 100644 --- a/lib/Lex/HeaderSearch.cpp +++ b/lib/Lex/HeaderSearch.cpp @@ -341,9 +341,10 @@ Optional<FileEntryRef> DirectoryLookup::LookupFile( SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath, Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound, - bool &HasBeenMapped, SmallVectorImpl<char> &MappedName) const { + bool &IsInHeaderMap, SmallVectorImpl<char> &MappedName) const { InUserSpecifiedSystemFramework = false; - HasBeenMapped = false; + IsInHeaderMap = false; + MappedName.clear(); SmallString<1024> TmpDir; if (isNormalDir()) { @@ -377,6 +378,8 @@ Optional<FileEntryRef> DirectoryLookup::LookupFile( if (Dest.empty()) return None; + IsInHeaderMap = true; + auto FixupSearchPath = [&]() { if (SearchPath) { StringRef SearchPathRef(getName()); @@ -393,10 +396,8 @@ Optional<FileEntryRef> DirectoryLookup::LookupFile( // ("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<FileEntryRef> Result = HM->LookupFile(Filename, HS.getFileMgr()); if (Result) { FixupSearchPath(); @@ -883,18 +884,22 @@ Optional<FileEntryRef> HeaderSearch::LookupFile( // 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<FileEntryRef> 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 diff --git a/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json b/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json index c69f1df772..4852f363e0 100644 --- a/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json +++ b/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" } } diff --git a/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Bar.h b/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Bar.h new file mode 100644 index 0000000000..e69de29bb2 diff --git a/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Baz.h b/test/Preprocessor/Inputs/nonportable-hmaps/headers/foo/Baz.h new file mode 100644 index 0000000000..e69de29bb2 diff --git a/test/Preprocessor/nonportable-include-with-hmap.c b/test/Preprocessor/nonportable-include-with-hmap.c index bbbd669a0f..6fd407c4fa 100644 --- a/test/Preprocessor/nonportable-include-with-hmap.c +++ b/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}} -- 2.40.0