[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>
(Sorry for the wrong email address in the previous reply.)

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

Thanks for your reply.

I noticed this problem when I was using clang-check to analyze a project which had a `compile_commands.json`. A simplified test case is provided in the patch.

As far as I know this problem only happens in `clang/lib/Tooling/Tooling.cpp`. (as @OikawaKirie has explained)

However, it is more likely a bug in `FileManager`, since `FileManager` is not aware that whether the current working directory has been changed or not.

One solution could be to add an API, such as `notifyCurrentWorkingDirectoryChange`. Clients call if necessary. (Similar to the 3rd one in your alternative solution). Now the question is what to do in `notifyCurrentWorkingDirectoryChange`:

- In my solution, I use a pretty straightforward approach, which is to have an individual cache (for `FileEntry` and any other related things) for each working directory, and switch caches between working directories.



- In your alternative solution, if I standerstand it correctly, `dropRelativePaths` is called in `notifyCurrentWorkingDirectoryChange`. All those `FileEntry` which are based on relative paths are dropped from the cache. I think this is a good idea, especially for keeping `FileManager` simple. However I'm not sure how many files are based on relative paths. If `dropRelativePaths` drops too many files and the working directory is switched back to a previous one (this could happen when analyzing with a `compile_commands.json`), it might result in many cache misses, thus involving more system calls to open files.

There was an another solution I have ever tried, without things like `notifyCurrentWorkingDirectoryChange`. The solution is still straightforward, which is to use absolute path (instead of filename) to query/store a `FileEntry` from/to the cache. Actually I have tried this before but it ended up with lots of test cases failed. I don't know where I did wrong. If you think this approach is okay, I will continue working on this, and it might take some time.

I'm still not sure which solution is more suitable, or if there is a better one. Any suggestions are welcome!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92160