-
Notifications
You must be signed in to change notification settings - Fork 431
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
base: main
Are you sure you want to change the base?
Support -Cpanic=abort
#1899
Changes from 10 commits
fdb1d6f
467876f
4b7446c
fc550fd
e54107a
51564cd
11680af
9bddec4
f20dd44
9f6ce9f
3ef7c63
13f426d
15be9d3
97f10e5
d643ffb
167a027
a84c84e
898ce3f
99b1bea
48447e0
c4e5dcb
3b30b0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ load( | |
"determine_output_hash", | ||
"expand_dict_value_locations", | ||
"find_toolchain", | ||
"force_panic_unwind_transition", | ||
"get_import_macro_deps", | ||
"transform_deps", | ||
) | ||
|
@@ -682,6 +683,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"), | ||
), | ||
|
@@ -910,6 +914,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 | ||
|
@@ -1132,7 +1137,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"], | ||
|
@@ -1141,6 +1150,7 @@ rust_test = rule( | |
str(Label("//rust:toolchain_type")), | ||
"@bazel_tools//tools/cpp:toolchain_type", | ||
], | ||
cfg = force_panic_unwind_transition, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). https://doc.rust-lang.org/cargo/reference/profiles.html#panic documents this:
|
||
incompatible_use_toolchain_transition = True, | ||
doc = dedent("""\ | ||
Builds a Rust test crate. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,13 @@ ErrorFormatInfo = provider( | |
fields = {"error_format": "(string) [" + ", ".join(_error_format_values) + "]"}, | ||
) | ||
|
||
_panic_style_values = ["unwind", "abort", ""] | ||
|
||
PanicStyleInfo = provider( | ||
doc = "Set the panic style", | ||
fields = {"panic_style": "(string) [" + ", ".join(_panic_style_values) + "]"}, | ||
) | ||
|
||
ExtraRustcFlagsInfo = provider( | ||
doc = "Pass each value as an additional flag to non-exec rustc invocations", | ||
fields = {"extra_rustc_flags": "List[string] Extra flags to pass to rustc in non-exec configuration"}, | ||
|
@@ -611,7 +618,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: | ||
|
@@ -884,6 +891,8 @@ def construct_arguments( | |
|
||
rustc_flags.add("--error-format=" + error_format) | ||
|
||
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. | ||
|
@@ -1029,6 +1038,30 @@ def construct_arguments( | |
|
||
return args, env | ||
|
||
def _get_panic_style(ctx, toolchain): | ||
panic_style = "unwind" | ||
|
||
# Most targets default to unwind, but a few default to abort. Can't find a list in the | ||
# documentation, this list is extracted from `library/panic_unwind/src/lib.rs` as of 1.68.1. | ||
target_triple = toolchain.target_triple | ||
if target_triple.arch.startswith("wasm") or target_triple.arch == "avr" or target_triple.system in ("none", "uefi", "espidf"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we move this logic over to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
panic_style = "abort" | ||
|
||
if hasattr(ctx.attr, "_panic_style"): | ||
flag_value = ctx.attr._panic_style[PanicStyleInfo].panic_style | ||
if flag_value: | ||
panic_style = flag_value | ||
return panic_style | ||
|
||
def _get_libstd_and_allocator_ccinfo(ctx, toolchain): | ||
panic_style = _get_panic_style(ctx, toolchain) | ||
if panic_style == "unwind": | ||
return toolchain.unwind_libstd_and_allocator_ccinfo | ||
elif panic_style == "abort": | ||
return toolchain.abort_libstd_and_allocator_ccinfo | ||
else: | ||
fail("Unrecognized panic style: " + panic_style) | ||
|
||
def rustc_compile_action( | ||
ctx, | ||
attr, | ||
|
@@ -1267,7 +1300,7 @@ def rustc_compile_action( | |
) | ||
|
||
# Collect the linking contexts of the standard library and dependencies. | ||
linking_contexts = [toolchain.libstd_and_allocator_ccinfo.linking_context, toolchain.stdlib_linkflags.linking_context] | ||
linking_contexts = [_get_libstd_and_allocator_ccinfo(ctx, toolchain).linking_context, toolchain.stdlib_linkflags.linking_context] | ||
|
||
for dep in crate_info.deps.to_list(): | ||
if dep.cc_info: | ||
|
@@ -1486,9 +1519,10 @@ def establish_cc_info(ctx, attr, crate_info, toolchain, cc_toolchain, feature_co | |
else: | ||
cc_infos.append(dep.cc_info) | ||
|
||
if crate_info.type in ("rlib", "lib") and toolchain.libstd_and_allocator_ccinfo: | ||
libstd_and_allocator_ccinfo = _get_libstd_and_allocator_ccinfo(ctx, toolchain) | ||
if crate_info.type in ("rlib", "lib") and libstd_and_allocator_ccinfo: | ||
# TODO: if we already have an rlib in our deps, we could skip this | ||
cc_infos.append(toolchain.libstd_and_allocator_ccinfo) | ||
cc_infos.append(libstd_and_allocator_ccinfo) | ||
|
||
return [cc_common.merge_cc_infos(cc_infos = cc_infos)] | ||
|
||
|
@@ -1907,6 +1941,34 @@ error_format = rule( | |
build_setting = config.string(flag = True), | ||
) | ||
|
||
def _panic_style_impl(ctx): | ||
"""Implementation of the `panic_style` rule | ||
|
||
Args: | ||
ctx (ctx): The rule's context object | ||
|
||
Returns: | ||
list: A list containing the PanicStyleInfo provider | ||
""" | ||
raw = ctx.build_setting_value | ||
if raw not in _panic_style_values: | ||
fail("{} expected a value in `{}` but got `{}`".format( | ||
ctx.label, | ||
_panic_style_values, | ||
raw, | ||
)) | ||
return [PanicStyleInfo(panic_style = raw)] | ||
|
||
panic_style = rule( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have a Is there a reason why this is a custom rule instead of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hadn't seen that, that is nicer. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although that does mean it disappears from the docs. |
||
doc = ( | ||
"Change the [-Cpanic](https://doc.rust-lang.org/rustc/codegen-options/index.html#panic) " + | ||
"flag from the command line with `--@rules_rust//:panic_style`. See rustc documentation for valid values. " + | ||
"Automatically set to `unwind` for proc macros and tests, or the per-target default." | ||
), | ||
implementation = _panic_style_impl, | ||
build_setting = config.string(flag = True), | ||
) | ||
|
||
def _extra_rustc_flags_impl(ctx): | ||
return ExtraRustcFlagsInfo(extra_rustc_flags = ctx.build_setting_value) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done