Support relative dest paths in headermap files

[sque]

Summary

`Existing behavior:

• If hmap dest path is absolute: = Look for the file and return it as result. = If file not found, return None.
• If hmap dest path is relative: = Look for entry in headermap with the dest path as key, where the dest is a file that exists. = If key or file not found, return None.

New behavior:

• If hmap dest path is absolute: (unchanged) = look for the file and return it as result. = If not found, return None.
• If hmap dest path is relative: (changed) = Look for entry in headermap with the dest path as key, where the dest is a file that exists. = If key or file not found, fall back to looking for the file directly. = If file not found, return none.

`

[takuto.ikuta]

add test?

[sque]

Added test

[takuto.ikuta]

clang/test/Headers/headermap_relpath.cpp

9

generate header map using hmaptool like
https://github.com/llvm/llvm-project/blob/e32ff096858578f526b6d05ab97c8f083f2e1834/clang/test/Preprocessor/include-header-missing-in-framework-with-headermap.c#L2
?

[sque]

clang/test/Headers/headermap_relpath.cpp

9

OK. Should this test also be moved to under clang/test/Preprocessor?

[sque]

Changed to generate hmap file using hmaptool.

[sque]

Delete unnecessary comment

[takuto.ikuta]

clang/test/Headers/Inputs/headermap_relpath/empty.hmap.json

2

better to format like other hmap.json?

[sque]

Format json according to RFC 8259 (https://jsonformatter.curiousconcept.com/)

[takuto.ikuta]

Nico, can we merge this?

[vsapsai]

What use case are you trying to support? Currently the added test headermap_relpath.cpp is passing without the changes to HeaderSearch.cpp, so it's not entirely clear what problem it should address.

[bruno]

I also want to understand why you want this change. I'm not convinced this is the behavior we want and it's not like we encourage people to use header maps in the first place. I'd be fine with such behavior under a CC1 flag though. Falling back to the filesystem is something that pertains more to a VFS kinda thing than header maps.

[rnk]

@vsapsai @bruno The use case is distributed compilation. I am unfamiliar with the changes in question, but essentially, if every input to clang is rooted in /Users/foo and the build works there, we should be able to move it all to /Users/bar, change directories, and the build should still work. It seems that this doesn't work for header maps, but I haven't reviewed the code change or test and know nothing else about this feature.

[rnk]

For more context: https://crbug.com/1056174

[vsapsai]

The issue with this change is that it claims to add functionality that exists already. I.e., the test is passing without the change. The confusing part might be that even if DirectoryLookup::LookupFile doesn't find a file for relative destination immediately, it updates MappedName which is used by HeaderSearch::LookupFile. And I haven't tested it but looks like the change violates that header search paths are tested in order. So when we have -I A -I B.hmap -I C instead of processing A, B.hmap, C in that order, we can end up with A, B.hmap, A, B.hmap, C.

Unfortunately, https://crbug.com/1056174 doesn't describe a reproducible problem but only a proposed solution. If you provide more details about distributed compilation pitfalls, which paths are relative and which are absolute, we'll be able to help you with header maps.

[thakis]

I don't know why we (chromium) want to use header maps, but assuming we do want to use them: The problem with absolute paths is that they're machine-dependent and can't be run on other machines and then cached globally. See also http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html

[vsapsai]

There is nothing in header maps preventing from using relative paths. For example, I was able to run one of the tests with relative paths like

./bin/clang -cc1 -fsyntax-only ../../../llvm-project/clang/test/Preprocessor/headermap-rel.c -I ./tools/clang/test/Preprocessor/Output/headermap-rel.c.tmp.hmap -F ../../../llvm-project/clang/test/Preprocessor/Inputs/headermap-rel

and its mapping is

{
  "mappings" :
    {
     "Foo.h" : "Foo/Foo.h"
    }
}

In the test suite most of hmap mappings use relative paths. Quick search has revealed only nonportable-hmaps/foo.hmap.json to use absolute paths. So relative paths are working and tests make sure they keep working.

[• ashi009]

Any update on this?

clang/lib/Lex/HeaderSearch.cpp

Optional<FileEntryRef> DirectoryLookup::LookupFile(
   if (llvm::sys::path::is_relative(Dest)) {
     MappedName.append(Dest.begin(), Dest.end());
     Filename = StringRef(MappedName.begin(), MappedName.size());
     Optional<FileEntryRef> Result = HM->LookupFile(Filename, HS.getFileMgr());
     if (Result) {
       FixupSearchPath();
       return *Result;
     }
-  } else if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest)) {
+    // If lookup failed, fall back to file lookup using relative path directly.
+  }
+  if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest)) {
     FixupSearchPath();
     return *Res;
   }
 
   return None;
 }
 
 /// Given a framework directory, find the top-most framework directory.

clang/test/Headers/Inputs/headermap_relpath/empty.hmap.json

+{
+  "mappings":{
+    "empty/empty.h":"include/empty.h"
+  }
+}

clang/test/Headers/Inputs/include/empty.h

+

clang/test/Headers/headermap_relpath.cpp

+// Test relative include paths in headermap files
+// ========================================================================== //
+
+// RUN: rm -f %t.hmap
+// RUN: %hmaptool write %S/Inputs/headermap_relpath/empty.hmap.json %t.hmap
+// RUN: %clang_cc1 -fsyntax-only -I %t.hmap -I %S/Inputs %s
+
+#include <empty/empty.h>
+int main() { return 0; }