show linker output even if the linker succeeds #119286
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
A-query-system
|
|
jyn514
force-pushed
the
linker-output
branch
2 times, most recently
from
060a457
to
a34d7ae
Compare
This comment has been minimized.
This comment has been minimized.
jyn514
force-pushed
the
linker-output
branch
2 times, most recently
from
64cb47a
to
ab6fb2b
Compare
|
also i know we settled on not showing stdout by default but it's useful for debugging search paths - maybe we should show it after all? it seems silly to have to pass both |
This comment has been minimized.
This comment has been minimized.
jyn514
force-pushed
the
linker-output
branch
from
ab6fb2b
to
ba4425d
Compare
Noratrieb
reviewed
Dec 25, 2023
bjorn3
reviewed
Dec 25, 2023
| @@ -714,7 +714,7 @@ fn test_unstable_options_tracking_hash() { | |||
| untracked!(unpretty, Some("expanded".to_string())); | |||
| untracked!(unstable_options, true); | |||
| untracked!(validate_mir, true); | |||
| untracked!(verbose, true); | |||
| untracked!(verbose_internals, true); | |||
There was a problem hiding this comment.
Don't diagnostics influenced by this flag end up in the incr cache? It should actually be tracked I think.
There was a problem hiding this comment.
hmm, do we cache diagnostic output? even on errors? that seems very strange to me.
i don't mind changing it to tracked, i'm just confused why it's necessary.
There was a problem hiding this comment.
On errors we don't finalize the incr comp cache, but non-fatal diagnostics are cached afaik. Otherwise we would have to replay the query in question, which we may not be able to do if the query key is not reconstructible from the dep node fingerprint.
bjorn3
reviewed
Dec 25, 2023
bjorn3
reviewed
Dec 25, 2023
jyn514
force-pushed
the
linker-output
branch
from
ba4425d
to
43edbe0
Compare
|
☔ The latest upstream changes (presumably #119662) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
|
hmm. that test has the following comment: rust/tests/incremental/commandline-args.rs Lines 13 to 20 in e9a009f i suppose those lines are all invalid now, it should be testing that the CGU is not reused? i'm not sure why it's testing --verbose specifically, maybe it's enough to test any untracked CLI arg. i'll change it to --diagnostic-width.
i'm realizing your comments are all about #119129. i feel slightly uncomfortable making unrelated changes in this PR ... how do you feel about splitting them into a separate PR so i can assign michaelwoerister? |
Sure |
I prefer not doing this. It is helpful whenever a linkage issue is opened to not have to ask to rerun with |
|
ok someone let me use their mac and i managed to fix the bug (the thing i needed was the original |
jyn514
force-pushed
the
linker-output
branch
from
8dca54d
to
0a65924
Compare
|
Some changes occurred in src/tools/cargo cc @ehuss |
jyn514
force-pushed
the
linker-output
branch
from
0a65924
to
217a531
Compare
This comment has been minimized.
This comment has been minimized.
jyn514
force-pushed
the
linker-output
branch
from
217a531
to
e7d328f
Compare
This comment has been minimized.
This comment has been minimized.
|
ok new plan. i don't think fixing these one-by-one is really feasible, there are just too many and they are too complicated to debug. instead i'm going to change this hard-warning to a lint and allow it by default in rustc's CI. that also has the advantage that downstream projects can opt-out of these warnings if they like. |
|
☔ The latest upstream changes (presumably #122709) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
This comment has been minimized.
This comment has been minimized.
mati865
reviewed
Oct 28, 2024
compiler/rustc_passes/messages.ftl
Outdated
| @@ -770,6 +770,9 @@ passes_unused_duplicate = | |||
| passes_unused_empty_lints_note = | |||
| attribute `{$name}` with an empty list has no effect | |||
|
|
|||
| passes_unused_linker_warnings_note = | |||
| the `linker_warnings` lint can only controlled at the root of a crate with `--crate-type bin` | |||
This comment has been minimized.
This comment has been minimized.
the linker arguments can be *very* long, especially for crates with many dependencies. often they are not useful. omit them unless the user specifically requests them.
this avoids warnings from macOS ld
this makes it much easier to understand test failures.
before:
```
diff of stderr:
1 error: linking with `LINKER` failed: exit status: 1
2 |
- ld: Undefined symbols:
4 _CFRunLoopGetTypeID, referenced from:
5 clang: error: linker command failed with exit code 1 (use -v to see invocation)
```
after:
```
=== HAYSTACK ===
error: linking with `cc` failed: exit status: 1
|
= note: use `--verbose` to show all linker arguments
= note: Undefined symbols for architecture arm64:
"_CFRunLoopGetTypeID", referenced from:
main::main::hbb553f5dda62d3ea in main.main.d17f5fbe6225cf88-cgu.0.rcgu.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
error: aborting due to 1 previous error
=== NEEDLE ===
_CFRunLoopGetTypeID\.?, referenced from:
thread 'main' panicked at /Users/jyn/git/rust-lang/rust/tests/run-make/linkage-attr-framework/rmake.rs:22:10:
needle was not found in haystack
```
this also fixes a failure related to missing whitespace; we don't actually care about whitespace in this test.
this was slightly complicated because codegen_ssa doesn't have access to a tcx.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 28, 2024
fix various linker warnings separated out from rust-lang#119286; this doesn't have anything user-facing, i just want to land these changes so i can stop rebasing them. r? `@bjorn3`
|
☔ The latest upstream changes (presumably #132317) made this pull request unmergeable. Please resolve the merge conflicts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
--verboseis passed--verboseis passedfixes #83436. fixes #38206. fixes #109979. helps with #46998. cc https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/uplift.20some.20-Zverbose.20calls.20and.20rename.20to.E2.80.A6.20compiler-team.23706/near/408986134
this is based on #119129 for convenience so i didn't have to duplicate the changes around saving
--verbosein cb6d033#diff-7a49efa20548d6806dbe1c66dd4dc445fda18fcbbf1709520cadecc4841aae12r? @bjorn3