[clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[DmitryPolukhin]

Summary

suggestPathToFileForDiagnostics is actively used in clangd for converting
an absolute path to a header file to a header name as it should be spelled
in the sources. Current approach converts absolute path to relative path.
This diff implements missing logic that makes a reverse lookup from the
relative path to the key in the header map that should be used in the sources.

Prerequisite diff https://reviews.llvm.org/D103229

Test Plan: check-clang

[DmitryPolukhin]

Fix clang-format issue

[DmitryPolukhin]

Fix linter

[bruno]

clang/lib/Lex/HeaderSearch.cpp

1851–1864

-> filename

1855

Can we save some dir scanning time by adding this logic to the previous loop? Shouldn't get hard to read if you early continue for each failed condition.

1859

I guess one of the rationale behind this change is that whatever is consuming your diagnostics is expecting the hmap key entry as an actionable path they could use.

I wonder whether other consumers of suggestPathToFileForDiagnostics expect an actual mapped value or just don't care. If the former, this might be better under some flag.

@dexonsmith @vsapsai @JDevlieghere @arphaman how does this relate with what you expect out of suggestPathToFileForDiagnostics? (If at all).

[dexonsmith]

Naively, this sounds like it could be a non-trivial tax on build times. But it looks like it's only called in Clang from Sema::diagnoseMissingImport, which only happens on error anyway.

clang/unittests/Lex/HeaderMapTest.cpp

9 ↗

(On Diff #347864)

Splitting this out seems like a great idea, but please split it out to a separate prep commit that's NFC.

[DmitryPolukhin]

Split changes into NFC https://reviews.llvm.org/D103229 and the rest

[DmitryPolukhin]

In D103142#2783665, @dexonsmith wrote:

Naively, this sounds like it could be a non-trivial tax on build times. But it looks like it's only called in Clang from Sema::diagnoseMissingImport, which only happens on error anyway.

Yes, in Clang suggestPathToFileForDiagnostics is used only in Sema::diagnoseMissingImport. In addition to clangd this function is also used in clang-include-fixer but new logic should also work there too (add @bkramer author of clang-include-fixer). This diff builds reverse index lazy only when it is required so it shouldn't normal build speed.

clang/lib/Lex/HeaderSearch.cpp

1855

It seems that unfortunately no, because we need the previous loop to convert absolute path into relative and this loop to convert relative path to key in header map. Normal header lookup also happens as two lookups: first lookup uses header map to convert key to relative path and then another lookup of the relative path.

1855

It seems that unfortunately no, because we need the previous loop to convert absolute path into relative and this loop to convert relative path to key in header map. Normal header lookup also happens as two lookups: first lookup uses header map to convert key to relative path and then another lookup of the relative path.

1859

I think it is safe to change default behaviour without introducing new flag/option because I don't expect that anyone really needs value from header map search in the source.

clang/unittests/Lex/HeaderMapTest.cpp

9 ↗

(On Diff #347864)

Done, https://reviews.llvm.org/D103229

[DmitryPolukhin]

Fix forgotten comment

[bruno]

Overall looks good, few remaining nitpicks.

clang/lib/Lex/HeaderMap.cpp

245

Please rewrite this to early return in case this isn't empty.

clang/lib/Lex/HeaderSearch.cpp

1856

Similar from above: rewrite to early continue if it's not meeting the conditions.

[DmitryPolukhin]

Resolved comments

clang/lib/Lex/HeaderMap.cpp

245

Done, but it causes a bit of code duplication due to second lookup. Please let me know if you thought about rewriting it somehow else.

[DmitryPolukhin]

Rebase + try build on Windows again

[bruno]

clang/lib/Lex/HeaderMap.cpp

265

How about something along the lines of:

...
StringRef Key;
for (unsigned i = 0; i != NumBuckets; ++i) {
    HMapBucket B = getBucket(i);
    if (B.Key == HMAP_EmptyBucketKey)
      continue;

    Key = getString(B.Key);
    Optional<StringRef> Prefix = getString(B.Prefix);
    Optional<StringRef> Suffix = getString(B.Suffix);
    if (!Key.empty() && Prefix && Suffix) {
      SmallVector<char, 1024> Buf;
      Buf.append(Prefix->begin(), Prefix->end());
      Buf.append(Suffix->begin(), Suffix->end());
      ReverseMap[StringRef(Buf.begin(), Buf.size())] = Key;
      break;
    }
}
assert(!Key.empty() && "expected valid key");
return Key;

While proposing this change I've noticed that it would keep looking for other buckets even in face of a valid result, so I've added a break. Was that intentional?

[DmitryPolukhin]

Updated HeaderMapImpl::reverseLookupFilename according to the review comment

[DmitryPolukhin]

clang/lib/Lex/HeaderMap.cpp

265

Yes, keep iterating over all key-value pairs in the header map is important to add all elements to the map for subsequent lookups. If we stop on the current match, next lookup will not even try to populate the reverse lookup map (because the map is not empty) but not all elements are in the map due to early return from previous lookup, so subsequent lookup may not find the match even if it exists in the header map. We also cannot expect that DestPath is present in the given header map, so we cannot assert if it is missing. This function should return empty string as an indication that DestPath is not found.

Updated implementation with the review suggestion, please take another look.

[bruno]

LGTM!

[DmitryPolukhin]

@bruno and @dexonsmith thank you for the review!

clang/include/clang/Lex/HeaderMap.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LEX_HEADERMAP_H
 #define LLVM_CLANG_LEX_HEADERMAP_H
 
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include <memory>
 
 namespace clang {
 
 struct HMapBucket;
 struct HMapHeader;
 
 /// Implementation for \a HeaderMap that doesn't depend on \a FileManager.
 class HeaderMapImpl {
   std::unique_ptr<const llvm::MemoryBuffer> FileBuffer;
   bool NeedsBSwap;
+  mutable llvm::StringMap<StringRef> ReverseMap;
 
 public:
   HeaderMapImpl(std::unique_ptr<const llvm::MemoryBuffer> File, bool NeedsBSwap)
       : FileBuffer(std::move(File)), NeedsBSwap(NeedsBSwap) {}
 
   // Check for a valid header and extract the byte swap.
   static bool checkHeader(const llvm::MemoryBuffer &File, bool &NeedsByteSwap);
 
   /// If the specified relative filename is located in this HeaderMap return
   /// the filename it is mapped to, otherwise return an empty StringRef.
   StringRef lookupFilename(StringRef Filename,
                            SmallVectorImpl<char> &DestPath) const;
 
   /// Return the filename of the headermap.
   StringRef getFileName() const;
 
   /// Print the contents of this headermap to stderr.
   void dump() const;
 
+  /// Return key for specifed path.
+  StringRef reverseLookupFilename(StringRef DestPath) const;
+
 private:
   unsigned getEndianAdjustedWord(unsigned X) const;
   const HMapHeader &getHeader() const;
   HMapBucket getBucket(unsigned BucketNo) const;
 
   /// Look up the specified string in the string table.  If the string index is
   /// not valid, return None.
   Optional<StringRef> getString(unsigned StrTabIdx) const;
public:
 
   /// Check to see if the specified relative filename is located in this
   /// HeaderMap.  If so, open it and return its FileEntry.  If RawPath is not
   /// NULL and the file is found, RawPath will be set to the raw path at which
   /// the file was found in the file system. For example, for a search path
   /// ".." and a filename "../file.h" this would be "../../file.h".
   Optional<FileEntryRef> LookupFile(StringRef Filename, FileManager &FM) const;
 
-  using HeaderMapImpl::lookupFilename;
-  using HeaderMapImpl::getFileName;
   using HeaderMapImpl::dump;
+  using HeaderMapImpl::getFileName;
+  using HeaderMapImpl::lookupFilename;
+  using HeaderMapImpl::reverseLookupFilename;
 };
 
 } // end namespace clang.
 
 #endif

clang/lib/Lex/HeaderMap.cpp

for (unsigned Bucket = HashHMapKey(Filename);; ++Bucket) {
     DestPath.clear();
     if (LLVM_LIKELY(Prefix && Suffix)) {
       DestPath.append(Prefix->begin(), Prefix->end());
       DestPath.append(Suffix->begin(), Suffix->end());
     }
     return StringRef(DestPath.begin(), DestPath.size());
   }
 }
+
+StringRef HeaderMapImpl::reverseLookupFilename(StringRef DestPath) const {
+  if (ReverseMap.empty()) {
+    const HMapHeader &Hdr = getHeader();
+    unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
+    for (unsigned i = 0; i != NumBuckets; ++i) {
+      HMapBucket B = getBucket(i);
+      if (B.Key == HMAP_EmptyBucketKey)
+        continue;
+
+      Optional<StringRef> Key = getString(B.Key);
+      Optional<StringRef> Prefix = getString(B.Prefix);
+      Optional<StringRef> Suffix = getString(B.Suffix);
+      if (Key && Prefix && Suffix) {
+        SmallVector<char, 1024> Buf;
+        Buf.append(Prefix->begin(), Prefix->end());
+        Buf.append(Suffix->begin(), Suffix->end());
+        ReverseMap[StringRef(Buf.begin(), Buf.size())] = *Key;
+      }
+    }
+  }
+  return ReverseMap.lookup(DestPath);
+}

clang/lib/Lex/HeaderSearch.cpp

for (auto NI = path::begin(File), NE = path::end(File),
 
       if (*NI != *DI)
         break;
     }
     return false;
   };
 
   for (unsigned I = 0; I != SearchDirs.size(); ++I) {
-    // FIXME: Support this search within frameworks and header maps.
+    // FIXME: Support this search within frameworks.
     if (!SearchDirs[I].isNormalDir())
       continue;
 
     StringRef Dir = SearchDirs[I].getDir()->getName();
     if (CheckDir(Dir) && IsSystem)
       *IsSystem = BestPrefixLength ? I >= SystemDirIdx : false;
   }
 
   // Try to shorten include path using TUs directory, if we couldn't find any
   // suitable prefix in include search paths.
   if (!BestPrefixLength && CheckDir(path::parent_path(MainFile)) && IsSystem)
     *IsSystem = false;
 
-
-  return path::convert_to_slash(File.drop_front(BestPrefixLength));
+  // Try resolving resulting filaname via reverse search in header maps,
+  // key from header name is user prefered name for the include file.
+  StringRef Filename = File.drop_front(BestPrefixLength);
+  for (unsigned I = 0; I != SearchDirs.size(); ++I) {
+    if (SearchDirs[I].isHeaderMap()) {
+      StringRef SpelledFilename =
+          SearchDirs[I].getHeaderMap()->reverseLookupFilename(Filename);
+      if (!SpelledFilename.empty()) {
+        Filename = SpelledFilename;
+        break;
+      }
+    }
+  }
+  return path::convert_to_slash(Filename);
 }

