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

Support -Cpanic=abort #1899

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,87 @@ tasks:
test_flags:
- "--@rules_rust//rust/toolchain/channel=nightly"
- "--@rules_rust//:no_std=alloc"
cc_common_link_panic_abort_ubuntu2004:
name: Build via cc_common.link and -Cpanic=abort
platform: ubuntu2004
working_directory: test/cc_common_link
build_targets:
- "--"
- "//..."
# The with_global_alloc directory is a repository on its own tested in the 'Build via cc_common.link using a global allocator' task.
- "-//with_global_alloc/..."
test_targets:
- "--"
- "//..."
# The with_global_alloc directory is a repository on its own tested in the 'Build via cc_common.link using a global allocator' task.
- "-//with_global_alloc/..."
build_flags:
- "--@rules_rust//rust/settings:experimental_use_cc_common_link=True"
- "--@rules_rust//:panic_style=abort"
test_flags:
- "--@rules_rust//rust/settings:experimental_use_cc_common_link=True"
- "--@rules_rust//:panic_style=abort"
cc_common_link_with_global_alloc_panic_abort_ubuntu2004:
name: Build via cc_common.link using a global allocator and -Cpanic=abort
platform: ubuntu2004
working_directory: test/cc_common_link/with_global_alloc
build_targets:
- "//..."
test_targets:
- "//..."
build_flags:
- "--@rules_rust//rust/settings:experimental_use_cc_common_link=True"
- "--@rules_rust//rust/settings:experimental_use_global_allocator=True"
- "--@rules_rust//:panic_style=abort"
test_flags:
- "--@rules_rust//rust/settings:experimental_use_cc_common_link=True"
- "--@rules_rust//rust/settings:experimental_use_global_allocator=True"
- "--@rules_rust//:panic_style=abort"
cc_common_link_no_std_panic_abort_ubuntu2004:
name: Build with no_std + alloc using cc_common.link infrastructure for linking and -Cpanic=abort
platform: ubuntu2004
working_directory: test/no_std
build_targets:
- "//..."
test_targets:
- "//..."
build_flags:
- "--@rules_rust//rust/toolchain/channel=nightly"
- "--@rules_rust//rust/settings:experimental_use_cc_common_link=True"
- "--@rules_rust//rust/settings:experimental_use_global_allocator=True"
- "--@rules_rust//:no_std=alloc"
- "--@rules_rust//:panic_style=abort"
test_flags:
- "--@rules_rust//rust/toolchain/channel=nightly"
- "--@rules_rust//rust/settings:experimental_use_cc_common_link=True"
- "--@rules_rust//rust/settings:experimental_use_global_allocator=True"
- "--@rules_rust//:no_std=alloc"
- "--@rules_rust//:panic_style=abort"
no_std_panic_abort_ubuntu2004:
name: Build with no_std + alloc + -Cpanic=abort
platform: ubuntu2004
working_directory: test/no_std
build_targets:
- "//..."
test_targets:
- "//..."
build_flags:
- "--@rules_rust//rust/toolchain/channel=nightly"
- "--@rules_rust//:no_std=alloc"
- "--@rules_rust//:panic_style=abort"
test_flags:
- "--@rules_rust//rust/toolchain/channel=nightly"
- "--@rules_rust//:no_std=alloc"
- "--@rules_rust//:panic_style=abort"
panic_abort_ubuntu2004:
name: Build and test with -Cpanic=abort
platform: ubuntu2004
build_targets: *default_linux_targets
test_targets: *default_linux_targets
build_flags:
- "--@rules_rust//:panic_style=abort"
test_flags:
- "--@rules_rust//:panic_style=abort"
android_examples_ubuntu2004:
name: Android Examples
platform: ubuntu2004
Expand Down
12 changes: 12 additions & 0 deletions BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ error_format(
visibility = ["//visibility:public"],
)

# This setting may be changed from the command line to panic immediately upon abort.
string_flag(
name = "panic_style",
build_setting_default = "",
values = [
"abort",
"unwind",
"",
],
visibility = ["//visibility:public"],
)

