dexonsmith added a comment.
In D92160#2447861 <https://reviews.llvm.org/D92160#2447861>, @OikawaKirie wrote:
> Replies from the original author Hao Zhang <zhangha...@ios.ac.cn>
>
> llvm::ErrorOr<const FileEntry *>
> FileManager::getFile(StringRef Filename, bool openFile, bool CacheFailure) {
> // Froce using the absolute path to query the cache
> llvm::SmallString<128> AbsPath(Filename);
> makeAbsolutePath(AbsPath);
> auto Result = getFileRef(AbsPath.str(), openFile, CacheFailure);
> if (Result)
> return &Result->getFileEntry();
> return llvm::errorToErrorCode(Result.takeError());
> }
(A quibble here is that `getFile` is just a wrapper around `getFileRef`, and
might eventually be removed; certainly `FileEntry::getName` is on its way out;
you'd want this logic at the top of `getFileRef`, `getVirtualFileRef`, etc.)
> It is more likely a defensive workaround to this problem. It might not be a
> good solution to the real problem (the assumption that the CWD won't change).
> The above code resulted in failures of some test cases, I haven't looked
> closely into why they failed. In my point of view, `Filename` should only be
> used as the key to query the cache. Logically, if I change it into the
> absolute path, those test cases should pass as usual.
I think it would be good to understand why the tests failed.
Are some clients relying on the name matching the query? If so, why?
My guess, but I'm really not sure and it would be good to confirm, is that we
need this to support reproducible builds, where you don't want the CWD to leak
into your output. In this case, you'd run all build commands from the same
directory, but invoke clang using all relative paths, perhaps something like
this for your example:
% clang -g -o a/a.o -Isrc/a src/a/a.c
% clang -g -o b/b.o -Isrc/b src/b/b.c
Perhaps (I'm not sure) there are places in clang that rely on `FileManager`
returning relative paths to support this kind of thing. If anything relied on
it, my bet would be modules-related code.
But it's possible we don't need this. If it's safe for us to update the tests
and make `FileManager::getFileRef` always canonicalize to an absolute path,
that would definitely be cleaner. `FileManager::makeAbsolute` can use whatever
the FS's CWD is at the time of query... nice and clean.
> Anyway, your approach (remove `FileSystemOptions`, one cache for absolute
> paths, multiple caches for relative paths) looks good to me.
(But as you mentioned, not as clean as canonicalizing to absolute paths, if we
can do it.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92160/new/
https://reviews.llvm.org/D92160
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
