Skip to content

Commit

Permalink
Rollup merge of rust-lang#85633 - lqd:stackless_span_stacks, r=oli-obk
Browse files Browse the repository at this point in the history
Post-monomorphization errors traces MVP

This PR works towards better diagnostics for the errors encountered in rust-lang#85155 and similar.

We can encounter post-monomorphization errors (PMEs) when collecting mono items. The current diagnostics are confusing for these cases when they happen in a dependency (but are acceptable when they happen in the local crate).

These kinds of errors will be more likely now that `stdarch` uses const generics for its intrinsics' immediate arguments, and validates these const arguments with a mechanism that triggers such PMEs.

(Not to mention that the errors happen during codegen, so only when building code that actually uses these code paths. Check builds don't trigger them, neither does unused code)

So in this PR, we detect these kinds of errors during the mono item graph walk: if any error happens while collecting a node or its neighbors, we print a diagnostic about the current collection step, so that the user has at least some context of which erroneous code and dependency triggered the error.

The diagnostics for issue rust-lang#85155 now have this note showing the source of the erroneous const argument:
```
note: the above error was encountered while instantiating `fn std::arch::x86_64::_mm_blend_ps::<51_i32>`
  --> issue-85155.rs:11:24
   |
11 |         let _blended = _mm_blend_ps(a, b, 0x33);
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
```

Note that rust-lang#85155 is a reduced version of a case happening in the wild, to indirect users of the `rustfft` crate, as seen in ejmahler/RustFFT#74. The crate had a few of these out-of-range immediates. Here's how the diagnostics in this PR would have looked on one of its examples before it was fixed:

<details>

```
error[E0080]: evaluation of constant value failed
 --> ./stdarch/crates/core_arch/src/macros.rs:8:9
  |
8 |         assert!(IMM >= MIN && IMM <= MAX, "IMM value not in expected range");
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'IMM value not in expected range', ./stdarch/crates/core_arch/src/macros.rs:8:9
  |
  = note: this error originates in the macro `$crate::panic::panic_2015` (in Nightly builds, run with -Z macro-backtrace for more info)

note: the above error was encountered while instantiating `fn _mm_blend_ps::<51_i32>`
    --> /tmp/RustFFT/src/avx/avx_vector.rs:1314:23
     |
1314 |         let blended = _mm_blend_ps(rows[0], rows[2], 0x33);
     |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: the above error was encountered while instantiating `fn _mm_permute_pd::<5_i32>`
    --> /tmp/RustFFT/src/avx/avx_vector.rs:1859:9
     |
1859 |         _mm_permute_pd(self, 0x05)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^

note: the above error was encountered while instantiating `fn _mm_permute_pd::<15_i32>`
    --> /tmp/RustFFT/src/avx/avx_vector.rs:1863:32
     |
1863 |         (_mm_movedup_pd(self), _mm_permute_pd(self, 0x0F))
     |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.
error: could not compile `rustfft`

To learn more, run the command again with --verbose.
```

</details>

I've developed and discussed this with them, so maybe r? `@oli-obk` -- but feel free to redirect to someone else of course.

(I'm not sure we can say that this PR definitely closes issue 85155, as it's still unclear exactly which diagnostics and information would be interesting to report in such cases -- and we've discussed printing backtraces before. I have prototypes of some complete and therefore noisy backtraces I showed Oli, but we decided to not include them in this PR for now)
  • Loading branch information
Dylan-DPC authored May 26, 2021
2 parents f5c5cca + 6f61456 commit 4b0014e
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 2 deletions.
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/mir/mono.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,15 @@ impl<'tcx> MonoItem<'tcx> {
pub fn codegen_dep_node(&self, tcx: TyCtxt<'tcx>) -> DepNode {
crate::dep_graph::make_compile_mono_item(tcx, self)
}

/// Returns the item's `CrateNum`
pub fn krate(&self) -> CrateNum {
match self {
MonoItem::Fn(ref instance) => instance.def_id().krate,
MonoItem::Static(def_id) => def_id.krate,
MonoItem::GlobalAsm(..) => LOCAL_CRATE,
}
}
}

impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for MonoItem<'tcx> {
Expand Down
46 changes: 44 additions & 2 deletions compiler/rustc_mir/src/monomorphize/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::sync::{par_iter, MTLock, MTRef, ParallelIterator};
use rustc_errors::{ErrorReported, FatalError};
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId};
use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId, LOCAL_CRATE};
use rustc_hir::itemlikevisit::ItemLikeVisitor;
use rustc_hir::lang_items::LangItem;
use rustc_index::bit_set::GrowableBitSet;
Expand Down Expand Up @@ -342,7 +342,8 @@ fn collect_roots(tcx: TyCtxt<'_>, mode: MonoItemCollectionMode) -> Vec<MonoItem<
.collect()
}

