[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.

In D92160#2443405 <https://reviews.llvm.org/D92160#2443405>, @dexonsmith wrote:

> 



In D92160#2443413 <https://reviews.llvm.org/D92160#2443413>, @dexonsmith wrote:

> 

Replies on my own:

> Can you tell me more about the scenario this happens?

You can first manually try the regression test case in this patch.
As the `%t/b/config.h` is empty, there should not be any reading operations to the file. However, as the length of `%t/a/config.h` is wrongly used for `%t/b/config.h` because of the bug, the file does be read, and some warnings about reading `'\0'` characters are generated during compilation.

> Why is the FileManager being reused after the working directory change?

It seems to be the problem of ClangTool. All the input source files are handled one by one with the same FileManager instance from the ClangTool::Files. (See the comments in the file `clang/lib/Tooling/Tooling.cpp`)

> I'm also wondering about an alternate solution that optimizes for the common case, such as the following:
>
> Don't specifically track relative paths / absolute paths.
> Add an API, something like: dropRelativePaths(), which drops all {File,Directory}Entry{,Ref} that are somehow based on relative paths. This can forward to a similar function the stat cache if necessary.
> Update clients to call that if the working directory changes (you'd want to do it only if it actually changed, not always).
> This would keep FileManager from having to growing complexity to track working directory changes.
>
> I realize this may not be clear; what I'm suggesting is that dropRelativePaths() would visit all entries, check if they're relative, and drop them if so.

I am not quite clear with the fixes in this patch, I will forward the replies to these questions from the original author Hao Zhang <zhanghao at ios.ac.cn> to you when he replies to me.

Thanks



================
Comment at: clang/lib/Tooling/Tooling.cpp:545
       LLVM_DEBUG({ llvm::dbgs() << "Processing: " << File << ".\n"; });
       ToolInvocation Invocation(std::move(CommandLine), Action, Files.get(),
                                 PCHContainerOps);
----------------
The CWD changes for each input file, but the same FileManager instance is used.

And relative paths are used to index the file entries.

When there are two files in different CWDs having the same relative path, they will be mixed up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92160