clang/unittests/Lex/HeaderSearchTest.cpp

 //===- unittests/Lex/HeaderSearchTest.cpp ------ HeaderSearch tests -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Lex/HeaderSearch.h"
+#include "HeaderMapTestUtils.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Lex/HeaderSearch.h"
void addSearchDir(llvm::StringRef Dir) {
     VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
                  /*Group=*/None, llvm::sys::fs::file_type::directory_file);
     auto DE = FileMgr.getOptionalDirectoryRef(Dir);
     assert(DE);
     auto DL = DirectoryLookup(*DE, SrcMgr::C_User, /*isFramework=*/false);
     Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void addHeaderMap(llvm::StringRef Filename,
+                    std::unique_ptr<llvm::MemoryBuffer> Buf) {
+    VFS->addFile(Filename, 0, std::move(Buf), /*User=*/None, /*Group=*/None,
+                 llvm::sys::fs::file_type::regular_file);
+    auto FE = FileMgr.getFile(Filename, true);
+    assert(FE);
+
+    // Test class supports only one HMap at a time.
+    assert(!HMap);
+    HMap = HeaderMap::Create(*FE, FileMgr);
+    auto DL =
+        DirectoryLookup(HMap.get(), SrcMgr::C_User, /*isFramework=*/false);
+    Search.AddSearchPath(DL, /*isAngled=*/false);
+  }
+
   IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID;
   DiagnosticsEngine Diags;
   SourceManager SourceMgr;
   LangOptions LangOpts;
   std::shared_ptr<TargetOptions> TargetOpts;
   IntrusiveRefCntPtr<TargetInfo> Target;
   HeaderSearch Search;