# This setting may be used to pass extra options to clippy from the command line.
# It applies across all targets.
clippy_flags(
Expand Down
12 changes: 11 additions & 1 deletion rust/private/rust.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ load(
"determine_output_hash",
"expand_dict_value_locations",
"find_toolchain",
"force_panic_unwind_transition",
"get_edition",
"get_import_macro_deps",
"transform_deps",
Expand Down Expand Up @@ -631,6 +632,9 @@ _common_attrs = {
"_is_proc_macro_dep_enabled": attr.label(
default = Label("//:is_proc_macro_dep_enabled"),
),
"_panic_style": attr.label(
default = Label("//:panic_style"),
),
"_per_crate_rustc_flag": attr.label(
default = Label("//:experimental_per_crate_rustc_flag"),
),
Expand Down Expand Up @@ -890,6 +894,7 @@ _proc_macro_dep_transition = transition(
rust_proc_macro = rule(
implementation = _rust_proc_macro_impl,
provides = _common_providers,
cfg = force_panic_unwind_transition,
# Start by copying the common attributes, then override the `deps` attribute
# to apply `_proc_macro_dep_transition`. To add this transition we additionally
# need to declare `_allowlist_function_transition`, see
Expand Down Expand Up @@ -1106,7 +1111,11 @@ rust_test = rule(
implementation = _rust_test_impl,
provides = _common_providers,
attrs = dict(_common_attrs.items() +
_rust_test_attrs.items()),
_rust_test_attrs.items() + {
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
}.items()),
executable = True,
fragments = ["cpp"],
host_fragments = ["cpp"],
Expand All @@ -1115,6 +1124,7 @@ rust_test = rule(
str(Label("//rust:toolchain_type")),
"@bazel_tools//tools/cpp:toolchain_type",
],
cfg = force_panic_unwind_transition,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this not also be solved by having this flag appear in the rust_toolchain instead of for each test and proc-macro? Also, why is this not for binaries as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to apply only to tests (the overall goal of this PR is build binaries without this transition). #[should_panic] doesn't work with -Cpanic=abort, so Cargo automatically builds tests with -Cpanic=unwind, which is the behavior I'm copying here.

https://doc.rust-lang.org/cargo/reference/profiles.html#panic documents this:

Tests, benchmarks, build scripts, and proc macros ignore the panic setting. The rustc test harness currently requires unwind behavior. See the panic-abort-tests unstable flag which enables abort behavior.

Additionally, when using the abort strategy and building a test, all of the dependencies will also be forced to build with the unwind strategy.

incompatible_use_toolchain_transition = True,
doc = dedent("""\
Builds a Rust test crate.
Expand Down
35 changes: 30 additions & 5 deletions rust/private/rustc.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ def collect_inputs(
force_depend_on_objects (bool, optional): Forces dependencies of this rule to be objects rather than
metadata, even for libraries. This is used in rustdoc tests.
experimental_use_cc_common_link (bool, optional): Whether rules_rust uses cc_common.link to link
rust binaries.
rust binaries.

Returns:
tuple: A tuple: A tuple of the following items:
Expand Down Expand Up @@ -932,6 +932,8 @@ def construct_arguments(

rustc_flags.add(error_format, format = "--error-format=%s")

rustc_flags.add("-Cpanic=" + _get_panic_style(ctx, toolchain))

# Mangle symbols to disambiguate crates with the same name. This could
# happen only for non-final artifacts where we compute an output_hash,
# e.g., rust_library.
Expand Down Expand Up @@ -1080,6 +1082,15 @@ def construct_arguments(

return args, env

def _get_panic_style(ctx, toolchain):
panic_style = toolchain.default_panic_style

if hasattr(ctx.attr, "_panic_style"):
flag_value = ctx.attr._panic_style[BuildSettingInfo].value
if flag_value:
panic_style = flag_value
return panic_style

def rustc_compile_action(
ctx,
attr,
Expand Down Expand Up @@ -1469,15 +1480,29 @@ def _is_no_std(ctx, toolchain, crate_info):
return True

def _get_std_and_alloc_info(ctx, toolchain, crate_info):
panic_style = _get_panic_style(ctx, toolchain)
if panic_style != "unwind" and panic_style != "abort":
fail("Unrecognized panic style: " + panic_style)

if is_exec_configuration(ctx):
return toolchain.libstd_and_allocator_ccinfo
if panic_style == "unwind":
return toolchain.libstd_and_allocator_unwind_ccinfo
else:
return toolchain.libstd_and_allocator_abort_ccinfo
if toolchain._experimental_use_global_allocator:
if _is_no_std(ctx, toolchain, crate_info):
return toolchain.nostd_and_global_allocator_cc_info
if panic_style == "unwind":
return toolchain.nostd_and_global_allocator_unwind_ccinfo
else:
return toolchain.nostd_and_global_allocator_abort_ccinfo
elif panic_style == "unwind":
return toolchain.libstd_and_global_allocator_unwind_ccinfo
else:
return toolchain.libstd_and_global_allocator_ccinfo
return toolchain.libstd_and_global_allocator_abort_ccinfo
elif panic_style == "unwind":
return toolchain.libstd_and_allocator_unwind_ccinfo
else:
return toolchain.libstd_and_allocator_ccinfo
return toolchain.libstd_and_allocator_abort_ccinfo

def _is_dylib(dep):
return not bool(dep.static_library or dep.pic_static_library)
Expand Down
6 changes: 5 additions & 1 deletion rust/private/rustdoc_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
load("//rust/private:common.bzl", "rust_common")
load("//rust/private:providers.bzl", "CrateInfo")
load("//rust/private:rustdoc.bzl", "rustdoc_compile_action")
load("//rust/private:utils.bzl", "dedent", "find_toolchain", "transform_deps")
load("//rust/private:utils.bzl", "dedent", "find_toolchain", "force_panic_unwind_transition", "transform_deps")

def _construct_writer_arguments(ctx, test_runner, opt_test_params, action, crate_info):
"""Construct arguments and environment variables specific to `rustdoc_test_writer`.
Expand Down Expand Up @@ -207,6 +207,9 @@ rust_doc_test = rule(
"""),
providers = [[CrateInfo], [CcInfo]],
),
"_allowlist_function_transition": attr.label(
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
),
"_cc_toolchain": attr.label(
doc = (
"In order to use find_cc_toolchain, your rule has to depend " +
Expand All @@ -229,6 +232,7 @@ rust_doc_test = rule(
),
},
test = True,
cfg = force_panic_unwind_transition,
fragments = ["cpp"],
host_fragments = ["cpp"],
toolchains = [
Expand Down
9 changes: 9 additions & 0 deletions rust/private/utils.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -849,3 +849,12 @@ def _symlink_for_non_generated_source(ctx, src_file, package_root):
return src_symlink
else:
return src_file

def _force_panic_unwind_transition_impl(_settings, _attr):
return {"//:panic_style": "unwind"}

force_panic_unwind_transition = transition(
implementation = _force_panic_unwind_transition_impl,
inputs = [],
outputs = ["//:panic_style"],
)
Loading
Loading