From 3074ed38e0df3ca0d1255986390f7d8db277510a Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Wed, 2 Jul 2025 22:54:27 +0100 Subject: [PATCH 1/6] Fix panic line numbers Without this patch, line numbers are missing for first-party code in panic traces. This reverts part of #1563 but keeps the test showing we have a relative path not an absolute one, and adds an integration test showing that line numbers are correctly reported in panic traces. As far as I know, path remapping has been improved in rustc over the last few years, so maybe the behaviour reported in that patch has been fixed. --- rust/private/rustc.bzl | 2 +- test/unit/remap_path_prefix/BUILD.bazel | 25 ++++++- .../remap_path_prefix/debug_transition.bzl | 73 +++++++++++++++++++ .../remap_path_prefix/integration_test.rs | 32 ++++++++ test/unit/remap_path_prefix/panic_bin.rs | 7 ++ test/unit/remap_path_prefix/test.rs | 2 +- 6 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 test/unit/remap_path_prefix/debug_transition.bzl create mode 100644 test/unit/remap_path_prefix/integration_test.rs create mode 100644 test/unit/remap_path_prefix/panic_bin.rs diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index d78c28902c..8faf729995 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -829,7 +829,7 @@ def construct_arguments( add_flags_for_binary = False, include_link_flags = True, stamp = False, - remap_path_prefix = "", + remap_path_prefix = ".", use_json_output = False, build_metadata = False, force_depend_on_objects = False, diff --git a/test/unit/remap_path_prefix/BUILD.bazel b/test/unit/remap_path_prefix/BUILD.bazel index 5227b87de1..03739da675 100644 --- a/test/unit/remap_path_prefix/BUILD.bazel +++ b/test/unit/remap_path_prefix/BUILD.bazel @@ -1,4 +1,5 @@ -load("//rust:defs.bzl", "rust_library", "rust_test") +load("//rust:defs.bzl", "rust_binary", "rust_library", "rust_test") +load(":debug_transition.bzl", "dbg_rust_binary") rust_library( name = "dep", @@ -12,3 +13,25 @@ rust_test( edition = "2018", deps = [":dep"], ) + +rust_binary( + name = "panic_bin", + srcs = ["panic_bin.rs"], + edition = "2018", +) + +dbg_rust_binary( + name = "panic_bin_dbg", + binary = ":panic_bin", +) + +rust_test( + name = "integration_test", + srcs = ["integration_test.rs"], + data = [":panic_bin_dbg"], + edition = "2018", + env = { + "BINARY": "$(rlocationpath :panic_bin_dbg)", + }, + deps = ["//rust/runfiles"], +) diff --git a/test/unit/remap_path_prefix/debug_transition.bzl b/test/unit/remap_path_prefix/debug_transition.bzl new file mode 100644 index 0000000000..eff4540e59 --- /dev/null +++ b/test/unit/remap_path_prefix/debug_transition.bzl @@ -0,0 +1,73 @@ +"""Internal utility for transitioning a target to -c dbg.""" + +load("@bazel_skylib//lib:paths.bzl", "paths") + +def _transition_dbg_impl(_1, _2): + return {"//command_line_option:compilation_mode": "dbg"} + +_transition_dbg = transition( + implementation = _transition_dbg_impl, + inputs = [], + outputs = ["//command_line_option:compilation_mode"], +) + +def _dbg_rust_binary_impl(ctx): + # We need to forward the DefaultInfo provider from the underlying rule. + # Unfortunately, we can't do this directly, because Bazel requires that the executable to run + # is actually generated by this rule, so we need to symlink to it, and generate a synthetic + # forwarding DefaultInfo. + + result = [] + binary = ctx.attr.binary[0] + + default_info = binary[DefaultInfo] + files = default_info.files + new_executable = None + original_executable = default_info.files_to_run.executable + runfiles = default_info.default_runfiles + + if not original_executable: + fail("Cannot transition a 'binary' that is not executable") + + new_executable_name = original_executable.basename + + # In order for the symlink to have the same basename as the original + # executable (important in the case of proto plugins), put it in a + # subdirectory named after the label to prevent collisions. + new_executable = ctx.actions.declare_file(paths.join(ctx.label.name, new_executable_name)) + + ctx.actions.symlink( + output = new_executable, + target_file = original_executable, + is_executable = True, + ) + + files = depset(direct = [new_executable]) + runfiles = ctx.runfiles([new_executable]) + + result.append( + DefaultInfo( + files = files, + runfiles = runfiles, + executable = new_executable, + ), + ) + + return result + +dbg_rust_binary = rule( + doc = "Transitions the binary to use the provided platform.", + implementation = _dbg_rust_binary_impl, + attrs = { + "binary": attr.label( + doc = "The target to transition.", + allow_files = True, + cfg = _transition_dbg, + mandatory = True, + ), + "_allowlist_function_transition": attr.label( + default = "@bazel_tools//tools/allowlists/function_transition_allowlist", + ), + }, + executable = True, +) diff --git a/test/unit/remap_path_prefix/integration_test.rs b/test/unit/remap_path_prefix/integration_test.rs new file mode 100644 index 0000000000..916959a385 --- /dev/null +++ b/test/unit/remap_path_prefix/integration_test.rs @@ -0,0 +1,32 @@ +#[test] +fn test_backtrace() { + let runfiles = runfiles::Runfiles::create().unwrap(); + + let binary_env = std::env::var("BINARY").unwrap(); + + let binary_path = runfiles::rlocation!(runfiles, &binary_env).unwrap(); + + eprintln!("Running {}", binary_path.display()); + + let output = std::process::Command::new(binary_path) + .env("RUST_BACKTRACE", "full") + .output() + .unwrap(); + let stderr = String::from_utf8(output.stderr).unwrap(); + + eprintln!("Saw backtrace:\n{}", stderr); + + let mut check_next = false; + for line in stderr.split('\n') { + if check_next { + if !line.contains("./test/unit/remap_path_prefix/panic_bin.rs:6:5") { + panic!("Expected line to contain ./test/unit/remap_path_prefix/panic_bin.rs:6:5 but was {}", line); + } + return; + } + if line.contains("panic_bin::do_call") { + check_next = true; + } + } + panic!("Didn't see expected line containing panic_bin::do_call"); +} diff --git a/test/unit/remap_path_prefix/panic_bin.rs b/test/unit/remap_path_prefix/panic_bin.rs new file mode 100644 index 0000000000..fc1b9a3eb3 --- /dev/null +++ b/test/unit/remap_path_prefix/panic_bin.rs @@ -0,0 +1,7 @@ +fn main() { + do_call(); +} + +fn do_call() { + panic!("This is bad!"); +} diff --git a/test/unit/remap_path_prefix/test.rs b/test/unit/remap_path_prefix/test.rs index 167f786d41..f2be45241c 100644 --- a/test/unit/remap_path_prefix/test.rs +++ b/test/unit/remap_path_prefix/test.rs @@ -2,6 +2,6 @@ fn test_dep_file_name() { assert_eq!( dep::get_file_name::<()>(), - "test/unit/remap_path_prefix/dep.rs" + "./test/unit/remap_path_prefix/dep.rs" ); } From bca9f8e17aab4b500bb57718076e8dbfc5e3f745 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Wed, 2 Jul 2025 23:28:04 +0100 Subject: [PATCH 2/6] Match line more vaguely - coverage mangles this --- test/unit/remap_path_prefix/integration_test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/remap_path_prefix/integration_test.rs b/test/unit/remap_path_prefix/integration_test.rs index 916959a385..dcac30d61b 100644 --- a/test/unit/remap_path_prefix/integration_test.rs +++ b/test/unit/remap_path_prefix/integration_test.rs @@ -24,7 +24,7 @@ fn test_backtrace() { } return; } - if line.contains("panic_bin::do_call") { + if line.contains("::do_call") { check_next = true; } } From 1fbe8b50cbca8d3d90530c6ac05f92fedad012e1 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Wed, 2 Jul 2025 23:35:09 +0100 Subject: [PATCH 3/6] Filter CI --- .bazelci/presubmit.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index b98647ea58..f1898583ea 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml @@ -66,9 +66,13 @@ default_rbe_targets: &default_rbe_targets default_macos_targets: &default_macos_targets - "--" - "//..." + # Not sure why this doesn't work on bazelci, it does locally on macOS. + - "-//test/unit/remap_path_prefix:integration_test" default_windows_targets: &default_windows_targets - "--" # Allows negative patterns; hack for https://github.com/bazelbuild/continuous-integration/pull/245 - "//..." + # Panic backtraces appear not to work well on Windows. + - "-//test/unit/remap_path_prefix:integration_test" crate_universe_vendor_example_targets: &crate_universe_vendor_example_targets - "//vendor_external:crates_vendor" - "//vendor_local_manifests:crates_vendor" From 7501b09bba61b8c32e25ae743124dabb1a51acf2 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Wed, 2 Jul 2025 23:42:31 +0100 Subject: [PATCH 4/6] Cross-platform expectation --- test/unit/remap_path_prefix/test.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/unit/remap_path_prefix/test.rs b/test/unit/remap_path_prefix/test.rs index f2be45241c..aed3805357 100644 --- a/test/unit/remap_path_prefix/test.rs +++ b/test/unit/remap_path_prefix/test.rs @@ -1,7 +1,13 @@ #[test] fn test_dep_file_name() { + let mut expected = std::path::PathBuf::from("."); + expected.push("test"); + expected.push("unit"); + expected.push("remap_path_prefix"); + expected.push("dep.rs"); + let expected_str = expected.to_str().unwrap(); assert_eq!( dep::get_file_name::<()>(), - "./test/unit/remap_path_prefix/dep.rs" + expected_str ); } From 57c03d7b22ad204da692d3c8310dd18535ca93fa Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Wed, 2 Jul 2025 23:45:23 +0100 Subject: [PATCH 5/6] fmt --- test/unit/remap_path_prefix/test.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/unit/remap_path_prefix/test.rs b/test/unit/remap_path_prefix/test.rs index aed3805357..0a658ffe36 100644 --- a/test/unit/remap_path_prefix/test.rs +++ b/test/unit/remap_path_prefix/test.rs @@ -6,8 +6,5 @@ fn test_dep_file_name() { expected.push("remap_path_prefix"); expected.push("dep.rs"); let expected_str = expected.to_str().unwrap(); - assert_eq!( - dep::get_file_name::<()>(), - expected_str - ); + assert_eq!(dep::get_file_name::<()>(), expected_str); } From db847248e30958307e326979eec0872e6aec2ec2 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Wed, 2 Jul 2025 23:49:24 +0100 Subject: [PATCH 6/6] Fix expectation on Windows --- test/unit/remap_path_prefix/test.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/unit/remap_path_prefix/test.rs b/test/unit/remap_path_prefix/test.rs index 0a658ffe36..bf1d7ae0b8 100644 --- a/test/unit/remap_path_prefix/test.rs +++ b/test/unit/remap_path_prefix/test.rs @@ -1,10 +1,9 @@ #[test] fn test_dep_file_name() { let mut expected = std::path::PathBuf::from("."); - expected.push("test"); - expected.push("unit"); - expected.push("remap_path_prefix"); - expected.push("dep.rs"); + // After the ., the path components appear to be joined with / on all platforms. + // This is probably a rustc bug we should report. + expected.push("test/unit/remap_path_prefix/dep.rs"); let expected_str = expected.to_str().unwrap(); assert_eq!(dep::get_file_name::<()>(), expected_str); }