Support `-Cpanic=abort` #1899

bsilver8192 · 2023-03-24T01:56:45Z

This takes some effort to switch back to -Cpanic=unwind for tests and
proc macros, which are incompatible with it.

This takes some effort to switch back to `-Cpanic=unwind` for tests and proc macros, which are incompatible with it.

bsilver8192 · 2023-03-24T19:25:33Z

Looks like wasm linking with CcInfo is currently broken on master, because wasm only supports -Cpanic=abort.

krasimirgg · 2023-03-29T12:32:21Z

Thank you! This is super exciting and I'm looking to review it but will need some time to read up on the details of panicking. Want to pay special attention to this because of the new test transition and since the default rustc panicking strategy (when we don't pass any -Cpanic seems non-obvious, target-, and final-artifact-type- specific).

Just to start with, what's your high-level expectations about this? Naively, since tests don't support -Cpanic=abort right now, if I wanted a rust_library or a rust_binary built with -Cpanic=abort, I would try something along the lines of:
bazel build --@rules_rust//:extra_rustc_flags=-Cpanic=abort //path/to:rust_lib, possibly adding it to a Bazel config file, and not invoking it for tests... Does this suffice for your workflow?

I also wonder how can we evolve this to also support the unstable -Zpanic-abort-tests feature (rust-lang/rust#67650)?

bsilver8192 · 2023-03-29T19:11:46Z

Thank you! This is super exciting and I'm looking to review it but will need some time to read up on the details of panicking. Want to pay special attention to this because of the new test transition and since the default rustc panicking strategy (when we don't pass any -Cpanic seems non-obvious, target-, and final-artifact-type- specific).

Yes indeed.

Just to start with, what's your high-level expectations about this? Naively, since tests don't support -Cpanic=abort right now, if I wanted a rust_library or a rust_binary built with -Cpanic=abort, I would try something along the lines of: bazel build --@rules_rust//:extra_rustc_flags=-Cpanic=abort //path/to:rust_lib, possibly adding it to a Bazel config file, and not invoking it for tests... Does this suffice for your workflow?

Basically I want things to work like they do with cargo, where I can build my binaries with -Cabort=panic but still have tests and proc macros work. My initial motivation for not using a flag is wanting bazel build and bazel test to share the local cache nicely, if you swap the flag then they overwrite things and end up over-rebuilding every time you switch. I later realized that proc macros also don't work correctly if you just add the flag to everything.

Also having the stdlib code dig through extra_rustc_flag and extra_rustc_flags to determine which library to include seems messy.

