This is an archive of the discontinued LLVM Phabricator instance.

[llvm][Support] Take in CurrentDirectory as a parameter in ExpandResponseFiles
ClosedPublic

Authored by kadircet on Nov 29 2019, 6:55 AM.

Details

Summary

This is a follow-up to D70769 and D70222, which allows propagation of
current directory down to ExpandResponseFiles for handling of relative paths.

Previously clients had to mutate FS to achieve that, which is not thread-safe
and can even be thread-hostile in the case of real file system.

Diff Detail

Event Timeline

kadircet created this revision.Nov 29 2019, 6:55 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 29 2019, 6:55 AM
sammccall accepted this revision.Dec 2 2019, 7:51 AM
sammccall added inline comments.
llvm/include/llvm/Support/CommandLine.h
1972

This seems confusing.
Suppose we have @x/a, which includes @y/b

Relative=true, CurrentDir=none: we read ./x/y/b
Relative=true, CurrentDir=foo: we read foo/x/y/b
Relative=false, CurrentDir=none: we read ./y/b
Relative=false, CurrentDir=foo: we read... ./y/b?

Seems like it would be clearer to just say relative paths are relative to this directory, so the last one would be foo/y/b.

Actually looking at the code, I think that's what you implemented?

llvm/lib/Support/CommandLine.cpp
1105

This is @/BasePath I think (args are segments).

(I think this means you were running the wrong tests)

1183

this is subtle, consider a comment: CurrentDir is only relevant for "top-level" expansions || !RelativeNames, but nested ones always have absolute paths if RelativeNames so CurrentDir is ignored.

This revision is now accepted and ready to land.Dec 2 2019, 7:51 AM
kadircet updated this revision to Diff 231947.Dec 3 2019, 10:42 AM
kadircet marked 4 inline comments as done.
  • Address comments
  • Change helper to take absolute paths, instead of taking a currentdir
  • Update tests to make use of FS
  • Add a new test for container-relative filenames

Build result: pass - 60441 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

kadircet added inline comments.Dec 3 2019, 11:14 PM
llvm/lib/Support/CommandLine.cpp
1183

did better, changed ExpandResponseFile to take FName as an absolute path, PTAL

sammccall accepted this revision.Dec 4 2019, 6:08 AM
This revision was automatically updated to reflect the committed changes.