// Collect all monomorphized items reachable from `starting_point`
/// Collect all monomorphized items reachable from `starting_point`, and emit a note diagnostic if a
/// post-monorphization error is encountered during a collection step.
fn collect_items_rec<'tcx>(
tcx: TyCtxt<'tcx>,
starting_point: Spanned<MonoItem<'tcx>>,
Expand All @@ -359,6 +360,31 @@ fn collect_items_rec<'tcx>(
let mut neighbors = Vec::new();
let recursion_depth_reset;

//
// Post-monomorphization errors MVP
//
// We can encounter errors while monomorphizing an item, but we don't have a good way of
// showing a complete stack of spans ultimately leading to collecting the erroneous one yet.
// (It's also currently unclear exactly which diagnostics and information would be interesting
// to report in such cases)
//
// This leads to suboptimal error reporting: a post-monomorphization error (PME) will be
// shown with just a spanned piece of code causing the error, without information on where
// it was called from. This is especially obscure if the erroneous mono item is in a
// dependency. See for example issue #85155, where, before minimization, a PME happened two
// crates downstream from libcore's stdarch, without a way to know which dependency was the
// cause.
//
// If such an error occurs in the current crate, its span will be enough to locate the
// source. If the cause is in another crate, the goal here is to quickly locate which mono
// item in the current crate is ultimately responsible for causing the error.
//
// To give at least _some_ context to the user: while collecting mono items, we check the
// error count. If it has changed, a PME occurred, and we trigger some diagnostics about the
// current step of mono items collection.
//
let error_count = tcx.sess.diagnostic().err_count();

match starting_point.node {
MonoItem::Static(def_id) => {
let instance = Instance::mono(tcx, def_id);
Expand Down Expand Up @@ -411,6 +437,22 @@ fn collect_items_rec<'tcx>(
}
}

// Check for PMEs and emit a diagnostic if one happened. To try to show relevant edges of the
// mono item graph where the PME diagnostics are currently the most problematic (e.g. ones
// involving a dependency, and the lack of context is confusing) in this MVP, we focus on
// diagnostics on edges crossing a crate boundary: the collected mono items which are not
// defined in the local crate.
if tcx.sess.diagnostic().err_count() > error_count && starting_point.node.krate() != LOCAL_CRATE
{
tcx.sess.span_note_without_error(
starting_point.span,
&format!(
"the above error was encountered while instantiating `{}`",
starting_point.node
),
);
}

record_accesses(tcx, starting_point.node, neighbors.iter().map(|i| &i.node), inlining_map);

for neighbour in neighbors {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Auxiliary crate used for testing post-monomorphization errors cross-crate.
// It duplicates the setup used in `stdarch` to validate its intrinsics' const arguments.

struct ValidateConstImm<const IMM: i32, const MIN: i32, const MAX: i32>;
impl<const IMM: i32, const MIN: i32, const MAX: i32> ValidateConstImm<IMM, MIN, MAX> {
pub(crate) const VALID: () = {
let _ = 1 / ((IMM >= MIN && IMM <= MAX) as usize);
};
}

macro_rules! static_assert_imm1 {
($imm:ident) => {
let _ = $crate::ValidateConstImm::<$imm, 0, { (1 << 1) - 1 }>::VALID;
};
}

// This function triggers an error whenever the const argument does not fit in 1-bit.
pub fn stdarch_intrinsic<const IMM1: i32>() {
static_assert_imm1!(IMM1);
}
21 changes: 21 additions & 0 deletions src/test/ui/consts/const-eval/issue-85155.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// This is a test with a setup similar to issue 85155, which triggers a const eval error: a const
// argument value is outside the range expected by the `stdarch` intrinsic.
//
// It's not the exact code mentioned in that issue because it depends both on `stdarch` intrinsics
// only available on x64, and internal implementation details of `stdarch`. But mostly because these
// are not important to trigger the diagnostics issue: it's specifically about the lack of context
// in the diagnostics of post-monomorphization errors (PMEs) for consts, happening in a dependency.
// Therefore, its setup is reproduced with an aux crate, which will similarly trigger a PME
// depending on the const argument value, like the `stdarch` intrinsics would.
//
// aux-build: post_monomorphization_error.rs
// build-fail: this is a post-monomorphization error, it passes check runs and requires building
// to actually fail.

extern crate post_monomorphization_error;

fn main() {
// This function triggers a PME whenever the const argument does not fit in 1-bit.
post_monomorphization_error::stdarch_intrinsic::<2>();
//~^ NOTE the above error was encountered while instantiating
}
15 changes: 15 additions & 0 deletions src/test/ui/consts/const-eval/issue-85155.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error[E0080]: evaluation of constant value failed
--> $DIR/auxiliary/post_monomorphization_error.rs:7:17
|
LL | let _ = 1 / ((IMM >= MIN && IMM <= MAX) as usize);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ attempt to divide `1_usize` by zero

note: the above error was encountered while instantiating `fn stdarch_intrinsic::<2_i32>`
--> $DIR/issue-85155.rs:19:5
|
LL | post_monomorphization_error::stdarch_intrinsic::<2>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.

0 comments on commit 4b0014e

Please sign in to comment.