Update `rustc` to `nightly-2022-08-08`

[kkysen]

Supercedes #513.

I kept behavior identical (at least I meant to) during this update, even if it may make more sense to relax a match or avoid an .unwrap() now. We can fix those later if we determine it is correct to do so. These places I marked with TODOs.

I fixed almost all of the errors that came up with upgrading rustc; many were similar/the same to #513, but there are a few left in c2rust-analyze that I was unsure how to handle. Some enums got more variants, and I'm not sure how we should handle those in matches, as that's new behavior. @spernsteiner, if you can help with those, that'd be great!

[spernsteiner]

I pushed fixes for the remaining c2rust-analyze build errors on the update-rustc-analyze-fixes branch.

[kkysen]

Thanks! You could've just pushed directly here, though.

[kkysen]

It seems atomic intrinsics were changed, so we need to update them: c2rust-testsuite CI failure.

Why are we using unstable intrinsics anyways?

[thedataking]

Why are we using unstable intrinsics anyways?

As far as I remember, these correspond to atomic operations on values in the input C code. We'd introduce some pretty horrible bugs in the generated Rust if we didn't.

[fw-immunant]

Looks like it was https://github.com/rust-lang/rust/pull/97423 that broke this. I'll see about a fix.

[kkysen]

It might be from here: https://github.com/immunant/c2rust/blob/df32e6c407db04eb592f83613d800caa075d4b8c/c2rust-transpile/src/translator/atomics.rs#L236-L244

[fw-immunant]

Now we hit an ICE on unsupported cast: *mut libc::c_void to usize...

I think it may be that we need a different CastKind in cast_ptr_to_usize.

[spernsteiner]

I think it may be that we need a different CastKind in cast_ptr_to_usize.

Looks like there's a new CastKind::PointerExposeAddress for this, as part of the strict provenance work

[kkysen]

Yeah, I was thinking it looked related to strict provenance.

Another thing, I think we shouldn't tie the transpiler to the current rustc version. That is, the rust-toolchain.toml that we write in the generated crate shouldn't be automatically the same as ours; it should be updated separately. That way upgrading rustc won't break the transpiler, and we can upgrade that transpiled code rustc version separately.

[kkysen]

Why do we need to cast pointer to usize? Can't we use an opaque pointer and avoid ptr to int casts? Either an extern type (unstable I think but we're using rustc internals anyways) or a pointer to an empty array.

[kkysen]

Per https://github.com/rust-lang/rust/pull/97423,

Suffixes for compare-and-exchange operations with two memory orderings:

Success	Failure	Before	After
Relaxed	Relaxed	_relaxed	_relaxed_relaxed
Relaxed	Acquire	❌	_relaxed_acquire
Relaxed	SeqCst	❌	_relaxed_seqcst
Acquire	Relaxed	_acq_failrelaxed	_acquire_relaxed
Acquire	Acquire	_acq	_acquire_acquire
Acquire	SeqCst	❌	_acquire_seqcst
Release	Relaxed	_rel	_release_relaxed
Release	Acquire	❌	_release_acquire
Release	SeqCst	❌	_release_seqcst
AcqRel	Relaxed	_acqrel_failrelaxed	_acqrel_relaxed
AcqRel	Acquire	_acqrel	_acqrel_acquire
AcqRel	SeqCst	❌	_acqrel_seqcst
SeqCst	Relaxed	_failrelaxed	_seqcst_relaxed
SeqCst	Acquire	_failacq	_seqcst_acquire
SeqCst	SeqCst	(none)	_seqcst_seqcst

all the combinations are now valid, so we should allow them here (are there other places)?: https://github.com/immunant/c2rust/blob/5e5fb311929dde9e248b3df7a5c8e7b75517714c/c2rust-transpile/src/translator/atomics.rs#L206-L215

Should that be done in a subsequent PR? As that wouldn't be necessary to fix code that broke in the update, but would allow more atomics.

[kkysen]

./scripts/test_translator.py tests/ --only-directories builtins is also broken now:

error[E0425]: cannot find function `atomic_xchg_acq` in module `core::intrinsics`
   --> src/atomics.rs:167:33
    |
167 |         ) = ::core::intrinsics::atomic_xchg_acq(&mut x, 33 as libc::c_int);
    |                                 ^^^^^^^^^^^^^^^ help: a function with a similar name exists: `atomic_cxchg_acq`
    |
   ::: /home/kkysen/.rustup/toolchains/nightly-2022-08-08-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/intrinsics.rs:320:5
    |
320 |     pub fn atomic_cxchg_acquire_acquire<T: Copy>(dst: *mut T, old: T, src: T) -> (T, bool);
    |     -------------------------------------------------------------------------------------- similarly named function `atomic_cxchg_acq` defined here

error[E0425]: cannot find function `atomic_store_rel` in module `core::intrinsics`
   --> src/atomics.rs:171:25
    |
