Repository navigation
Conversation
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I went through the rabbit hole and the warning for PGO was added as the initial PGO support 8a4cebd (#48346) with the reasoning that should be at least a warning.
Then, #52474 built on top of it adding warnings for previously ignored LTO errors.
I've kept this for now as we still error down the line and this ends up providing a bit more information (which file the duplicate symbol originates from).
I agree that seeing where the error comes from is potentially useful for non-mangled symbols, but currently this seems to depend entirely on the next stage to emit an error upon encountering bad inputs. I imagine (uneducated guess) there might be cases where we print LLVM's error as a warning, but the next stage emits just a warning or is silent.
r=me with sentence above and comments addressed or discarded.
| dcx.emit_warn(FromLlvmDiag { message }); | ||
| } | ||
| llvm::diagnostic::Unsupported(diagnostic_ref) => { | ||
| llvm::diagnostic::PGO(diagnostic_ref) |
There was a problem hiding this comment.
Adding PGO test akin to lto-duplicate-symbol shouldn't be hard, no? Could be a good E-easy issue.
| llvm::diagnostic::Unsupported(diagnostic_ref) => { | ||
| llvm::diagnostic::PGO(diagnostic_ref) | ||
| | llvm::diagnostic::Unsupported(diagnostic_ref) | ||
| | llvm::diagnostic::UnknownDiagnostic(diagnostic_ref) => { |
There was a problem hiding this comment.
Just asking because I'm not familiar with how we want to handle LLVM errors.
Would it be useful to split UnknownDiagnostic variant into LLVM's unknown errors and "LLVM errors that we didn't map yet", so we can indicate the not mapped ones here?
Acting only when we hit a not yet mapped error that needs special handling seems fine too.
There was a problem hiding this comment.
TBH I think we should rather go the other way around and remove any diagnostic kinds that we don't specially handle, i.e. drop PGO and Unsupported and use UnknownDiagnostic for everything. Apart from a few special cases (inline asm and optimization remarks), Rust mostly shouldn't care about where the diagnostic originates and just forward it.
|
I've pushed two additional changes here. One to remove the |
|
Sounds good, thanks! @bors r+ |
Don't ignore unknown LLVM diagnostics Rust currently ignores LLVM diagnostic kinds that it does not know about. This means that we sometimes ignore error diagnostics, which can then result in silent miscompiles. This exposes `LLVMGetDiagInfoSeverity` returning `DiagnosticSeverity`, renamed from `DiagnosticLevel` to match LLVM terminology. Custom code to fetch the diagnostic severity for inline asm diagnostics can then be dropped. For unknown diagnostics, we emit a warning/error depending on severity. Notes and remarks continue to be ignored. One oddity I ran into along the way is that the linker diagnostic has severity error, but we only report a warning. I've kept this for now as we still error down the line and this ends up providing a bit more information (which file the duplicate symbol originates from). Fixes rust-lang#146393.
…uwer Rollup of 6 pull requests Successful merges: - #163104 ([debuginfo] Emit `usize` and `isize` as typedefs) - #164488 (Don't try to skip anything in `Skip::advance_by(0)`) - #164578 (Don't ignore unknown LLVM diagnostics) - #164611 (Improve bit permutation codegen) - #162668 (document that `!` does bitwise and logical negation) - #164264 (lint: suggest `fXX::is_nan` for unsuffixed NaN comparisons)
Don't ignore unknown LLVM diagnostics Rust currently ignores LLVM diagnostic kinds that it does not know about. This means that we sometimes ignore error diagnostics, which can then result in silent miscompiles. This exposes `LLVMGetDiagInfoSeverity` returning `DiagnosticSeverity`, renamed from `DiagnosticLevel` to match LLVM terminology. Custom code to fetch the diagnostic severity for inline asm diagnostics can then be dropped. For unknown diagnostics, we emit a warning/error depending on severity. Notes and remarks continue to be ignored. One oddity I ran into along the way is that the linker diagnostic has severity error, but we only report a warning. I've kept this for now as we still error down the line and this ends up providing a bit more information (which file the duplicate symbol originates from). Fixes rust-lang#146393.
…uwer Rollup of 14 pull requests Successful merges: - #163575 (core: Add examples and assembly tests for `abort_immediate`) - #163664 (Add new `RUSTC_BOOTSTRAP=-allow-unstable-compiler-flags` mode for testing stable compiler behavior in UI tests) - #163104 ([debuginfo] Emit `usize` and `isize` as typedefs) - #163338 (MIR move elimination [4/6]: MoveElimination) - #163701 (add ReadDir::dir to get the directory handle of the underlying directory) - #164488 (Don't try to skip anything in `Skip::advance_by(0)`) - #164578 (Don't ignore unknown LLVM diagnostics) - #164611 (Improve bit permutation codegen) - #164612 (std: Combine `is_powerpc64_feature_detected` into `is_powerpc_feature_detected`) - #162668 (document that `!` does bitwise and logical negation) - #164264 (lint: suggest `fXX::is_nan` for unsuffixed NaN comparisons) - #164509 (layout invariant: turn delayed bug back into proper assertion) - #164594 (rustdoc: Correctly handle inner `deprecated` attribute on modules) - #164629 (arch::breakpoint: avoid confusing 'equivalent' terminology)
Don't ignore unknown LLVM diagnostics Rust currently ignores LLVM diagnostic kinds that it does not know about. This means that we sometimes ignore error diagnostics, which can then result in silent miscompiles. This exposes `LLVMGetDiagInfoSeverity` returning `DiagnosticSeverity`, renamed from `DiagnosticLevel` to match LLVM terminology. Custom code to fetch the diagnostic severity for inline asm diagnostics can then be dropped. For unknown diagnostics, we emit a warning/error depending on severity. Notes and remarks continue to be ignored. One oddity I ran into along the way is that the linker diagnostic has severity error, but we only report a warning. I've kept this for now as we still error down the line and this ends up providing a bit more information (which file the duplicate symbol originates from). Fixes rust-lang#146393.
…uwer Rollup of 13 pull requests Successful merges: - #163664 (Add new `RUSTC_BOOTSTRAP=-allow-unstable-compiler-flags` mode for testing stable compiler behavior in UI tests) - #163104 ([debuginfo] Emit `usize` and `isize` as typedefs) - #163338 (MIR move elimination [4/6]: MoveElimination) - #163701 (add ReadDir::dir to get the directory handle of the underlying directory) - #164488 (Don't try to skip anything in `Skip::advance_by(0)`) - #164578 (Don't ignore unknown LLVM diagnostics) - #164611 (Improve bit permutation codegen) - #164612 (std: Combine `is_powerpc64_feature_detected` into `is_powerpc_feature_detected`) - #162668 (document that `!` does bitwise and logical negation) - #164264 (lint: suggest `fXX::is_nan` for unsuffixed NaN comparisons) - #164509 (layout invariant: turn delayed bug back into proper assertion) - #164594 (rustdoc: Correctly handle inner `deprecated` attribute on modules) - #164629 (arch::breakpoint: avoid confusing 'equivalent' terminology)
…uwer Rollup of 13 pull requests Successful merges: - #163664 (Add new `RUSTC_BOOTSTRAP=-allow-unstable-compiler-flags` mode for testing stable compiler behavior in UI tests) - #163104 ([debuginfo] Emit `usize` and `isize` as typedefs) - #163338 (MIR move elimination [4/6]: MoveElimination) - #163701 (add ReadDir::dir to get the directory handle of the underlying directory) - #164488 (Don't try to skip anything in `Skip::advance_by(0)`) - #164578 (Don't ignore unknown LLVM diagnostics) - #164611 (Improve bit permutation codegen) - #164612 (std: Combine `is_powerpc64_feature_detected` into `is_powerpc_feature_detected`) - #162668 (document that `!` does bitwise and logical negation) - #164264 (lint: suggest `fXX::is_nan` for unsuffixed NaN comparisons) - #164509 (layout invariant: turn delayed bug back into proper assertion) - #164594 (rustdoc: Correctly handle inner `deprecated` attribute on modules) - #164629 (arch::breakpoint: avoid confusing 'equivalent' terminology)
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #164634 (comment) @bors try jobs=test-x86_64-gnu-nopt |
|
This pull request was unapproved. This PR was contained in a rollup (#164634), which was unapproved. |
This comment has been minimized.
This comment has been minimized.
…uwer Rollup of 8 pull requests Successful merges: - #162460 (Feat - `#[inline(always)] + #[target_feature(enable = "....")]` #2) - #163485 (`rustc_builtin_macros` cleanup, part 8) - #164578 (Don't ignore unknown LLVM diagnostics) - #162369 (Document the `recursion_limit` built-in attribute) - #162840 (tier 3 target policy: target CPU and features must be supported by upstream backend) - #164330 (Skip redundant monomorphization checks) - #164656 (stabilize ExitCodeExt for Windows) - #164660 (Touch up attribute docs)
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #164687 (comment) |
|
@bors rollup=iffy |
Don't ignore unknown LLVM diagnostics Rust currently ignores LLVM diagnostic kinds that it does not know about. This means that we sometimes ignore error diagnostics, which can then result in silent miscompiles. This exposes `LLVMGetDiagInfoSeverity` returning `DiagnosticSeverity`, renamed from `DiagnosticLevel` to match LLVM terminology. Custom code to fetch the diagnostic severity for inline asm diagnostics can then be dropped. For unknown diagnostics, we emit a warning/error depending on severity. Notes and remarks continue to be ignored. One oddity I ran into along the way is that the linker diagnostic has severity error, but we only report a warning. I've kept this for now as we still error down the line and this ends up providing a bit more information (which file the duplicate symbol originates from). Fixes rust-lang#146393.
|
This pull request was unapproved. This PR was contained in a rollup (#164697), which was unapproved. |
|
Let's just avoid rolling up this PR, I don't know how many errors are there to uncover. |
|
@bors try jobs=test-x86_64-gnu-debug |
This comment has been minimized.
This comment has been minimized.
Don't ignore unknown LLVM diagnostics try-job: test-x86_64-gnu-debug
|
💔 Test for e83a3e0 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
Rust currently ignores LLVM diagnostic kinds that it does not know about. This means that we sometimes ignore error diagnostics, which can then result in silent miscompiles. This exposes `LLVMGetDiagInfoSeverity` returning `DiagnosticSeverity`, renamed from `DiagnosticLevel` to match LLVM terminology. Custom code to fetch the diagnostic severity for inline asm diagnostics can then be dropped. For unknown diagnostics, we emit a warning/error depending on severity. Notes and remarks continue to be ignored. One oddity I ran into along the way is that the linker diagnostic has severity error, but we only report a warning. I've kept this for now as we still error down the line and this ends up providing a bit more information (which file the duplicate symbol originates from).
Merge the PGO, Unsupported into UnknownDiagnostic and rename it to Other. We don't do anything special for them, so there is no need to represent them differently.
This preserves the existing behavior. Enabling these warnings is tracked in a new issue.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The error message is different depending on whether there is debuginfo. I've forced As a side note, in the cases where LLVM diagnostics have a DiagnosticLocation, we should probably map that to a Rust diagnostic location as well, instead of letting LLVM print this as part of the error message... @bors try jobs=test-x86_64-gnu-debug |
This comment has been minimized.
This comment has been minimized.
Don't ignore unknown LLVM diagnostics try-job: test-x86_64-gnu-debug
This comment has been minimized.
This comment has been minimized.
Apparently this depends on where the tests are run from?
|
💔 Test for f1ec490 failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Turns out that the path depends on which directory the tests are run from. Trying again with normalization... @bors try jobs=test-x86_64-gnu-debug |
|
⌛ Trying commit 3e4d3ee with merge 326b86a… To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/39762784913 |
Don't ignore unknown LLVM diagnostics try-job: test-x86_64-gnu-debug
View all comments
Rust currently ignores LLVM diagnostic kinds that it does not know about. This means that we sometimes ignore error diagnostics, which can then result in silent miscompiles.
This exposes
LLVMGetDiagInfoSeverityreturningDiagnosticSeverity, renamed fromDiagnosticLevelto match LLVM terminology. Custom code to fetch the diagnostic severity for inline asm diagnostics can then be dropped.For unknown diagnostics, we emit a warning/error depending on severity. Notes and remarks continue to be ignored.
One oddity I ran into along the way is that the linker diagnostic has severity error, but we only report a warning. I've kept this for now as we still error down the line and this ends up providing a bit more information (which file the duplicate symbol originates from).
Fixes #146393.