Fix -Wnonportable-include-path suppression for header maps with absolute paths.

[vsapsai]

Summary

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

[vsapsai]

clang/lib/Lex/HeaderSearch.cpp

388 ↗

(On Diff #186389)

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.

[vsapsai]

Ping.

[vsapsai]

Ping.

[vsapsai]

Ping.

[vsapsai]

• Rebase the patch.

[vsapsai]

Ping.

[dexonsmith]

clang/lib/Lex/HeaderSearch.cpp

888 ↗

(On Diff #218794)

Why not name this the same way as the parameter? (IsInHeaderMap)

896–897 ↗

(On Diff #218794)

It's not obvious to me why this changed from Filename to MappedName. Can you explain it?

898 ↗

(On Diff #218794)

Do we need HasBeenMapped outside of the loop, or can you just use the loop-local variable?

908–909 ↗

(On Diff #218794)

Are you intentionally delaying this until after the if (!File) check? If so, why?

[vsapsai]

clang/lib/Lex/HeaderSearch.cpp

888 ↗

(On Diff #218794)

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.

896–897 ↗

(On Diff #218794)

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

if (!MappedName.empty())
  CacheLookup.MappedName = copyString(MappedName);

than

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.

898 ↗

(On Diff #218794)

Yes. Let me add a test case and a comment explaining it.

[vsapsai]

• Add a test for unused absolute path in a header map; simplify code.

[vsapsai]

Updated the code. Hope it is easier to understand now.

clang/lib/Lex/HeaderSearch.cpp

908–909 ↗

(On Diff #218794)

Agree it is non-obvious and error-prone when changing the code. Changed it.

[dexonsmith]

LGTM. I have one idea for you to consider inline.

clang/lib/Lex/HeaderSearch.cpp

892–902 ↗

(On Diff #219194)

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.

[vsapsai]

Thanks for the review.

clang/lib/Lex/HeaderSearch.cpp

892–902 ↗

(On Diff #219194)

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.

cfe/trunk/include/clang/Lex/DirectoryLookup.h

public:
   /// a framework include ("Foo.h" -> "Foo/Foo.h"), set the new name to this
   /// vector and point Filename to it.
   Optional<FileEntryRef>
   LookupFile(StringRef &Filename, HeaderSearch &HS, SourceLocation IncludeLoc,
              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;
 
 private:
   Optional<FileEntryRef> DoFrameworkLookup(
       StringRef Filename, HeaderSearch &HS, SmallVectorImpl<char> *SearchPath,
       SmallVectorImpl<char> *RelativePath, Module *RequestingModule,
       ModuleMap::KnownHeader *SuggestedModule,
       bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound) const;
 };
 
 }  // end namespace clang
 
 #endif

cfe/trunk/lib/Lex/HeaderSearch.cpp

 
 /// LookupFile - Lookup the specified file in this search path, returning it
 /// if it exists or returning null if not.
 Optional<FileEntryRef> DirectoryLookup::LookupFile(
     StringRef &Filename, HeaderSearch &HS, SourceLocation IncludeLoc,
     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()) {
     // Concatenate the requested file onto the directory.
     TmpDir = getDir()->getName();
     llvm::sys::path::append(TmpDir, Filename);
     if (SearchPath) {
       StringRef SearchPathRef(getDir()->getName());
Optional<FileEntryRef> DirectoryLookup::LookupFile(
 
   assert(isHeaderMap() && "Unknown directory lookup");
   const HeaderMap *HM = getHeaderMap();
   SmallString<1024> Path;
   StringRef Dest = HM->lookupFilename(Filename, Path);
   if (Dest.empty())
     return None;
 
+  IsInHeaderMap = true;
+
   auto FixupSearchPath = [&]() {
     if (SearchPath) {
       StringRef SearchPathRef(getName());
       SearchPath->clear();
       SearchPath->append(SearchPathRef.begin(), SearchPathRef.end());
     }
     if (RelativePath) {
       RelativePath->clear();
       RelativePath->append(Filename.begin(), Filename.end());
     }
   };
 
   // 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;
     Optional<FileEntryRef> Result = HM->LookupFile(Filename, HS.getFileMgr());
     if (Result) {
       FixupSearchPath();
       return *Result;
     }
   } else if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest)) {
     FixupSearchPath();
     return *Res;
if (!SkipCache && CacheLookup.StartIdx == i+1) {
     CacheLookup.reset(/*StartIdx=*/i+1);
   }
 
   SmallString<64> MappedName;
 
   // 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
       // just for remapping in a current search directory.
       *IsFrameworkFound |= (IsFrameworkFoundInDir && !CacheLookup.MappedName);
     if (!File)
       continue;
 

cfe/trunk/test/Preprocessor/Inputs/nonportable-hmaps/foo.hmap.json

 {
   "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"
     }
 }

cfe/trunk/test/Preprocessor/nonportable-include-with-hmap.c

+// 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          \
 // RUN:   %s -verify
 //
 // foo.hmap contains: Foo/Foo.h -> headers/foo/Foo.h
 //
 // Header search of "Foo/Foo.h" follows this path:
 //  1. Look for "Foo/Foo.h".
 //  2. Find "Foo/Foo.h" in "nonportable-hmaps/foo.hmap".
 //  3. Look for "headers/foo/Foo.h".
 //  4. Find "headers/foo/Foo.h" in "nonportable-hmaps".
 //  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}}