This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Search computed sysroot for libc++ header paths
ClosedPublic

Authored by rprichard on Jun 10 2020, 5:44 PM.

Details

Summary

The Android NDK's clang driver is used with an Android -target setting,
and the driver automatically finds the Android sysroot at a path
relative to the driver. The sysroot has the libc++ headers in it.

Remove Hurd::computeSysRoot as it is equivalent to the new
ToolChain::computeSysRoot method.

Fixes PR46213.

Diff Detail

Event Timeline

rprichard created this revision.Jun 10 2020, 5:44 PM
rprichard updated this revision to Diff 270005.Jun 10 2020, 5:46 PM

Rerun after installing clang-format

rprichard updated this revision to Diff 270009.Jun 10 2020, 5:48 PM

Reverse clang-format's change.

rprichard added reviewers: srhines, danalbert, Restricted Project, kristina.Jun 10 2020, 6:01 PM

FWIW, computeSysRoot is overridden by these toolchains:

  • Linux
  • MSP430ToolChain
  • MipsLLVMToolChain
  • RISCVToolChain

I think this change restores the pre D69758 behavior for Linux and MipsLLVMToolChain. It might be nice to have a review from someone familiar with the MSP430 and RISC-V toolchains.

Harbormaster completed remote builds in B59903: Diff 270005.

Thanks, Ryan, for diagnosing and addressing this bug.

clang/lib/Driver/ToolChains/Linux.h
45

Can drop virtual here.

rprichard updated this revision to Diff 270030.Jun 10 2020, 9:37 PM

Remove unnecessary virtual keyword.

srhines accepted this revision.Jun 15 2020, 12:44 PM

Thanks, Ryan!

This revision is now accepted and ready to land.Jun 15 2020, 12:44 PM

While this doesn't look wrong to me -- and the correctness of this depends entirely on where vendors decide to put their headers so it's hard for me to verify -- I'm wondering why not all toolchains use this mechanism. We seem to be adding an abstraction that's used only by some toolchains, but not all. I think it would be great to have a single canonical way of representing the system root.

Commenting out of curiosity, don't let this block you.

danalbert accepted this revision.Jun 17 2020, 3:40 PM

While this doesn't look wrong to me -- and the correctness of this depends entirely on where vendors decide to put their headers so it's hard for me to verify -- I'm wondering why not all toolchains use this mechanism. We seem to be adding an abstraction that's used only by some toolchains, but not all. I think it would be great to have a single canonical way of representing the system root.

Yeah, I agree that the inconsistencies between vendors in the driver are pretty odd. I think essentially the way we got into this situation is that it's not always clear what the right way to do things is, and if it's not even clear that there even is an existing mechanism we can easily end up creating our own per vendor :(

I'd be interested in seeing if there are things we can do to try to unify the behavior in this area. I suspect it could be difficult given that it necessarily involves all of the different vendors, but maybe we could come up with one recommended way of doing all this and any of us that don't fit that model can make the change at a pace convenient for us?

Commenting out of curiosity, don't let this block you.

Thanks for making that clear :) We're going to go ahead and submit since this does fix a regression but I'm open to making changes here if (hopefully when) we find some cleanup that can be done.

This revision was automatically updated to reflect the committed changes.

Hi, you can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (I have updated my script to use --date=now (setting author date to committer date))

https://reviews.llvm.org/D80978 contains a git pre-push hook to automate this.

Ok, I'll run that command in the future, and I'll watch D80978 for updates.