Update LLVM #4530
Update LLVM #4530
Conversation
| @@ -58,7 +58,8 @@ TEST(ClangRunnerTest, Version) { | |||
| const auto install_paths = | |||
| InstallPaths::MakeForBazelRunfiles(Testing::GetExePath()); | |||
| std::string target = llvm::sys::getDefaultTargetTriple(); | |||
| ClangRunner runner(&install_paths, target, &test_os); | |||
| auto VFS = llvm::vfs::getRealFileSystem(); | |||
There was a problem hiding this comment.
Shouldn't this be "vfs", not "VFS", since it's a local variable?
There was a problem hiding this comment.
Sorry, I copied that from some llvm code. Fixed now.
toolchain/driver/clang_runner.cpp
Outdated
| : installation_(install_paths), | ||
| target_(target), | ||
| fs_(fs), |
There was a problem hiding this comment.
We don't use the driver's VFS in other parts, and I'm concerned that an incomplete use will yield inconsistent results. Had you considered using the real filesystem here?
There was a problem hiding this comment.
The change causing this is allowing users to disable warnings on a per-file basis. Carbon would want to honour the same when building the same C++ code with the same paths.
There was a problem hiding this comment.
I think you're arguing a different issue. At present, the clang::Driver invocation doesn't get a VFS passed in, so it's probably using the default (real) VFS. While we might want to pass in the VFS we get from Carbon's driver to clang::Driver (I'm not sure if Chandler had it use the real filesystem deliberately), this change doesn't change what clang::Driver uses. Instead, it only passes it to ProcessWarningOptions.
The end result is that ProcessWarningOptions uses Carbon's VFS, while clang::Driver uses the real filesystem. This is the undesirable inconsistency that I'm concerned with.
There was a problem hiding this comment.
@chandlerc Should we use the real file system for everything or the vfs from the driver_env for everything?
There was a problem hiding this comment.
To be sure, I really think for the update PR you might be best off just maintaining current filesystem behavior, rather than changing it. However, if I'm in the minority here, note that clang::driver::Driver takes a IntrusiveRefCntPtr, so you'll want to change the types you're using for storage on ClangRunner and probably also DriverEnv and the FileTest infrastructure. (which might be the actual reason we haven't done this yet)
There was a problem hiding this comment.
Agreed that for an LLVM update PR, the smaller change is more appropriate, and any other changes can be done as a follow-up if desired. Updated.
There was a problem hiding this comment.
(agreed about not shuffling anything in the update PR)
For any follow-up change: I think we'll want to pass down the driver_env VFS -- basically, when we reach Clang functionality from the driver, we want Clang to see a consistent world as the non-Clang parts of the driver from a VFS perspective.
Includes updates to reflect these LLVM changes: