Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doctests in libcore sometimes use wrong external crate #4

Closed
RalfJung opened this issue Jul 16, 2021 · 7 comments · Fixed by #5 or rust-lang/rust#95917
Closed

doctests in libcore sometimes use wrong external crate #4

RalfJung opened this issue Jul 16, 2021 · 7 comments · Fixed by #5 or rust-lang/rust#95917

Comments

@RalfJung
Copy link
Member

Under some circumstances, Miri goes all crazy on libcore doctests: it seems to load two distinct copies of libcore and then hell breaks loose.

Specifically, this seems to be triggered when the code contains a variable that does not exist -- rustc will try to make szggestions for names to use, which can trigger loading external crates, which might be what is happening here.

Cc @hyd-dev

@RalfJung
Copy link
Member Author

RalfJung commented Jul 16, 2021

@hyd-dev wrote

cargo test without Miri fails with same errors in that repository and all other libcore tests passed. It's caused by the miri-test-libstd-specific core_miri_test crate which is effectively another libcore.

Well, but real libcore has something similar, doesn't it? There's libcore and there's "libcore built for tests".

@ghost
Copy link

ghost commented Jul 16, 2021

Well, but real libcore has something similar, doesn't it? There's libcore and there's "libcore built for tests".

This is the command used by x.py test library/core --stage 0 --doc:

/checkout/build/x86_64-unknown-linux-gnu/stage0/bin/rustc --crate-type bin --cfg bootstrap --sysroot /checkout/build/x86_64-unknown-linux-gnu/stage0-sysroot --edition 2018 -o /tmp/rustdoctest1Vq8r8/rust_out -L dependency=/checkout/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/build/x86_64-unknown-linux-gnu/stage0-std/release/deps --extern core=/checkout/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/libcore-2905b2c8ac6b4c16.rlib --extern rand=/checkout/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps/librand-bbbfa0c2da24b204.rlib -Ccodegen-units=1 -C lto=off -C embed-bitcode=no -Z crate-attr=doc(html_root_url="https://doc.rust-lang.org/nightly/") -Z force-unstable-if-unmarked --target x86_64-unknown-linux-gnu --color always -

It looks like it's passing --extern core=... which will override the libcore from sysroot. Actually the sysroot one and the deps one have the same content in this case (I belive one is copied/hardlinked to another), so there is not a seperate "libcore built for tests".
However, the situation is different for core_miri_test. It's built differently than the sysroot core. Even if I rename the crate to core, rustc/Miri thinks libstd (and maybe also other libraries in the sysroot) needs a different version of core (it has a different hash), and still tries to load the core from sysroot.

@RalfJung
Copy link
Member Author

Hm... you are probably right. I think for unit tests the situation is very much like core: those are a separately built crate in both cases. For doctests the situation might well be different. We'd need to make it so that rustdoc/rustc think of the sysroot core crate as being "the crate associated with this doctest" -- like, not have core_miri_test around at all.

Though I am not sure how core_miri_test gets imported at all currently... maybe that is the thing that the search for suggestions is doing, eagerly loading more crates than it otherwise would?

@ghost
Copy link

ghost commented Jul 18, 2021

maybe that is the thing that the search for suggestions is doing, eagerly loading more crates than it otherwise would?

Seems like so. This is the backtrace for the "resolving crate core_miri_test" log message:

   1: tracing_core::dispatcher::get_default
   2: tracing_core::event::Event::dispatch
   3: rustc_metadata::creader::CrateLoader::maybe_resolve_crate
   4: rustc_metadata::creader::CrateLoader::maybe_process_path_extern
   5: rustc_resolve::diagnostics::<impl rustc_resolve::Resolver>::lookup_import_candidates
   6: rustc_resolve::late::diagnostics::<impl rustc_resolve::late::LateResolutionVisitor>::smart_resolve_report_errors
   7: rustc_resolve::late::LateResolutionVisitor::smart_resolve_path_fragment::{{closure}}
   8: rustc_resolve::late::LateResolutionVisitor::smart_resolve_path_fragment
   9: rustc_resolve::late::LateResolutionVisitor::resolve_expr
  10: rustc_ast::visit::walk_expr
  11: rustc_resolve::late::LateResolutionVisitor::resolve_expr
  12: <rustc_resolve::late::LateResolutionVisitor as rustc_ast::visit::Visitor>::visit_block
  13: <rustc_resolve::late::LateResolutionVisitor as rustc_ast::visit::Visitor>::visit_fn
  14: rustc_ast::visit::walk_item
  15: rustc_resolve::late::LateResolutionVisitor::resolve_item
  16: <rustc_resolve::late::LateResolutionVisitor as rustc_ast::visit::Visitor>::visit_block
  17: <rustc_resolve::late::LateResolutionVisitor as rustc_ast::visit::Visitor>::visit_fn
  18: rustc_ast::visit::walk_item
  19: rustc_resolve::late::LateResolutionVisitor::resolve_item
  20: <rustc_resolve::late::LateResolutionVisitor as rustc_ast::visit::Visitor>::visit_block
  21: <rustc_resolve::late::LateResolutionVisitor as rustc_ast::visit::Visitor>::visit_fn
  22: rustc_ast::visit::walk_item
  23: rustc_resolve::late::LateResolutionVisitor::resolve_item
  24: rustc_ast::visit::walk_crate
  25: rustc_resolve::late::<impl rustc_resolve::Resolver>::late_resolve_crate
  26: rustc_session::utils::<impl rustc_session::session::Session>::time
  27: rustc_resolve::Resolver::resolve_crate
  28: rustc_interface::passes::configure_and_expand
  29: rustc_interface::queries::Queries::expansion
  30: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::enter
  31: rustc_span::with_source_map
  32: rustc_interface::interface::create_compiler_and_run
  33: scoped_tls::ScopedKey<T>::set
  34: std::sys_common::backtrace::__rust_begin_short_backtrace
  35: core::ops::function::FnOnce::call_once{{vtable.shim}}
  36: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/b9197978a90be6f7570741eabe2da175fec75375/library/alloc/src/boxed.rs:1572:9
  37: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/b9197978a90be6f7570741eabe2da175fec75375/library/alloc/src/boxed.rs:1572:9
  38: std::sys::unix::thread::Thread::new::thread_start
             at /rustc/b9197978a90be6f7570741eabe2da175fec75375/library/std/src/sys/unix/thread.rs:74:17
  39: start_thread
  40: clone

@RalfJung
Copy link
Member Author

I think I've seen this behavior of rustc suggestions cause similar trouble in the past... but OTOH it kind of has to load all the crates since ´extern crate` is implicit since the 2018 edition.

So probably the only thing we can do is to try and make the core_miri_test crate empty. I assume that is what the patch you showed on Zulip does?

@ghost
Copy link

ghost commented Jul 18, 2021

So probably the only thing we can do is to try and make the core_miri_test crate empty. I assume that is what the patch you showed on Zulip does?

Yes.

@RalfJung
Copy link
Member Author

For the record, that diff was

diff --git a/core_miri_test/Cargo.toml b/core_miri_test/Cargo.toml
index 524cce8..b8df702 100644
--- a/core_miri_test/Cargo.toml
+++ b/core_miri_test/Cargo.toml
@@ -12,6 +12,10 @@ path = "../libcore/src/lib.rs"
 test = false
 bench = false

+[features]
+miri-test = []
+default = ["miri-test"]
+
 [[test]]
 name = "coretests"
 path = "../libcore/tests/lib.rs"
diff --git a/rust-src.diff b/rust-src.diff
index e69de29..965f559 100644
--- a/rust-src.diff
+++ b/rust-src.diff
@@ -0,0 +1,12 @@
+diff --git a/library/core/src/lib.rs b/library/core/src/lib.rs
+index 3557dbad..6c089b0c 100644
+--- a/library/core/src/lib.rs
++++ b/library/core/src/lib.rs
+@@ -49,6 +49,7 @@
+ //
+ // This cfg won't affect doc tests.
+ #![cfg(not(test))]
++#![cfg(any(not(feature = "miri-test"), doctest))]
+ #![stable(feature = "core", since = "1.6.0")]
+ #![doc(
+     html_playground_url = "https://play.rust-lang.org/",

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 23, 2021
…Simulacrum

better support for running libcore tests with Miri

See rust-lang/miri-test-libstd#4 for a description of the problem that this fixes.
Thanks to `@hyd-dev` for suggesting this patch!
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 11, 2022
thin_box test: import from std, not alloc

Importing from `alloc` makes [Miri fail](https://github.com/rust-lang/miri-test-libstd/runs/5964922742?check_suite_focus=true), probably due to the hack that we used to resolve rust-lang/miri-test-libstd#4. There might be better ways around this, but for now this is the easiest thing to do -- no other alloc integration test is importing from `alloc::`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 11, 2022
thin_box test: import from std, not alloc

Importing from `alloc` makes [Miri fail](https://github.com/rust-lang/miri-test-libstd/runs/5964922742?check_suite_focus=true), probably due to the hack that we used to resolve rust-lang/miri-test-libstd#4. There might be better ways around this, but for now this is the easiest thing to do -- no other alloc integration test is importing from `alloc::`.
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
thin_box test: import from std, not alloc

Importing from `alloc` makes [Miri fail](https://github.com/rust-lang/miri-test-libstd/runs/5964922742?check_suite_focus=true), probably due to the hack that we used to resolve rust-lang/miri-test-libstd#4. There might be better ways around this, but for now this is the easiest thing to do -- no other alloc integration test is importing from `alloc::`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant