Feature request: more powerful way to set C/C++ compile/link flags

rongjiecomputer commented 6 years ago

Problem

Currently, Bazel does not have a way to set C/C++ compile/link flags in BUILD/.bzl files to all targets other than manually adding copts/linkotps manually to all cc_* rules.

Not being able to set copts/linkopts to all targets has caused a lot of problems in Tensorflow (e.g. compile error due to some targets not getting -DNOGDI flag in MSVC build). Other Google projects such as Protobuf and gRPC are also affected, but less severe than Tensorflow.

As far as I know, Abseil C++ is the only Google project that has the discipline to set copts to every cc_* with ABSL_DEFAULT_COPTS/ABSL_TEST_COPTS from copts.bzl manually, and even then accident can happen (uses “-fexceptions` directly instead of ABSL_EXCEPTIONS_FLAG, which break MSVC build).

Existing workarounds

Tensorflow currently uses --copt in .bazelrc to set global compile flags.
- Control of flags are passed over from BUILD and .bzl to configure.py. Need to implement compiler/platform detection yourself.
- All Tensorflow’s dependencies are affected.
Adding flags to CROSSTOOL.
- Now all Bazel projects are affected. I violently oppose this workaround.
- Need to maintain your own CROSSTOOL which is painful.
Use macro to wrap around cc_library
- Example: llvm_cc_library, tf_cc_library.
- For some reason, Tensorflow team dislikes this and proposes the .bazelrc and CROSSTOOL workarounds as above, which in my opinion are even worse.
- This is currently blocking my PR to make LLVM build with Bazel on Windows (because Tensorflow team don't want llvm_cc_library macro).

What we need

A way to set compile/link flags to targets with custom visibility (current package only, current package with subpackages or whole @project//*).

Internal version of Blaze seems to have default_copts parameter in package rule.

What CMake does

In CMake, you can use add_compile_options and add_definitions to add compile/define flags to all targets under this current directory and sub-directories. (You can also directly add/replace/delete flags to CMAKE_C_FLAGS and CMAKE_CXX_FLAGS, but that will affect all directories which is very intrusive).

hlopko commented 6 years ago

Hi @rongjiecomputer, somehow this issue fell of my radar, and I'm on training till Thursday. Let me come back to you on Friday.

rongjiecomputer commented 6 years ago

@mhlopko ping?

hlopko commented 6 years ago

Hi @rongjiecomputer

Let me get one my assumptions out of the way. I think the crosstool is a good abstraction, although not super user-friendly (yet :) In my ideal world, users don't have to think about the compiler flags (or even the compiler used, or even the platform used, to some extent). Specifying the flags using --copt or --linkopt should be fine for tweaking stuff during development, but in the general case it's the toolchain author who thinks about flags, and who has to have a way to specify which flags are applied when.

I fully agree that the legacy crosstool fields (compiler_flag, linker_flag ...) are not sufficient for that. That's why we introduced action_configs and features as a way to dynamically control flags.

Example: toolchain author creates a feature named fully_static_link, that will encapsulate all the flags needed to create a C++ binary that statically links libc. Users don't have to know the details, they only need to enable this feature for their cc_binary:

    cc_binary(
      name = "foo",
      features = [ "fully_static_link" ]
      ...)

You can also enable a feature for the entire build, disable features conditionally and some other things. I don't want to dive into that here in this issue, we are in the process of writing docs for all of this with @spomorski.

Currently one thing that cannot be done is to enable a certain feature for all transitive dependencies of a particular target. We will work with @gregestren on this eventually, but we are not at the moment.

I agree that changing the crosstool is hard, and we're trying to make it simpler. Ideally there are way less crosstools than there are projects, and projects don't have to duplicate all the flags over and over, and instead they can reuse features.

Issue tracking the migration in bazel will be https://github.com/bazelbuild/bazel/issues/5187. Currently this effort is blocked by https://github.com/bazelbuild/bazel/issues/4571 which is seeing good progress.

I assume this will raise many questions, please don't hesitate to ask/challenge me :)

rongjiecomputer commented 6 years ago

Currently one thing that cannot be done is to enable a certain feature for all transitive dependencies of a particular target.

This seems to solve part of my problem. Can it work backward (i.e. a target can force everyone that depends on it to turn on/off certain feature, such no_asan if a target is known to not work with address sanitizer)?

but in the general case it's the toolchain author who thinks about flags, and who has to have a way to specify which flags are applied when.

For me, Crosstool/Toolchain author's responsibility is only to specify the least flags needed to use a toolchain ("least" is the keyword here). Project-specific flags should not appear in Crosstool.

Effect of my assumption:

Crosstool should be reusable for most projects no matter what special requirement a project has.
If two Crosstools support the same Clang, using either of them should not change the output.
BUILD authors rarely have to communicate with Crosstool authors.

Crosstool is useful, but as far as I know, only one Crosstool file can be used in one build. If I have one project with 5 dependencies, all with one custom Crosstool just to set compiler/linker flags for its project, then which one should user chooses to use for the entire build?

In my ideal world, users don't have to think about the compiler flags (or even the compiler used, or even the platform used, to some extent).

I don't think this is possible in the world of C and C++. GCC, Clang and MSVC all have thousands of warning and optimization flags, no one can abstract that away. You can abstract away more general things such as no_exceptions, use_rtti, fully_static_link (and I wish Bazel will support more of such things), but beyond that, you can't abstract away every single warning and optimization flag (and it is not going to be useful to anyone).

Different projects have different set of warning and optimization flags they want to disable (e.g. some projects might want to disable -Wnarrowing warning, some projects might not; some projects need -fdelete-null-pointer-checks optimization, some projects cannot use it).
Most Windows projects will define /DMINMAX to not define min and max macros, but there will be some Windows projects out there that do not want this flag.

My point is Crosstool should not be the one to force certain flags down these projects' throat. Unless in Bazel's ideal world, each project should have its own Crosstool. Note that in the above examples, those flags need to be set for all targets of a project or you might get some obscure compile/runtime errors.

I can sort of guess how Google uses Crosstool internally by looking at how Chromium uses GN. Chromium's //src/build/config (which is kinda like Bazel's Crosstool) is very gigantic. All Chromium's dependencies will use this //src/build/config, which is feasible for Chromium because they have a team to deal with everything about "build" and owners for each third party dependency has the responsibility to maintain its custom BUILD.gn. In outside world however, people prefer the maintainer(s) of the dependencies to write build file so that they don't have to deal with it.

//src/build/config is so tuned to Chromium's usage (like turn on/off certain optimizations and adding defines such as SAFE_BROWSING_DB_REMOTE that only make sense to Chromium) that it is very difficult to modify it to use with GN outside Chromium.

Some other comments about Crosstool.

I fully agree that the legacy crosstool fields (compiler_flag, linker_flag ...) are not sufficient for that. That's why we introduced action_configs and features as a way to dynamically control flags.

I agree on this too. I have written a working CROSSTOOL for LLVM on Windows toolchain based on action_config. It can express more things than compiler_flag, linker_flag etc. alone.

Example: toolchain author creates a feature named fully_static_link, that will encapsulate all the flags needed to create a C++ binary that statically links libc. Users don't have to know the details, they only need to enable this feature for their cc_binary:

That is also my dream. I wish I can just features = ["cpp14", "no_exceptions", "no_rtti"] in one cc_library, package, whole project (this one is currently impossible which I need heavily) or entire build (already possible with --features, can be use for thinlto, asan, ubsan ...).

Issue tracking the migration in bazel will be #5187. Currently this effort is blocked by #4571 which is seeing good progress.

I have been tracking some Github issues about C++ Crosstool too. I have read Crosstool in Skylark since it was published for review, but I don't fully understand what I read (not much example and have too much reference to Bazel's Java implementation). I think it is already partially implemented? I can see action_config in Skylark macro form. I am very impressed about how the implementation moves closer to the goal of code-reusability and human-readability with Skylark macros while under-the-hood still produces proto text format for smoother migration.

I am willing to help with the Windows part when it is ready for external contributors to contribute.

Some more complaints about Crosstool

When can Bazel finally decouple operating system from --cpu? k8 (for Linux) and darwin (for Mac) are not even CPUs, and don't let me start with x64_windows and x64_windows_msvc (cpu + os + compiler)! (Perhaps this should be in another Github issue?)

Bazel should be able to figure out cpu and os easily. Compiler can be selected based on --compiler or just from environment variables (#5186, I will add some comments in that issue). User should only need to specify cpu + os + compiler when cross-compiling with LLVM-style --target flag. (And that is for target_platform. Bazel should still figure out cpu + os for host_platform automatically).

Thanks for reading till the end.

hlopko commented 6 years ago

Re: Backwards work: we don't propagate features currently, but that would be a simple change. I would want to have multiple feature types then, because not all features are meant to be used transitively, and not all are meant to be used on targets directly.
Re: "least amount of flags": I see your point, and I actually agree. Writing a good crosstool (that has modules, thinlto, can verify headers, layering checks, uses linker effectively, has sanitizers) is hard. Individual projects should not be forced to be in the business of writing crosstools. I'd argue that features allow you to be decoupled from the crosstool more than copts/linkopts.
Re: Warnings and fine tuning: If the project has good reasons to fine tune, great. But good crosstool that sets the standard for the modern C++ development (so most people don't have to do "anything") will have to be opinionated. I can also imagine a robust-enough way of modifying the crosstool for every project in BUILD files without the need to change the original (think adding a custom, project only features).
Re: Google use of the Crosstool, you're mostly accurate.
Re: ["cpp14", "no_exceptions", "no_rtti"] - that is exactly what I think the good crosstool should contain.

There are 2 parallel efforts. One is changing crosstool format from text protobuf to Skylark code. That what Crosstool in Skylark is about. Second it making it easier to write cc_configure which is an autotools-like script that inspects the environment and autogenerates the crosstool. action_config macros you saw are for the latter.

We are migrating away from --cpu and --compiler options towards platforms. It's not only about figuring it out (bazel already does that), but sometimes you need to cross compile, sometimes you need to cross compile on a different machine, etc. But yes, we're working on it :)

Thanks for taking the time to comment!

hnsl commented 6 years ago

What is left on cc_configure to allow it to used for this? I'd love to disable exceptions, disable rtti and compile with C++17, but maintaining a custom crosstool seems way too painful.

hlopko commented 5 years ago

Feel free to comment on https://github.com/bazelbuild/bazel/issues/6926, which is tracking the effort to make cc_configure better. I'll close this issue since I don't know what the action item here is. Please complain if I'm wrong.

Riatre commented 5 years ago

I have read through all the comments in this issue, but it seems like none of these solve the problem raised in the original feature request.

What if the project:

Do require tweaking warning flags,
or insist on having some "configuration" macros like PROJECT_NAME_ENABLE_LOGGING being defined (no, this cannot be WORKSPACE wide), and the maintainers wants to provide a sane default for different compilation modes via select(),
or even needs some bizarre flags being set such as -include a.h.
And the project maintainers are fully aware of the implication of putting compiler flags into BUILDs, decided to give up supporting too foreign CROSSTOOLs and take a stab at picking the correct set of flags for different toolchains via select()?

What is the suggested way to do this, except repeating the same copts in every cc_library like Abseil?

And, what's wrong with having a default_copts on package()? I think this is a valid feature request.

hlopko commented 5 years ago

The problem of having default_copts in package is that Bazel is a multi-language build system, and having language specific attributes in the basic things such as package just doesn't scale. We work hard on removing all the traces of C++ from Bazel, and instead express everything in Starlark.

Can you elaborate why having a package-local or project-local macro doesn't work?

Riatre commented 5 years ago

Thanks for your reply, that sounds very reasonable.

Project-local macros work okay for this, it's just a little bit hard to setup and enforce, I agree it's not a problem of Bazel itself and is rather minor.

pvmilk commented 2 years ago

Hope I am not too late for the show.

I would like to have a pre-defined feature to disable the following c++ -Wl,-no-as-needed linkage flag, without going all the way to define my cc_toolchains. I think the flag is added here.

bazelbuild / bazel