171 |     ::core::intrinsics::atomic_store_rel(&mut x, 0);
    |                         ^^^^^^^^^^^^^^^^ help: a function with a similar name exists: `atomic_store`
    |
   ::: /home/kkysen/.rustup/toolchains/nightly-2022-08-08-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/intrinsics.rs:523:5
    |
523 |     pub fn atomic_store_seqcst<T: Copy>(dst: *mut T, val: T);
    |     -------------------------------------------------------- similarly named function `atomic_store` defined here

By the way, these error messages are a bug, right? They're still referring to the old intrinsic names.

[fw-immunant]

./scripts/test_translator.py tests/ --only-directories builtins is also broken now:

Oops, I forgot to save some changes before committing. I'll push a fix.

By the way, these error messages are a bug, right? They're still referring to the old intrinsic names.

They do look wrong, but I think they're caused by the backwards-compatibility aliases which still exist: https://github.com/m-ou-se/rust/blob/4982a59986f7393ace98f63c10e6c435ffba1420/library/core/src/intrinsics.rs#L949-L965

Re: cxchg orderings, I think it's fine to change them in a follow-up PR.

[kkysen]

Ah, that's why the error messages are wrong. I already commented on that PR that they are wrong, though, but I do still think they're very misleading as it says nothing about them being re-exported.

[kkysen]

For cast_ptr_to_usize, do we want the provenance? That is, do we want .addr() or .expose_addr()? And why not keep it an opaque pointer (with provenance)?

[kkysen]

If I just switch CastKind::Misc to CastKind::PointerExposeAddress, I get this error that I'm unsure why is happening:

error: internal compiler error: /rustc/d394408fb38c4de61f765a3ed5189d2731a1da91/compiler/rustc_codegen_ssa/src/mir/operand.rs:120:18: not immediate: OperandRef(Pair(([0 x { [0 x i8]*, i64 }]*:[0 x { [0 x i8]*, i64 }]* bitcast (<{ i8*, [8 x i8] }>* @5 to [0 x { [0 x i8]*, i64 }]*)), (i64:i64 1)) @ TyAndLayout { ty: *const [&str], layout: Layout { size: Size(16 bytes), align: AbiAndPrefAlign { abi: Align(8 bytes), pref: Align(8 bytes) }, abi: ScalarPair(Initialized { value: Pointer, valid_range: 0..=18446744073709551615 }, Initialized { value: Int(I64, false), valid_range: 0..=18446744073709551615 }), fields: Arbitrary { offsets: [Size(0 bytes), Size(8 bytes)], memory_index: [0, 1] }, largest_niche: None, variants: Single { index: 0 } } })

thread 'rustc' panicked at 'Box<dyn Any>', /rustc/d394408fb38c4de61f765a3ed5189d2731a1da91/compiler/rustc_errors/src/lib.rs:1392:9
stack backtrace:
   0: std::panicking::begin_panic::<rustc_errors::ExplicitBug>
   1: std::panic::panic_any::<rustc_errors::ExplicitBug>
   2: <rustc_errors::HandlerInner>::bug::<&alloc::string::String>
   3: <rustc_errors::Handler>::bug::<&alloc::string::String>
   4: rustc_middle::ty::context::tls::with_context_opt::<rustc_middle::ty::context::tls::with_opt<rustc_middle::util::bug::opt_span_bug_fmt<rustc_span::span_encoding::Span>::{closure#0}, ()>::{closure#0}, ()>
   5: rustc_middle::util::bug::opt_span_bug_fmt::<rustc_span::span_encoding::Span>
   6: rustc_middle::util::bug::bug_fmt
   7: rustc_codegen_ssa::mir::codegen_mir::<rustc_codegen_llvm::builder::Builder>
   8: rustc_codegen_llvm::base::compile_codegen_unit::module_codegen
   9: <rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::with_task::<rustc_middle::ty::context::TyCtxt, rustc_span::symbol::Symbol, rustc_codegen_ssa::ModuleCodegen<rustc_codegen_llvm::ModuleLlvm>>
  10: rustc_codegen_llvm::base::compile_codegen_unit
  11: rustc_codegen_ssa::base::codegen_crate::<rustc_codegen_llvm::LlvmCodegenBackend>
  12: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_ssa::traits::backend::CodegenBackend>::codegen_crate
  13: <rustc_session::session::Session>::time::<alloc::boxed::Box<dyn core::any::Any>, rustc_interface::passes::start_codegen::{closure#0}>
  14: <rustc_interface::passes::QueryContext>::enter::<<rustc_interface::queries::Queries>::ongoing_codegen::{closure#0}::{closure#0}, core::result::Result<alloc::boxed::Box<dyn core::any::Any>, rustc_errors::ErrorGuaranteed>>
  15: <rustc_interface::queries::Queries>::ongoing_codegen
  16: <rustc_interface::interface::Compiler>::enter::<rustc_driver::run_compiler::{closure#1}::{closure#2}, core::result::Result<core::option::Option<rustc_interface::queries::Linker>, rustc_errors::ErrorGuaranteed>>
  17: rustc_span::with_source_map::<core::result::Result<(), rustc_errors::ErrorGuaranteed>, rustc_interface::interface::create_compiler_and_run<core::result::Result<(), rustc_errors::ErrorGuaranteed>, rustc_driver::run_compiler::{closure#1}>::{closure#1}>
  18: rustc_interface::interface::create_compiler_and_run::<core::result::Result<(), rustc_errors::ErrorGuaranteed>, rustc_driver::run_compiler::{closure#1}>
  19: <scoped_tls::ScopedKey<rustc_span::SessionGlobals>>::set::<rustc_interface::interface::run_compiler<core::result::Result<(), rustc_errors::ErrorGuaranteed>, rustc_driver::run_compiler::{closure#1}>::{closure#0}, core::result::Result<(), rustc_errors::ErrorGuaranteed>>

Happening here: https://github.com/rust-lang/rust/blob/b998821e4c51c44a9ebee395c91323c374236bbb/compiler/rustc_codegen_ssa/src/mir/operand.rs#L115-L122, but I'm unsure why without building and debugging rustc.

[spernsteiner]

Are we trying to cast &T/&mut T directly to usize, without going through *const T/*mut T first? I don't think this was ever supported, but maybe we were getting away with it under the old nightly.

[kkysen]

I didn't think so, and the comments say we cast references first to pointers.

[fw-immunant]

Based on the error message, it sounds like we're trying to do something illegal with *const [&str] (perhaps casting it to usize?).

[fw-immunant]

Adding this assertion seems to pre-empt the later error:

diff --git a/dynamic_instrumentation/src/point/cast.rs b/dynamic_instrumentation/src/point/cast.rs
index 4b5db5fa..25799dba 100644
--- a/dynamic_instrumentation/src/point/cast.rs
+++ b/dynamic_instrumentation/src/point/cast.rs
@@ -29,6 +29,12 @@ pub fn cast_ptr_to_usize<'tcx>(
 
     let arg_ty = arg.inner().ty(locals, tcx);
 
+    assert!(!arg_ty.is_array_slice(),
+        "{:?}: {:?} is a slice ptr",
+        arg,
+        arg_ty
+    );
+
     let ptr = match arg {
         // If we were given an address as a usize, no conversion is necessary
         InstrumentationArg::Op(ArgKind::AddressUsize(_arg)) => {iff

Is it sufficient for instrumentation to only trace scalar accesses? If so, we can add this assertion and also skip tracing these values when collecting instrumentation points.

[spernsteiner]

It would be nice to keep instrumenting slice pointers, just in case. You could have it cast *const [T] to *const T (discarding the length) and from there cast to usize.

[kkysen]

Do we need to track both originally raw fat pointers and ones that originally were non-raw fat pointers (e.x. slices)?

[kkysen]

I'm not sure why this broke during the update, though, as I would've thought this was an error previously, too.

[kkysen]

I managed to fix the fat ptr to usize cast error in https://github.com/immunant/c2rust/pull/595/commits/906968bffba98ecf95487bec9c6e64d374b176d0 by casting all pointers to *const [(); 0], an opaque pointer. Then this is cast to usize. I think we should use this kind of opaque pointer in the runtime itself and not cast it to a usize at all, but that'd be for another PR.

I haven't updated the snapshot yet as it's quite noisy since changing the MIR length (from PointerExposeAddress instead of Misc) adds a bunch of padding. That's fixed in #610, so I want to merge that one first (it's pretty trivial).

[spernsteiner]

Why *const [(); 0] instead of *const ()? Shouldn't the two be basically equivalent for this purpose?

[kkysen]

Why *const [(); 0] instead of *const ()? Shouldn't the two be basically equivalent for this purpose?

I believe it's because () can't be used in FFI sometimes (maybe just without warnings), so the empty array is preferred instead. Ideally extern types would be used but those are unstable.

[spernsteiner]

() can't be used in FFI sometimes (maybe just without warnings)

Does that apply here? If we're immediately casting to usize, then this pointer type won't appear in any FFI signatures. And even if we push the cast into the runtime library, calls into the runtime are ordinary Rust calls, not FFI calls.

[kkysen]

Does that apply here? If we're immediately casting to usize, then this pointer type won't appear in any FFI signatures. And even if we push the cast into the runtime library, calls into the runtime are ordinary Rust calls, not FFI calls.

It might not, but I wanted to err on the safe side as it doesn't really matter which opaque pointer type we pick. If a bunch of other Rust code uses this, too, I thought why not copy it. I know they used to recommend using a never type (an uninhabitable enum), but there were concerns with that over UB, so now the recommendation is an empty array.

[kkysen]

Works on lighttpd after merging the fixes in #605 from master.