This is an archive of the discontinued LLVM Phabricator instance.

[Tooling] Do not restore working dir in ClangTool
ClosedPublic

Authored by ilya-biryukov on Aug 29 2018, 2:04 AM.

Details

Summary

Resolve all relative paths before running the tool instead.

This fixes the usage of ClangTool in AllTUsExecutor. The executor will
try running multiple ClangTool instances in parallel with compile
commands that usually have the same working directory.

Changing working directory is a global operation, so we end up
changing working directory in the middle of running other actions,
which leads to spurious compile errors.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Aug 29 2018, 2:04 AM
ioeric added inline comments.Aug 29 2018, 2:08 AM
lib/Tooling/Tooling.cpp
419 ↗(On Diff #163028)

Should we still check if CWD is correctly set?

ilya-biryukov added inline comments.Aug 29 2018, 3:37 AM
lib/Tooling/Tooling.cpp
419 ↗(On Diff #163028)

Not sure we know which CWD is correct at this point. Do you mean to check that getCurrentWorkingDirectory does not return an error?

ioeric added inline comments.Aug 29 2018, 3:42 AM
lib/Tooling/Tooling.cpp
419 ↗(On Diff #163028)

Yes.

Actually, after a closer look, I noticed getAbsolutePath below is just using the current working directory in real file system, which seems wrong. This is pre-existing but would be nice to fix since we are here :)

  • Use vfs for file accesses
  • Report errors when getAbsolutePath fails, skip those files
lib/Tooling/Tooling.cpp
419 ↗(On Diff #163028)

What do we do if CWD is wrong? Checking for errors in getAbsolutePath and setCurrentWorkingDirectory should be enough and that's what we already do.

Actually, after a closer look, I noticed getAbsolutePath below is just using the current working directory in real file system, which seems wrong. This is pre-existing but would be nice to fix since we are here :)

Unfortunately, getAbsolutePath is a public API (with not too many usages, but still). I've added a helper function that goes through VFS and forwarding to it in the getAbsoltuePath for now, but we might want to look at changing the API there.

ioeric accepted this revision.Aug 29 2018, 8:11 AM

lg

lib/Tooling/Tooling.cpp
202 ↗(On Diff #163071)

I think this is a better alternative to the original getAbsolutePath as a public interface. Maybe also expose it?

This revision is now accepted and ready to land.Aug 29 2018, 8:11 AM
  • Expose getAbsolutePath in the public API
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.