Page MenuHomePhabricator

Support relative dest paths in headermap files
Needs RevisionPublic

Authored by sque on Feb 27 2020, 5:44 PM.

Details

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.

`

Diff Detail

Event Timeline

sque created this revision.Feb 27 2020, 5:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2020, 5:44 PM
takuto.ikuta requested changes to this revision.Feb 27 2020, 6:09 PM
takuto.ikuta added a subscriber: takuto.ikuta.

add test?

This revision now requires changes to proceed.Feb 27 2020, 6:09 PM
sque updated this revision to Diff 247177.Feb 28 2020, 12:51 AM
sque edited the summary of this revision. (Show Details)

Added test

sque edited the summary of this revision. (Show Details)Feb 28 2020, 12:53 AM
sque marked an inline comment as done.Mar 1 2020, 6:40 PM
sque added inline comments.
clang/test/Headers/headermap_relpath.cpp
9

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

sque updated this revision to Diff 247540.Mar 1 2020, 7:10 PM
sque marked an inline comment as done.

Changed to generate hmap file using hmaptool.

sque updated this revision to Diff 247554.Mar 1 2020, 10:29 PM

Delete unnecessary comment

takuto.ikuta added inline comments.Mar 1 2020, 11:01 PM
clang/test/Headers/Inputs/headermap_relpath/empty.hmap.json
2

better to format like other hmap.json?

sque updated this revision to Diff 247558.Mar 1 2020, 11:11 PM

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

sque marked an inline comment as done.Mar 1 2020, 11:12 PM

Nico, can we merge this?

takuto.ikuta accepted this revision.Mar 6 2020, 1:04 AM

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 requested changes to this revision.Mar 9 2020, 7:15 PM

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.

This revision now requires changes to proceed.Mar 9 2020, 7:15 PM
rnk added a subscriber: rnk.Mon, May 18, 6:30 PM

@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.

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.

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

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.