Page MenuHomePhabricator

Driver: fallback to parent directory of clang if no sysroot for mingw-w64 target
ClosedPublic

Authored by martell on Oct 28 2015, 4:21 PM.

Details

Summary

Driver: fallback to the location of clang if no sysroot,
hard coding /usr makes little sense for mingw-w64.

If we have portable toolchains having /usr breaks that.
If the clang we use is in /usr/bin or /usr/sbin etc this will still detect as though it was hard coded to /usr

This makes the most sense going forward for mingw-w64 toolchains on both linux and mac

Diff Detail

Repository
rL LLVM

Event Timeline

martell updated this revision to Diff 38695.Oct 28 2015, 4:21 PM
martell retitled this revision from to Driver: fallback to parent directory of clang if no sysroot for mingw-w64 target.
martell updated this object.
martell added a reviewer: rnk.
martell added a subscriber: yaron.keren.

The code could now be merged (untested). I disconnected the third part on purpose based on comment from Joerg Sonnenberger on the previous code that having else after #endif is confusing.

if (getDriver().SysRoot.size())
  Base = getDriver().SysRoot;
#ifdef LLVM_ON_WIN32
else if (llvm::ErrorOr<std::string> GPPName =
             llvm::sys::findProgramByName("gcc"))
  Base = llvm::sys::path::parent_path(
      llvm::sys::path::parent_path(GPPName.get()));
#endif

if (!Base.size())
  Base = llvm::sys::path::parent_path(getDriver().getInstalledDir());

Yes I was thinking about that.
I just wanted to get the initial patch in for review incase you had a specific reason to keep /usr

If everything is good with replacing /usr I will update the patch merging to 2 code paths ?

Yes, I think this is a better default than /usr.

martell updated this revision to Diff 38743.Oct 29 2015, 9:00 AM
martell added a subscriber: llvm-commits.

My code is a little different from yours as I wanted to preserve as much of the original code as possible.

Added llvm-commits as a subscriber.

This code was my first thought too, but I had been commented on the structure of very similar code in this file:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150713/133105.html

martell updated this revision to Diff 39069.Nov 3 2015, 8:22 AM

Thanks for the tip.
That does make sense :)
Updated accordingly with the comment also

We did not have cfe-commits as subscriber so I'm adding it now (this is a clang commit), see if someone would like to further comment.

yaron.keren accepted this revision.EditedNov 4 2015, 11:15 AM
yaron.keren added a reviewer: yaron.keren.

The formatting is wrong, aligned to the left, clang-format the new code.

Can we control getInstalledDir() when running under LIT so this could be tested? if possible, add a test before committing.

LGTM.

This revision is now accepted and ready to land.Nov 4 2015, 11:15 AM
This revision was automatically updated to reflect the committed changes.

I tried adding a test case but I couldn't see how to control getInstalledDir from LIT.
It is a bit redundant because this was already true for windows and there was no test case for that.
After trying to create one I saw why :)

I missed to formatting so I updated it in rL253899