[PATCH] D92160: [clang] Fix wrong FDs are used for files with same name in Tooling

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D92160#2447861 <https://reviews.llvm.org/D92160#2447861>, @OikawaKirie wrote:

> Replies from the original author Hao Zhang <zhanghao19 at 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