Page MenuHomePhabricator

Make clang-based tools find libc++ on MacOS
ClosedPublic

Authored by ilya-biryukov on Nov 9 2018, 6:49 AM.

Details

Summary

When they read compiler args from compile_commands.json.
This change allows to run clang-based tools, like clang-tidy or clangd,
built from head using the compile_commands.json file produced for XCode
toolchains.

On MacOS clang can find the C++ standard library relative to the
compiler installation dir.

The logic to do this was based on resource dir as an approximation of
where the compiler is installed. This broke the tools that read
'compile_commands.json' and don't ship with the compiler, as they
typically change resource dir.

To workaround this, we now use compiler install dir detected by the driver
to better mimic the behavior of the original compiler when replaying the
compilations using other tools.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Nov 9 2018, 6:49 AM
sammccall accepted this revision.Nov 9 2018, 7:55 AM

This is hacky, but solves an important problem. If you know of a good reviewer for this, you may want a second opinion!

include/clang/Lex/HeaderSearchOptions.h
111 ↗(On Diff #173318)

Could you add "typically contains bin/clang, lib/libclang_rt, and include/<builtins>"?

lib/Frontend/CompilerInvocation.cpp
1776 ↗(On Diff #173318)

(is this needed? I guess you fall back to this if you don't create the CI from the command line?)

This revision is now accepted and ready to land.Nov 9 2018, 7:55 AM
ilya-biryukov marked 2 inline comments as done.
  • Update the comment.

If you know of a good reviewer for this, you may want a second opinion!

This would definitely be nice, but I also don't know who'd be the proper person to review this.
I'll take a pause until Monday, maybe @arphaman or someone else from the community can suggest a better way of fixing this or proper reviewers for this change.

include/clang/Lex/HeaderSearchOptions.h
111 ↗(On Diff #173318)

This is actually the 'bin' subdirectory, I've added a comment about that but you see a better way to communicate this, please let me know! E.g. actually mentioning the parallel lib/ and include/ subdirs.

lib/Frontend/CompilerInvocation.cpp
1776 ↗(On Diff #173318)

Thanks for noticing this!
This is a leftover from a previous iteration, it's actually incorrect now since we rely on InstallDir pointing to a different place from the resource dir.
Removed it.

  • Added a test with a compiler path relative to the working dir
Closed by commit rL346652: Make clang-based tools find libc++ on MacOS (authored by ibiryukov, committed by ). · Explain WhyNov 12 2018, 5:58 AM
This revision was automatically updated to reflect the committed changes.

I reverted this because it broke the LLDB bots on GreenDragon: http://green.lab.llvm.org/green/view/LLDB/

Will take a look, thanks for the revert.

I don't quite understand the need for this patch.
If we are talking about a binary built from source, wouldn't it make more sense to build libcxx from source as well?
If libcxx is built from source in the same repo, clang-based tools do find it.

In the future, for macOS-specific changes I think it would be better to wait for a sign-off from at least one maintainer who is an expert on Apple tools.

Apologies for not seeing this earlier.
I agree with George, this behavior doesn't seem right to me.

The logic to do this was based on resource dir as an approximation of
where the compiler is installed. This broke the tools that read
'compile_commands.json' and don't ship with the compiler, as they
typically change resource dir.

In that case I think the tool should adjust the -resource-dir to point to the appropriate -resource-dir for the compiler in the toolchain. However, that comes with its own problems, as the builtin headers might be incompatible between the tool and the compiler in the toolchain. In that case I think it would be preferable for the tool to ship with its own libc++ headers in addition to the builtin headers rather than implementing this behavior. If that's unacceptable I think we should adjust libTooling to add a search path for the libc++ headers when the tool detects that it's running outside of the toolchain on Darwin (i.e. 'clang' doesn't exist in the same directory as the tool, and '../include/c++' doesn't exist as well) instead of changing this behavior.

Apologies for not seeing this earlier.

No worries, thanks for the input!

The logic to do this was based on resource dir as an approximation of
where the compiler is installed. This broke the tools that read
'compile_commands.json' and don't ship with the compiler, as they
typically change resource dir.

In that case I think the tool should adjust the -resource-dir to point to the appropriate -resource-dir for the compiler in the toolchain. However, that comes with its own problems, as the builtin headers might be incompatible between the tool and the compiler in the toolchain.

Exactly the problem we're facing. Since the -resource-dir is primarily used to find the builtin headers, we have to override it in the tools to avoid breaking them.

In that case I think it would be preferable for the tool to ship with its own libc++ headers in addition to the builtin headers rather than implementing this behavior.

There's value in providing the users with warnings/errors from exactly the same version of the C++ standard library that's used by the compiler, so I would avoid going down this path if possible.

If that's unacceptable I think we should adjust libTooling to add a search path for the libc++ headers when the tool detects that it's running outside of the toolchain on Darwin (i.e. 'clang' doesn't exist in the same directory as the tool, and '../include/c++' doesn't exist as well) instead of changing this behavior.

This was the original intention of the patch, but I don't think we should limit this to libTooling.
I'll have a look at what exactly lldb broke and will come up with a fix to this behavior.