Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cc_fuzz_test() breaks non-Clang builds #176

Closed
haberman opened this issue Aug 13, 2021 · 7 comments · Fixed by #177
Closed

cc_fuzz_test() breaks non-Clang builds #176

haberman opened this issue Aug 13, 2021 · 7 comments · Fixed by #177

Comments

@haberman
Copy link

Actual Behavior

The docs for cc_fuzz_target require you to add the following lines to your .bazelrc:

# Force the use of Clang for C++ builds.
build --action_env=CC=clang
build --action_env=CXX=clang++

This makes it impossible to build the repo with any compiler other than Clang. But I want my repo to be compatible with GCC, MSVC, and other compilers, for testing my code across compilers.

I tried specifying these actions for the libfuzzer configurations only:

# Force the use of Clang for fuzzer builds only.
build:asan-libfuzzer --action_env=CC=clang
build:asan-libfuzzer --action_env=CXX=clang++

However, I discovered that the cc_fuzz_target() would then fail to build with other compilers. For example, compiling with GCC I got the following error:

bazel-out/k8-fastbuild-ST-6de91758680c/bin/external/rules_fuzzing/fuzzing/replay/_objs/replay_main/replay_main.pic.o:replay_main.cc:function main: error: undefined reference to 'LLVMFuzzerInitialize'

This is unfortunate; even if nobody tries to build the fuzzing target directly, a broken target like this will make bazel test ... fail.

Expected Behavior

Ideally, cc_fuzz_target() would automatically opt itself out of non-Clang builds using target_compatible_with.

This would cause the fuzz targets to be automatically skipped by Bazel on non-Clang builds. Users could get the best of both worlds: the repo could continue to be compatible with any compiler, but Bazel would only attempt to build the fuzzing rules if the build is using Clang.

I tried doing this myself by adding target_compatible_with to my cc_fuzz_target() rule:

cc_fuzz_test(
    name = "file_descriptor_parsenew_fuzzer",
    srcs = ["file_descriptor_parsenew_fuzzer.cc"],
    deps = [
        "//:descriptor_upb_proto",
        "//:upb",
    ],
    target_compatible_with = [
        "@bazel_tools//tools/cpp:clang",
    ],
)

However this didn't work; Bazel seemed to skip the test no matter what I did. I tried both of these commands, and both of them skipped the fuzz test:

CC=clang CXX=clang++ bazel test ...

and

bazel test --compiler=clang ...

I'm probably doing something wrong at the Bazel level. If cc_fuzz_target() has this built in I wouldn't need to figure out the right incantation to get this working. :)

@fmeum
Copy link
Member

fmeum commented Aug 13, 2021

This is a very good suggestion and I don't understand why it doesn't work. I filed bazelbuild/bazel#13841 and will investigate further.

@haberman
Copy link
Author

Awesome, thanks for looking into it!

@fmeum
Copy link
Member

fmeum commented Aug 14, 2021

I found a design doc explaining why target or execution platforms are not the right place for compiler constraints. In fact, I believe that the @bazel_tools//tools/cpp:... constraint_values are only used to select toolchains (via exec_compatible_with), not targets. There is nice syntax for your use case described in the Incompatible Target Skipping design doc, but it hasn't been implemented yet.

What does work today is the following:

config_setting(
    name = "is_clang",
    flag_values = {"@bazel_tools//tools/cpp:compiler": "clang"},
)

cc_test(
    name = "clang",
    srcs = ["empty.cc"],
    target_compatible_with = select({
        ":is_clang": [],
        "//conditions:default": ["@platforms//:incompatible"]
    }),
)

@haberman If you find that this works for you, I would look into whether rules_fuzzing fuzz tests could have this behavior out of the box.

@haberman
Copy link
Author

That seems to work!

  • bazel test ... successfully skips the test
  • CC=clang bazel test ... successfully runs the test
  • bazel test ... --action_env=CC=clang --action_env=CXX=clang++ successfully runs the test
  • bazel test --config=asan-libfuzzer ... successfully runs the test if my .bazelrc has build:asan-libfuzzer --action_env=CC=clang and build:asan-libfuzzer --action_env=CXX=clang++

I'm slightly surprised that a default blaze test uses gcc instead of cc (on my system cc symlinks to clang). But I know now that I need to explicitly specify clang to get clang.

@fmeum
Copy link
Member

fmeum commented Aug 15, 2021

@stefanbucur Would you like to preserve compatibility with Bazel 3.x? If that is not a priority, the fuzz tests macros could use target_compatible_with to get graceful skipping with compiler other than clang by default.

@stefanbucur
Copy link
Collaborator

Technically, clang is only required for the libfuzzer configuration. So disabling the targets in non-clang configurations for all fuzzing engines seems a bit too broad.

The replay fuzzing engine should actually work with all major compilers, and can be used to turn all fuzz tests into regression tests.

But I see from @haberman's report that this doesn't happen:

However, I discovered that the cc_fuzz_target() would then fail to build with other compilers. For example, compiling with GCC I got the following error:

bazel-out/k8-fastbuild-ST-6de91758680c/bin/external/rules_fuzzing/fuzzing/replay/_objs/replay_main/replay_main.pic.o:replay_main.cc:function main: error: undefined reference to 'LLVMFuzzerInitialize'

I think this part is problematic, since the symbol was supposed to be weakly defined: https://github.com/bazelbuild/rules_fuzzing/blob/b2b3efdcd70d319c6e0e9b687baff3b54e5a5711/fuzzing/replay/replay_main.cc#L25

Maybe #139 introduced an incompatibility with GCC? @fmeum would you like to look into this?

@fmeum
Copy link
Member

fmeum commented Aug 16, 2021

I will try to restore GCC compatibility and verify it in the CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants