bazel icon indicating copy to clipboard operation
bazel copied to clipboard

`layering_check` doesn't work without sandboxing

Open fmeum opened this issue 11 months ago • 9 comments

Description of the bug:

When compiling C++ targets with the layering_check feature, Bazel only stages the clang module maps of direct dependencies of a target as inputs, not all transitive module maps: https://github.com/bazelbuild/bazel/blob/f5c8deb4fb36f49bb6c473b8655dfef8435b0c99/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationContext.java#L578-L582

As clang doesn't seem to fail if it can't find a module map file referenced by another module map, this works fine as long as sandboxing is enabled. However, if the compilation action doesn't run sandboxed, clang may load the referenced transitive module maps files, resulting in a "missing dependency declaration" error.

Which category does this issue belong to?

C++ Rules

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

# .bazelrc
build --features=layering_check
build --repo_env=CC=clang

# MODULE.bazel
bazel_dep(name = "abseil-cpp", version = "20240116.1")
$ bazel clean --expunge && bazel --nohome_rc --nosystem_rc build @abseil-cpp//absl/base:throw_delegate --spawn_strategy=sandboxed
INFO: Invocation ID: aa6bf76b-d655-4601-904a-654a621f86ca
INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.
Starting local Bazel server and connecting to it...
INFO: Analyzed target @@abseil-cpp~20240116.1//absl/base:throw_delegate (70 packages loaded, 345 targets configured).
INFO: Found 1 target...
Target @@abseil-cpp~20240116.1//absl/base:throw_delegate up-to-date:
  bazel-bin/external/abseil-cpp~20240116.1/absl/base/libthrow_delegate.a
  bazel-bin/external/abseil-cpp~20240116.1/absl/base/libthrow_delegate.so
INFO: Elapsed time: 8.820s, Critical Path: 0.70s
INFO: 15 processes: 10 internal, 5 linux-sandbox.
INFO: Build completed successfully, 15 total actions

$ bazel clean --expunge && bazel --nohome_rc --nosystem_rc build @abseil-cpp//absl/base:throw_delegate --spawn_strategy=standalone
INFO: Invocation ID: 550feb94-c0c0-4f84-a50d-b17a2d73dbe3
INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.
Starting local Bazel server and connecting to it...
INFO: Analyzed target @@abseil-cpp~20240116.1//absl/base:throw_delegate (70 packages loaded, 345 targets configured).
ERROR: /home/fhenneke/.cache/bazel/_bazel_fhenneke/1265e716d099d259b9c08d61b24f01b4/external/abseil-cpp~20240116.1/absl/base/BUILD.bazel:339:11: Compiling absl/base/internal/throw_delegate.cc failed: undeclared inclusion(s) in rule '@@abseil-cpp~20240116.1//absl/base:throw_delegate':
this rule is missing dependency declarations for the following files included by 'absl/base/internal/throw_delegate.cc':
  'bazel-out/k8-fastbuild/bin/external/abseil-cpp~20240116.1/absl/base/core_headers.cppmap'
  'bazel-out/k8-fastbuild/bin/external/abseil-cpp~20240116.1/absl/base/atomic_hook.cppmap'
  'bazel-out/k8-fastbuild/bin/external/abseil-cpp~20240116.1/absl/base/errno_saver.cppmap'
  'bazel-out/k8-fastbuild/bin/external/abseil-cpp~20240116.1/absl/base/log_severity.cppmap'
Target @@abseil-cpp~20240116.1//absl/base:throw_delegate failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 8.400s, Critical Path: 0.59s
INFO: 13 processes: 11 internal, 2 local.
ERROR: Build did NOT complete successfully

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

7.0.2

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

This was originally discovered in https://github.com/bazelbuild/rules_fuzzing/pull/242#issuecomment-1974913042.

fmeum avatar Mar 06 '24 09:03 fmeum

I briefly discussed this with @oquenchil, who mentioned that this was likely a design decision for Blaze. This leads to a few questions:

  1. How does include scanning interact with the need to stage (or not to stage) transitive module maps?
  2. Is it guaranteed that clang won't fail when it can't find a module map referenced by another module map? This behavior was pretty surprising to me.
  3. Since LLVM 17, clang can suggest a module providing a header included from a non-direct dependency (https://github.com/llvm/llvm-project/commit/ab0116e2f05c6156c4bc3d35986de1e98cc27016). This convenience feature is similar to JavaBuilder strict deps fixups, but requires transitive module maps. Does Google have an alternative for this feature that doesn't require more inputs?

fmeum avatar Mar 06 '24 09:03 fmeum

Hit this same case trying to upgrade LLVM to bazel 7.x, not sure yet how I can workaround it

keith avatar Mar 22 '24 19:03 keith

@keith Does it happen with sandboxing enabled?

fmeum avatar Mar 22 '24 19:03 fmeum

Yes, but I can't currently repro to test without sandboxing https://github.com/llvm/llvm-project/pull/86297#issuecomment-2015660662

keith avatar Mar 22 '24 20:03 keith

We stumbled upon this issue here. We need to turn off sandboxing to do static analysis. Our workaround just consists in disabling the feature, which seems to be on by default:

bazel build //your/target --spawn_strategy=local --features=-layering_check

redsun82 avatar Mar 26 '24 16:03 redsun82

Debugging this a bit, it appears the issue is that when sandboxing is disabled the target's .d file ends up with dependencies on more cppmaps than it depends on (presumably from its transitives) which leads to this error, which is actually correct if those were really being depended on. Looking for a fix

keith avatar Mar 27 '24 17:03 keith

This is trivially reproducible outside of a bazel as well. It appears clang includes any transitive modulemaps in the dotd file when they are readable (hence the sandboxing requirement here), even if there is no direct dependency on them.

keith avatar Mar 27 '24 17:03 keith

submitted a fix here https://github.com/bazelbuild/bazel/pull/21832, please leave any feedback there!

keith avatar Mar 27 '24 18:03 keith

I noticed that this case is actually super hard to get out of as well I think. Unsurprising since it's theoretically non-hermetic but I think you have to clean expunge in some cases.

keith avatar Apr 30 '24 00:04 keith

A fix for this issue has been included in Bazel 7.3.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.3.0rc1. Thanks!

iancha1992 avatar Jul 29 '24 22:07 iancha1992