+  std::unique_ptr<HeaderMap> HMap;
 };
 
 TEST_F(HeaderSearchTest, NoSearchDir) {
   EXPECT_EQ(Search.search_dir_size(), 0u);
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/y/z", /*WorkingDir=*/"",
                                                    /*MainFile=*/""),
             "/x/y/z");
 }
TEST_F(HeaderSearchTest, IncludeFromSameDirectory) {
 
   addSearchDir("/");
   EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/y/z/t.h",
                                                    /*WorkingDir=*/"",
                                                    /*MainFile=*/"/y/a.cc"),
             "y/z/t.h");
 }
 
+// Helper struct with null terminator character to make MemoryBuffer happy.
+template <class FileTy, class PaddingTy>
+struct NullTerminatedFile : public FileTy {
+  PaddingTy Padding = 0;
+};
+
+TEST_F(HeaderSearchTest, HeaderMapReverseLookup) {
+  typedef NullTerminatedFile<test::HMapFileMock<2, 32>, char> FileTy;
+  FileTy File;
+  File.init();
+
+  test::HMapFileMockMaker<FileTy> Maker(File);
+  auto a = Maker.addString("d.h");
+  auto b = Maker.addString("b/");
+  auto c = Maker.addString("c.h");
+  Maker.addBucket("d.h", a, b, c);
+
+  addHeaderMap("/x/y/z.hmap", File.getBuffer());
+  addSearchDir("/a");
+
+  EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/a/b/c.h",
+                                                   /*WorkingDir=*/"",
+                                                   /*MainFile=*/""),
+            "d.h");
+}
+
 } // namespace
 } // namespace clang