Avoid unused unwindlib flag in rustc links - #38
Conversation
This reverts commit 59a507e.
Add support for tier 3 targets bpfeb-unknown-none and bpfel-unknown-none (see https://github.com/rust-lang/rust/blob/f5e2df7/src/doc/rustc/src/platform-support.md?plain=1#L311-L312). This is modeled after bazelbuild#3507 and should probably be updated if/when bazelbuild/platforms#131 is merged. (please use rebase merge when landing this as the proper commit message is in the commit, rather than the PR description) /cc @avrabe
…elbuild#3829)" This reverts commit f198dde.
…() (bazelbuild#3816)" This reverts commit 9586468.
…bazelbuild#3) * 0 * Add rust analyzer test coverage
…hollow rlibs: the RustcMetadata action runs rustc to completion with -Zno-codegen, emitting a .rlib archive. This approach mirrors the one used by buck2 and avoids needing to kill rustc mid-output in order to produce metadata. While not fixing problems with SVH mismatches when non-determinism, this does simplify the codepath and uses a production tested technique that doesn't have any of the dangers associated with killing the rustc process while it's still active.
Port the sharding wrapper feature from bazelbuild#3774 into the hermeticbuild fork. The implementation wraps rust_test executables when experimental_enable_sharding is set while keeping rustc_compile_action's existing provider-list API for internal and extension callers. rust_test now scans the returned providers to replace DefaultInfo for the wrapper, so extensions such as prost and wasm-bindgen continue to consume rustc_compile_action without API churn. Co-authored-by: Brian Duff <bduff@linkedin.com> Co-authored-by: Codex <noreply@openai.com>
Rustc emits GNU-like Windows staticlibs as lib<crate>.a, but rules_rust was stripping the lib prefix for all Windows non-rlib library outputs. Keep the prefix for staticlib outputs when the target ABI is gnu or gnullvm so declared outputs match rustc.
Add documentation extraction targets for the public cargo and rust Starlark packages. Declare the bazel_features, selects, and cc_debug_helper_bzl dependencies required by those targets.
Add ctx.coverage_instrumented() check so that only targets matching --instrumentation_filter get -Cinstrument-coverage, consistent with Bazel's recommended approach for rules. For rust_test targets with a crate attribute, also check if the underlying crate should be instrumented. Rust compiles the crate sources directly into the test binary, so the test must be built with -Cinstrument-coverage for the crate's code to produce coverage. Add --instrumentation_filter=^// and --instrument_test_targets to CI coverage tasks so all workspace targets (including tests) are instrumented while excluding external dependencies.
Remove --instrument_test_targets from recommended settings since it is not needed for the common case of rust_test with a crate attribute. Document the inconsistency where #[cfg(test)] code gets instrumented even without the flag.
This fixes a problem pointed out by bazelbuild#4092 (comment). `-Zsplit-dwarf-out-dir` is only available on nightly, so we have to gate split debug info on it. Also added some tests that check that we don't add the flag when using other channels.
Scan code-generating rustc outputs for the resolved ${pwd} value after successful compilation. Reject embedded CARGO_MANIFEST_DIR, OUT_DIR, and other sandbox paths while preserving compile-time include_str! and relative paths.
Inspect cargo_build_script OUT_DIR through test runfiles instead of retaining compile-action paths.
Assisted-by: OpenAI Codex
Assisted-by: OpenAI Codex
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd9a6c651a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Assisted-by: OpenAI Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e3b9f08d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Assisted-by: OpenAI Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a7d5db6a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Assisted-by: OpenAI Codex
|
Thanks for the PR! Let me double check on some internal repos but the description of the problem does seem reasonable! It feels like it would be cleaner to expose a way for the hermetic toolchains to avoid the unwindlib flag, perhaps through constraint. Let me think on it a bit |
|
Related issue here hermeticbuild/hermetic-llvm#304 The main problem here is that the only way to get dynamic linker inputs to participate in the RPATH and runfiles is to use the dynamic_runtime_lib feature and nothing else (which means we need to provide those not as driver inferred, but explicitely in the linker arguments, with --unwundlib=none)... The only way for hermetic-llvm to be completely correct would be to have a way to have linker inputs participate in the runfiles and RPATH without using the dynamic_runtime_lib. Meanwhile, maybe we should provide a flag specific for that problem that convey the fact that a conflicting hermetic flag is added later by a third party command (like rustc for --nodefaultlibs). While I'm writing this, I also realize that --unwindlib=none LDFLAGS only make sense if cc_common.link is used, so maybe a flag that says, cc_common_link not used downstream ? See bazelbuild/rules_cc#722 and hermeticbuild/hermetic-llvm#564 which would be enabled by the former, and fix this issue entirely (relying on the driver entirely instead of passing hermetic flags by hand). |
11 participants
Summary
-unwindlib=none/--unwindlib=nonearguments throughrustcwhen rustc disables the linker's default librariesArgsor@response-filecontents leave the effective setting unknownRoot cause
rules_rustasks Bazel's C++ toolchain for its executable-link command line and forwards those arguments torustcas-C link-arg. Hermetic Clang toolchains can include an unwind-library selection in that command line.By default,
rustcinvokes a compiler driver with-nodefaultlibsand supplies the Rust runtime and unwind library explicitly. Clang therefore cannot act on-unwindlib=noneor--unwindlib=noneand correctly reports an unused-command-line warning.Rust 1.97's
linker_messageslint surfaces successful-link stderr as a Rust warning, so the redundant toolchain argument became visible across every Rust binary and test link. See rust-lang/rust#136096 for the linker-message behavior.Conditional behavior
Targets can opt back into compiler-driver defaults with bare
-Cdefault-linker-librariesor an affirmative value such as-Cdefault-linker-libraries=yes. In those modes Clang can use the unwind-library selection, so this change preserves it.Rustc uses the last occurrence of the option. The implementation therefore evaluates statically available flag sources in their real command-line order, including later disabling values such as
=no. OpaqueArgsand@response-filecontents are treated as unknown; the unwind selection is preserved unless a later static option establishes that default linker libraries are disabled.Why remove the arguments conditionally
Adding
-Wno-unused-command-line-argumentwould hide an accurate diagnostic and could conceal other genuinely stale linker-driver options. This change removes only the two accepted spellings of the known-inapplicable unwind option when rustc's effective behavior makes them unusable. All other toolchain arguments remain intact.Verbose-link inspection confirmed that removing these arguments in the default mode does not change the final LLD inputs:
rustcstill links the explicit unwind runtime.Validation
bazelisk test //test/unit/...— 404 tests passed, 24 skipped=nooverrides an earlier=yesand removes both spellings-nodefaultlibs, while both the bare and=yesforms omit it//services/oriel/pid-gen:pid-gen-testin the downstream monorepo using this checkout via--override_repository, with unused-command-line warnings explicitly re-enabled; the target built successfully without the warningmainAI assistance
OpenAI Codex assisted with diagnosis, implementation, and test construction. I reviewed the resulting code and validated it against both the upstream unit suite and the downstream reproducer.