[Preprocessor] Fix crash emitting note with framework location for "file not found" error.

[vsapsai]

Summary

A filename can be remapped with a header map to point to a framework
header and we can find the corresponding framework without the header.
But if the original filename doesn't have a remapped framework name,
we'll fail to find its location and will dereference a null pointer
during diagnostics emission.

Fix by tracking remappings better and emit the note only if a framework
is found before any of the remappings.

rdar://problem/48883447

[vsapsai]

Ping.

[vsapsai]

Ping.

[arphaman]

clang/lib/Lex/HeaderSearch.cpp

821

NIT: It looks like HasBeenMapped should be always set when CacheLookup.MappedName is set as well. Would it make sense to check CacheLookup.MappedName instead to avoid divergence in the future? You could have a helper lambda HasBeenMapped = [] (&) { return CacheLookup.MappedName; } as well.

clang/test/Preprocessor/include-header-missing-in-framework-with-headermap.c

10

would it make sense to exercise both #ifdef in the test in the two clang invocations, but guard the expected-errors with the #ifdef? This way the test can ensure that the error is not emitted in the other invocation.

[vsapsai]

clang/lib/Lex/HeaderSearch.cpp

821

Good point, will change. Now that you've mentioned it, keeping IsMapped and CacheLookup.MappedName in sync doesn't look great.

clang/test/Preprocessor/include-header-missing-in-framework-with-headermap.c

10

The tricky part is that "file not found" is a fatal error and %clang_cc1 will ignore subsequent errors. So we cannot test both includes in the same invocation. It is possible to test all combinations by adding 2 more clang invocations but those extra cases aren't very useful.

[vsapsai]

• Address review comments: don't introduce HasBeenMapped and rely on CacheLookup.MappedName.

Changes are rebased, so diff between diffs can be noisy.

[arphaman]

LGTM

[vsapsai]

Thanks for the review, Alex.

clang/include/clang/Lex/HeaderSearch.h

public:
   /// \param SuggestedModule If non-null, and the file found is semantically
   /// part of a known module, this will be set to the module that should
   /// be imported instead of preprocessing/parsing the file found.
   ///
   /// \param IsMapped If non-null, and the search involved header maps, set to
   /// true.
   ///
   /// \param IsFrameworkFound If non-null, will be set to true if a framework is
-  /// found in any of searched SearchDirs. Doesn't guarantee the requested file
-  /// is found.
+  /// found in any of searched SearchDirs. Will be set to false if a framework
+  /// is found only through header maps. Doesn't guarantee the requested file is
+  /// found.
   const FileEntry *LookupFile(
       StringRef Filename, SourceLocation IncludeLoc, bool isAngled,
       const DirectoryLookup *FromDir, const DirectoryLookup *&CurDir,
       ArrayRef<std::pair<const FileEntry *, const DirectoryEntry *>> Includers,
       SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath,
       Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule,
       bool *IsMapped, bool *IsFrameworkFound, bool SkipCache = false,
       bool BuildSystemModule = false);

clang/lib/Lex/HeaderSearch.cpp

for (const auto &IncluderAndDir : Includers) {
           break;
         }
       }
       First = false;
     }
   }
 
   CurDir = nullptr;
