Repository navigation
Don't ignore unknown LLVM diagnostics - #164578
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.
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 42df3ea failed: CI. Failed job:
|
This preserves the existing behavior. Enabling these warnings is tracked in a new issue.
Apparently this depends on where the tests are run from?
Don't ignore unknown LLVM diagnostics try-job: test-aarch64-msvc-1
This comment has been minimized.
This comment has been minimized.
|
@nikic Are you looking for another approval? I've looked over the code and it seems good to me. |
|
Let's see if fourth time's the charm... @bors r=mati865 |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing eec3f1e (parent) -> f90a2d2 (this PR) Test differencesShow 8 test diffsStage 1
Stage 2
Additionally, 4 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard f90a2d27c2a1e18f10f4fd84c0dcd02710c37de8 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (f90a2d2): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (primary -2.2%, secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 488.328s -> 486.771s (-0.32%) |
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.