Page MenuHomePhabricator

[clang] Allow -DDEFAULT_SYSROOT to be a relative path
ClosedPublic

Authored by sbc100 on Mar 23 2020, 5:06 PM.

Details

Summary

In this case we interpret the path as relative the clang binary.

This allows SDKs to be build that include clang along with a custom
sysroot without requiring users to specify --sysroot to point to the
directory where they install the SDK.

See https://github.com/WebAssembly/wasi-sdk/issues/58

Diff Detail

Event Timeline

sbc100 created this revision.Mar 23 2020, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2020, 5:06 PM
sbc100 retitled this revision from Allow -DDEFAULT_SYSROOT to be a relative path to [clang] Allow -DDEFAULT_SYSROOT to be a relative path.Mar 23 2020, 5:06 PM
sbc100 added reviewers: sunfish, phosek.

I have no idea how to write a test for this, but I tested it locally with wasi SDK.

Hopefully fucia will find this useful too as a generic version of https://reviews.llvm.org/D42019

sbc100 edited the summary of this revision. (Show Details)Mar 23 2020, 5:51 PM
phosek added inline comments.Mar 23 2020, 6:02 PM
clang/lib/Driver/Driver.cpp
140

I don't think this is an intended use for GetResourcesPath since this is not a resource path. Since you set custom resource dir, this is equivalent to:

llvm::sys::path::append(llvm::sys::path::parent_path(ClangExecutable), SysRoot)

On line 144 we already set Dir = llvm::sys::path::parent_path(ClangExecutable), so if you move this below you can just use SysRoot = llvm::sys::path::append(Dir, SysRoot) which should be sufficient.

sbc100 marked an inline comment as done.Mar 24 2020, 10:32 AM

Is this patch up to date?

sbc100 updated this revision to Diff 252724.Mar 25 2020, 6:54 PM

feedback

Hmm.. arc diff was behaving strangely for me and failed to update this PR as I expected. Should fixed now.

phosek added inline comments.Mar 26 2020, 10:41 AM
clang/lib/Driver/Driver.cpp
143

Does this return true or false when sysroot is empty?

145

Nit: according to LLVM conventions, this should FullPath (or just P which is also very common temporary path variables).

sbc100 marked 3 inline comments as done.Mar 26 2020, 11:13 AM
sbc100 added inline comments.
clang/lib/Driver/Driver.cpp
143

Good questions. As it happens it looks like it returns true, which is not the result we want here. Added explicit check.

phosek accepted this revision.Mar 26 2020, 11:44 AM

LGTM

This revision is now accepted and ready to land.Mar 26 2020, 11:44 AM
This revision was automatically updated to reflect the committed changes.
sbc100 marked an inline comment as done.