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

[Ella Ma via Phabricator via cfe-commits]

OikawaKirie added a comment.

Replies from the original author Hao Zhang <zhanghao19 at ios.ac.cn>

--------------------------------------------------------------

> ...split the data structures between relative and absolute paths. The existing data structures would only store absolute paths, but there are new ones for relative paths that can be cached and swapped out.

I like the idea that using one cache for absolute paths and multiple caches for relative paths. Better than my approach.

> Another point is that FileManager takes the working directory at construction time via FileSystemOptions. There's a FIXME to remove that argument, but (as you've seen) the assumption is baked in that the CWD doesn't change. I think the current patch may not completely fix it, since you can't modify FileSystemOptions.

It's the point. It is the assumption that the CWD won't change leads to the problem. I agree with you that `FileSystemOptions` should be removed and grab the CWD directly from the VFS.

> Another option here is to assume the VFS never changes directories (no need to store the CWD) ... Then if ClangTool wants to change directories, it would be expected to maintain an appropriate set of VFS layers.

IIUC, does this mean that clients should create new `Filesystem`s when they change directories? It looks to me that it might conflict with the design of `Filesystem` since it already provides an API to change the CWD.

Another option (which I mentioned in the previous post) is to use absolute paths to query the cache, something like this:

  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());
  }

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.

Anyway, your approach (remove `FileSystemOptions`, one cache for absolute paths, multiple caches for relative paths) looks good to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92160/new/

https://reviews.llvm.org/D92160