I also wonder how can we evolve this to also support the unstable -Zpanic-abort-tests feature (rust-lang/rust#67650)?

I'm thinking that would be another flag, and then switch tests to use something besides force_panic_unwind_transition which looks at the flag. I think proc macros will still use force_panic_unwind_transition as-is.

I'm hoping to have time to debug the Windows test failures this week/weekend. I need to get a Windows dev environment set up, for this and another of my open PRs.

scentini · 2023-04-11T12:20:14Z

rust/private/rustc.bzl

+        ))
+    return [PanicStyleInfo(panic_style = raw)]
+
+panic_style = rule(


We already have a string_flag build setting: https://bazel.build/extending/config#predefined-settings

Is there a reason why this is a custom rule instead of a string_flag?

I hadn't seen that, that is nicer. Done

Although that does mean it disappears from the docs.

rust/toolchain.bzl

scentini · 2023-04-11T12:24:09Z

rust/private/rustc.bzl

+    # 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"):


Could we move this logic over to toolchain.bzl, and use a toolchain.default_panic_style here?

scentini · 2023-04-11T12:25:44Z

rust/toolchain.bzl

    """Make the CcInfo (if possible) for libstd and allocator libraries.

    Args:
        ctx (ctx): The rule's context object.
        rust_std: The Rust standard library.
        allocator_library: The target to use for providing allocator functions.
+        panic: Either "unwind" or "abort" to selection which panic implementation to include.


Suggested change

panic: Either "unwind" or "abort" to selection which panic implementation to include.

panic: Either "unwind" or "abort" to select which panic implementation to include.

scentini · 2023-04-11T12:29:53Z

test/unit/toolchain_make_variables/toolchain_make_variables_test.bzl

@@ -16,6 +16,13 @@ _RUSTFMT_TOOLCHAIN_ENV = {
    "ENV_VAR_RUSTFMT": "$(RUSTFMT)",
 }

+def _mangle_path(path):


I don't quite understand the change here, and even less how it relates to this PR in general, do you mind elaborating?

With the transitions, the outdir component of the path ends up different, even though it's pointing to the same file. The test fails if you revert this change.

scentini · 2023-04-11T12:32:13Z

.bazelci/presubmit.yml

@@ -496,6 +496,15 @@ tasks:
      - "//..."
    build_flags:
      - "--@rules_rust//rust/settings:experimental_use_cc_common_link=True"
+  ubuntu2004_panic_abort:
+    name: -Cabort=panic


Suggested change

name: -Cabort=panic

name: -Cpanic=abort

scentini · 2023-04-11T12:54:32Z

test/panic_style/defs.bzl

+    implementation = _with_panic_style_cfg_impl,
+    attrs = {
+        "panic_style": attr.string(mandatory = True),
+        "srcs": attr.label_list(


This is not really srcs but rather binary, right? As in, we pass the executable generated by a rust_binary here?

In this case, yes. If you look at this rule as a more general way of applying that transition to any set of things, then srcs is the more common name. I don't really care which name, do you want me to change it?

scentini · 2023-04-11T16:53:16Z

As for the Windows failures, at least this one is due to too long a path, due to the transition hash we now have in the output path:

LINK : fatal error LNK1104: cannot open file 'C:\b\tycizaqx\execroot\examples\bazel-out\x64_windows-opt-exec-2B5CBBC6-ST-e9b575d06caf\bin\external\rust_windows_x86_64__x86_64-pc-windows-msvc__stable_tools\rust_toolchain\lib\rustlib\x86_64-pc-windows-msvc\lib\librustc_std_workspace_alloc-15cc9d7e933adbe4.rlib'

I think we might be able to shorten this section: rust_windows_x86_64__x86_64-pc-windows-msvc__stable_tools by quite a bit.

scentini · 2023-04-11T18:24:29Z

I'm afraid the rest of the Windows failures might be due to too long a path too :( The Crate Universe Unnamed Examples fails with
note: LINK : fatal error LNK1181: cannot open input file 'windows.lib'
I tracked down windows.lib to the data section of a build script, which manifests as

"/LIBPATH:C:\\b\\vaql5ptt\\execroot\\__main__\\bazel-out/x64_windows-opt-exec-2B5CBBC6-ST-e9b575d06caf/bin/external/crates_vendor_manifests__windows_x86_64_msvc-0.42.2/windows_x86_64_msvc_build_script_.exe.runfiles/crates_vendor_manifests__windows_x86_64_msvc-0.42.2/lib"

on the command line - the path being 255 characters. Appending /windows.lib puts us over the limit.

We don't have much choice in the matter but trying to trim the generated paths. In this case we could maybe shorten the crates_vendor_manifests directory to gain some characters back. @bsilver8192 are you up for pursuing this?

scentini · 2023-04-20T12:27:21Z

Hey @bsilver8192 do you have a timeline for pushing this PR through?
I believe my #1926 has introduced some merge conflicts. Upcoming work on supporting no_std builds is going to further intertwine with code you're touching here. Finally, after I'm done with no_std I will very soon need --Cpanic=abort myself :)

UebelAndre · 2023-07-13T00:36:37Z

@bsilver8192 @scentini Frinedly ping, any updates here?

bsilver8192 · 2023-07-13T01:00:38Z

@UebelAndre I tried poking at #1782 to try getting that one finished first, but the Windows command environment is truly a nightmare. There's two shells plus Rust with four different kinds of processing that all have to pass arbitrary strings through without mangling them. This one is stuck behind issues in the same area, and I'm not going to get around to fixing it anytime soon. I don't have experience with or interest in Windows shells and command line paths, and these are not quick fixes (at least for me). It's kind of frustrating that both of my changes are blocked on pre-existing issues with a platform that I don't use, but at this point I've concluded that fixing them is never going to get to the top of my priority list.

I think the Windows issues are independent of this and my other PR, so somebody else can grab the tests I wrote that surface the issues and fix them. Alternatively we could just agree to skip the new tests on Windows for now. If somebody else can fix the Windows issues, I'd be happy to resolve the merge conflicts and finish the review. Alternatively if somebody else wants to take over the PRs, I'd be fine with that.

scentini · 2023-07-27T20:02:16Z

@bsilver8192 if you rebase and move forward with the review, I could work on clearing the Windows hurdles. Alternatively I'm also happy to take over the PR.

bsilver8192 · 2023-09-28T21:48:28Z

Ok @scentini I finally got back to this, sorry for the delay. I rebased and I think I responded to all your comments, when you get a chance could you take another look and work on the Windows problems?

scentini · 2023-10-02T15:57:52Z

Thanks a lot @bsilver8192 , I'll target to fix the windows failures this week.

krasimirgg · 2023-10-04T12:42:17Z

rust/toolchain.bzl

@@ -187,39 +187,29 @@ def _make_libstd_and_allocator_ccinfo(ctx, rust_std, allocator_library, std = "s

        # The libraries panic_abort and panic_unwind are alternatives.
        # The std by default requires panic_unwind.
-        # Exclude panic_abort if panic_unwind is present.
-        # TODO: Provide a setting to choose between panic_abort and panic_unwind.
+        # Exclude panic_abort if panic_unwind is present, and vice versa.


note: I'm looking into whether we may entirely remove this filtering as a follow-up to this work (originally, this was put in place due to the specifics of how rustc formed the underlying linker invocation that includes these libraries, and that has changed since). Nothing actionable for this PR, just informational.

This reverts commit cdc82e2.

UebelAndre · 2023-10-31T16:27:37Z

rust/private/rust.bzl

@@ -1115,6 +1124,7 @@ rust_test = rule(
        str(Label("//rust:toolchain_type")),
        "@bazel_tools//tools/cpp:toolchain_type",
    ],
+    cfg = force_panic_unwind_transition,


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?

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.

goffrie · 2024-04-23T21:36:00Z

Is there a way we could implement this without using transitions? Transitions are really unfortunate if you depend on any non-Rust code as all that stuff has to be rebuilt even though it doesn't change at all, due to the separate bazel-out directory :/

Previously (before using rules_rust) we had internal rules that supported panic=abort and panic=unwind by creating two actions for each rust_library target, and the provider would expose both versions of the library.

bsilver8192 added 6 commits March 23, 2023 18:54

Support -Cpanic=abort

fdb1d6f

This takes some effort to switch back to `-Cpanic=unwind` for tests and proc macros, which are incompatible with it.

Regenerate documentation

467876f

Fix some typos

4b7446c

Fix rustdoc_test

fc550fd

Fix more typos

e54107a

Merge remote-tracking branch 'upstream/main' into HEAD

51564cd

bsilver8192 added 4 commits March 24, 2023 13:09

Fix wasm and proc_macro deps

11680af

Add to CI

9bddec4

Regenerate documentation

f20dd44

Fix unused variables

9f6ce9f

scentini requested a review from krasimirgg March 28, 2023 13:10

scentini reviewed Apr 11, 2023

View reviewed changes

bsilver8192 added 9 commits September 28, 2023 13:00

Merge remote-tracking branch 'upstream/main' into HEAD

3ef7c63

Add CI jobs for some other flag permutations

13f426d

Fix merge with new allocator style stuff

15be9d3

Buildifier

97f10e5

Fix CI copy/paste

d643ffb

Fix typos

167a027

Use string_flag

a84c84e

Move the default panic style logic

898ce3f

Regenerate documentation

99b1bea

bsilver8192 added 2 commits September 28, 2023 14:42

Remove panic_style from docs

48447e0

Finish removing custom panic_style flag

c4e5dcb

krasimirgg reviewed Oct 4, 2023

View reviewed changes

Merge branch 'main' into panic-style

3b30b0c

daivinhtran pushed a commit to daivinhtran/rules_rust that referenced this pull request Oct 26, 2023

Patch github.com/bazelbuild/pull/1899

cdc82e2

daivinhtran pushed a commit to daivinhtran/rules_rust that referenced this pull request Oct 27, 2023

Revert "Patch github.com/bazelbuild/pull/1899"

3b2f9c0

This reverts commit cdc82e2.

daivinhtran pushed a commit to daivinhtran/rules_rust that referenced this pull request Oct 27, 2023

Revert "Patch github.com/bazelbuild/pull/1899"

94b8be0

This reverts commit cdc82e2.

daivinhtran mentioned this pull request Oct 27, 2023

Fix flaky build and test failures on windows #2217

Open

UebelAndre requested changes Oct 31, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `-Cpanic=abort` #1899

Support `-Cpanic=abort` #1899

bsilver8192 commented Mar 24, 2023

bsilver8192 commented Mar 24, 2023 •

edited

Loading

krasimirgg commented Mar 29, 2023

bsilver8192 commented Mar 29, 2023

scentini Apr 11, 2023

bsilver8192 Sep 28, 2023

bsilver8192 Sep 28, 2023

scentini Apr 11, 2023

bsilver8192 Sep 28, 2023

scentini Apr 11, 2023

bsilver8192 Sep 28, 2023

scentini Apr 11, 2023

bsilver8192 Apr 12, 2023

scentini Apr 11, 2023

bsilver8192 Sep 28, 2023

scentini Apr 11, 2023

bsilver8192 Sep 28, 2023

scentini commented Apr 11, 2023

scentini commented Apr 11, 2023

scentini commented Apr 20, 2023

UebelAndre commented Jul 13, 2023

bsilver8192 commented Jul 13, 2023

scentini commented Jul 27, 2023

bsilver8192 commented Sep 28, 2023

scentini commented Oct 2, 2023

krasimirgg Oct 4, 2023

UebelAndre Oct 31, 2023

bsilver8192 Oct 31, 2023

goffrie commented Apr 23, 2024

	panic: Either "unwind" or "abort" to selection which panic implementation to include.
	panic: Either "unwind" or "abort" to select which panic implementation to include.

Support -Cpanic=abort #1899

Are you sure you want to change the base?

Support -Cpanic=abort #1899

Conversation

bsilver8192 commented Mar 24, 2023

bsilver8192 commented Mar 24, 2023 • edited Loading

krasimirgg commented Mar 29, 2023

bsilver8192 commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scentini commented Apr 11, 2023

scentini commented Apr 11, 2023

scentini commented Apr 20, 2023

UebelAndre commented Jul 13, 2023

bsilver8192 commented Jul 13, 2023

scentini commented Jul 27, 2023

bsilver8192 commented Sep 28, 2023

scentini commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goffrie commented Apr 23, 2024

Support `-Cpanic=abort` #1899

Support `-Cpanic=abort` #1899

bsilver8192 commented Mar 24, 2023 •

edited

Loading