+  bool HasBeenMapped = false;
 
   // If this is a system #include, ignore the user #include locs.
   unsigned i = isAngled ? AngledDirIdx : 0;
 
   // If this is a #include_next request, start searching after the directory the
   // file was found in.
   if (FromDir)
     i = FromDir-&SearchDirs[0];
 
   // Cache all of the lookups performed by this method.  Many headers are
   // multiply included, and the "pragma once" optimization prevents them from
   // being relex/pp'd, but they would still have to search through a
   // (potentially huge) series of SearchDirs to find it.
   LookupFileCacheInfo &CacheLookup = LookupFileCache[Filename];
 
   // If the entry has been previously looked up, the first value will be
   // non-zero.  If the value is equal to i (the start point of our search), then
   // this is a matching hit.
   if (!SkipCache && CacheLookup.StartIdx == i+1) {
     // Skip querying potentially lots of directories for this lookup.
     i = CacheLookup.HitIdx;
     if (CacheLookup.MappedName) {
       Filename = CacheLookup.MappedName;
+      HasBeenMapped = true;
       if (IsMapped)
         *IsMapped = true;
     }
   } else {
     // Otherwise, this is the first query, or the previous query didn't match
     // our search start.  We will fill in our found location below, so prime the
     // start point value.
     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 HasBeenMappedInDir = false;
     bool IsFrameworkFoundInDir = false;
     const FileEntry *FE = SearchDirs[i].LookupFile(
         Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule,
         SuggestedModule, InUserSpecifiedSystemFramework, IsFrameworkFoundInDir,
-        HasBeenMapped, MappedName);
-    if (HasBeenMapped) {
+        HasBeenMappedInDir, MappedName);
+    if (HasBeenMappedInDir) {
       CacheLookup.MappedName =
           copyString(Filename, LookupFileCache.getAllocator());
+      HasBeenMapped = true;
       if (IsMapped)
         *IsMapped = true;
     }
     if (IsFrameworkFound)
-      *IsFrameworkFound |= IsFrameworkFoundInDir;
+      // 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 && !HasBeenMapped);
     if (!FE) continue;
 
     CurDir = &SearchDirs[i];
 
     // This file is a system header or C++ unfriendly if the dir is.
     HeaderFileInfo &HFI = getFileInfo(FE);
     HFI.DirInfo = CurDir->getDirCharacteristic();
 
for (; i != SearchDirs.size(); ++i) {
     }
 
     if (checkMSVCHeaderSearch(Diags, MSFE, FE, IncludeLoc)) {
       if (SuggestedModule)
         *SuggestedModule = MSSuggestedModule;
       return MSFE;
     }
 
-    bool FoundByHeaderMap = !IsMapped ? false : *IsMapped;
     if (!Includers.empty())
       diagnoseFrameworkInclude(Diags, IncludeLoc,
                                Includers.front().second->getName(), Filename,
-                               FE, isAngled, FoundByHeaderMap);
+                               FE, isAngled, HasBeenMapped);
 
     // Remember this location for the next lookup we do.
     CacheLookup.HitIdx = i;
     return FE;
   }
 
   // If we are including a file with a quoted include "foo.h" from inside
   // a header in a framework that is currently being built, and we couldn't

clang/test/Preprocessor/Inputs/include-header-missing-in-framework/TestFramework.hmap.json

+{
+  "mappings" :
+    {
+     "RemappedHeader.h" : "TestFramework/RemappedHeader.h",
+     "TestFramework/BeforeRemapping.h" : "TestFramework/AfterRemapping.h"
+    }
+}

clang/test/Preprocessor/include-header-missing-in-framework-with-headermap.c

+// RUN: rm -f %t.hmap
+// RUN: %hmaptool write %S/Inputs/include-header-missing-in-framework/TestFramework.hmap.json %t.hmap
+// RUN: %clang_cc1 -fsyntax-only -F %S/Inputs -I %t.hmap -verify %s -DLATE_REMAPPING
+// RUN: %clang_cc1 -fsyntax-only -I %t.hmap -F %S/Inputs -verify %s
+
+// The test is similar to 'include-header-missing-in-framework.c' but covers
+// the case when a header is remapped to a framework-like path with a .hmap
+// file. And we can find the framework but not the header.
+
+#ifdef LATE_REMAPPING
+// Framework is found before remapping.
+#include <TestFramework/BeforeRemapping.h>
+// expected-error@-1 {{'TestFramework/BeforeRemapping.h' file not found}}
+// expected-note@-2 {{did not find header 'BeforeRemapping.h' in framework 'TestFramework' (loaded from}}
+
+#else
+// Framework is found after remapping.
+#include "RemappedHeader.h"
+// expected-error@-1 {{'RemappedHeader.h' file not found}}
+#endif // LATE_REMAPPING