Miri: basic dyn* support #107728

RalfJung · 2023-02-06T15:10:20Z

As usual I am very unsure about the dynamic dispatch stuff, but it passes even the Pin<&mut dyn* Trait> test so that is something.

TBH I think it was a mistake to make dyn Trait and dyn* Trait part of the same TyKind variant. Almost everywhere in Miri this lead to the wrong default behavior, resulting in strange ICEs instead of nice "unimplemented" messages. The two types describe pretty different runtime data layout after all.

Strangely I did not need to do the equivalent of this diff in Miri. Maybe that is because the unsizing logic matches on ty::Dynamic(.., ty::Dyn) already? In unsized_info I don't think the target_dyn_kind can be DynStar, since then it wouldn't be unsized!

r? @oli-obk Cc @eholk (dyn-star) #102425

rustbot · 2023-02-06T15:10:28Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2023-02-06T15:14:30Z

Strangely I did not need to do the equivalent of #106532 (comment) in Miri. Maybe that is because the unsizing logic matches on ty::Dynamic(.., ty::Dyn) already? In unsized_info I don't think the target_dyn_kind can be DynStar, since then it wouldn't be unsized!

Indeed I confirmed if I weaken the match in cast.rs to ignore Dyn vs DynStar, I get a strange ICE. Yet another argument for why those shouldn't be conflated into a single TyKind.

RalfJung · 2023-02-06T15:34:16Z

compiler/rustc_ty_utils/src/layout.rs

@@ -193,7 +193,7 @@ fn layout_of_uncached<'tcx>(
        }

        ty::Dynamic(_, _, ty::DynStar) => {
-            let mut data = scalar_unit(Int(dl.ptr_sized_integer(), false));
+            let mut data = scalar_unit(Pointer(AddressSpace::DATA));


This seems to cause issues.

Invalid InsertValueInst operands! %15 = insertvalue { ptr, ptr } undef, i64 %14, 0 in function _ZN3box13make_dyn_star17h0c15bb343f1925bdE LLVM ERROR: Broken function found, compilation aborted!

Probably somewhere in the backend something still assumes an integer type here, and inserts an i64 into this { ptr, ptr }?

RalfJung · 2023-02-06T15:40:22Z

compiler/rustc_ty_utils/src/layout.rs

@@ -193,7 +193,7 @@ fn layout_of_uncached<'tcx>(
        }

        ty::Dynamic(_, _, ty::DynStar) => {
-            let mut data = scalar_unit(Int(dl.ptr_sized_integer(), false));
+            let mut data = scalar_unit(Pointer(AddressSpace::DATA));


This seems to cause issues.

---- [ui] tests/ui/dyn-star/box.rs stdout ---- error: test compilation failed although it shouldn't! status: exit status: 101 command: "/home/r/src/rust/rustc/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "/home/r/src/rust/rustc/tests/ui/dyn-star/box.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Zdeduplicate-diagnostics=no" "-Cstrip=debuginfo" "--remap-path-prefix=/home/r/src/rust/rustc/tests/ui=fake-test-src-base" "-C" "prefer-dynamic" "-o" "/home/r/src/rust/rustc/build/x86_64-unknown-linux-gnu/test/ui/dyn-star/box/a" "-Crpath" "-Cdebuginfo=0" "-Lnative=/home/r/src/rust/rustc/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/home/r/src/rust/rustc/build/x86_64-unknown-linux-gnu/test/ui/dyn-star/box/auxiliary" "-C" "opt-level=0" stdout: none --- stderr ------------------------------- Invalid InsertValueInst operands! %15 = insertvalue { ptr, ptr } undef, i64 %14, 0 in function _ZN3box13make_dyn_star17h0c15bb343f1925bdE LLVM ERROR: Broken function found, compilation aborted!

Probably somewhere in the backend something still assumes an integer type here, and inserts an i64 into this { ptr, ptr }? This actually casts a Box to dyn*, so the input value is a pointer -- no idea where the i64 comes from.

Looking at the LLVM IR on current nightly

%10 = ptrtoint ptr %_4.i to i64, !dbg !1309 %11 = insertvalue { i64, ptr } undef, i64 %10, 0, !dbg !1310 %12 = insertvalue { i64, ptr } %11, ptr @vtable.1, 1, !dbg !1310

We are generating a ptrtoint?!? That doesn't sound right at all. We shouldn't be generating such a cursed operation unless the user actually asked for it.

Oh, I just found #102641...

We should be doing the opposite: if the input is an integer, we should cast/transmute/bit/cast/whatever it to a pointer.

Strangely just removing that pointer type hack seems to work? I would have expected errors from where we cast usize to dyn* (I think we are now inserting an i64 where a ptr should go) but strangely, there are no such errors...

Ah, on CI it does fail. CI uses an older, pre-ptr version of LLVM.

eholk · 2023-02-07T01:23:36Z

TBH I think it was a mistake to make dyn Trait and dyn* Trait part of the same TyKind variant. Almost everywhere in Miri this lead to the wrong default behavior, resulting in strange ICEs instead of nice "unimplemented" messages. The two types describe pretty different runtime data layout after all.

This totally makes sense, and I'd be up for rewriting it to split them apart.

The reason I added the layout as a field within the same variant originally was that, at least in the front parts of the compiler, they generally are pretty similar. For a lot of the type system, the only difference between dyn Trait and dyn* Trait is that dyn* is Sized while dyn isn't.

But yeah, later on the layouts are quite different, and it's not just too hard to do Dynamic(..) | DynStar(..) in the parts of the compiler where they really are the same.

RalfJung · 2023-02-07T09:33:01Z

--- stderr -------------------------------
Invalid bitcast
  %2 = bitcast i64 %0 to i8*
LLVM ERROR: Broken module found, compilation aborted!
------------------------------------------

So looks like something is inserting bitcasts, and that works fine on recent LLVM but old LLVM doesn't like them?

Probably the change to using a pointer ABI should be its own PR, a prerequisite to this one. @eholk is that something you could help with?

compiler-errors · 2023-02-07T18:05:15Z

@RalfJung I think I can do it unless @eholk already planned to -- I'll cherry-pick d64cb07d1d5a57117de2f1680f20aa14755a4f29 and fix the LLVM bugs on top of it. Seems like we just need to insert an inttoptr instead of ptrtoint when doing the usize -> *const () during codegen.

RalfJung · 2023-02-07T18:08:28Z

Seems like we just need to insert an inttoptr instead of ptrtoint when doing the usize -> *const () during codegen.

Yeah, probably. Ideally we'd insert the equivalent of ptr::invalid_mut but I don't think LLVM has a convenient way of expressing that.

There is also a bitcast coming from somewhere that we do not want... or maybe that's some automatic thing the codegen backend is doing when an immediate of one type is used in a situation that expects another type?

eholk · 2023-02-07T18:12:15Z

@RalfJung I think I can do it unless @eholk already planned to -- I'll cherry-pick d64cb07 and fix the LLVM bugs on top of it. Seems like we just need to insert an inttoptr instead of ptrtoint when doing the usize -> *const () during codegen.

@compiler-errors - go for it!

I was going to spend some time this afternoon splitting dyn* out from the ty::Dynamic variant to make it less easy to accidentally do the wrong thing with `dyn*.

…s-ptr, r=eholk Make `dyn*`'s value backend type a pointer One tweak on top of Ralf's commit should fix using `usize` as a `dyn*`-coercible type, and should fix when we're using various other pointer types when LLVM opaque pointers is disabled. r? `@eholk` but feel free to reassign cc rust-lang#107728 (comment) `@RalfJung`

oli-obk · 2023-02-18T00:27:31Z

@rustbot blocked #107772

oli-obk · 2023-02-20T14:08:57Z

~~I blessed some tests and refactored the main dyn/* condition, lmk what you think~~

edit: race condition, you pushed faster lol

oli-obk · 2023-02-20T14:12:54Z

ok, another try :)

compiler/rustc_const_eval/src/interpret/terminator.rs

RalfJung · 2023-02-20T17:04:25Z

r=me on your commits, and I think this is good to go.

oli-obk · 2023-02-20T18:09:11Z

@bors r=RalfJung,oli-obk

bors · 2023-02-20T18:09:12Z

📌 Commit 054c76d has been approved by RalfJung,oli-obk

It is now in the queue for this repository.

bors · 2023-02-21T04:22:26Z

⌛ Testing commit 054c76d with merge f715e43...

bors · 2023-02-21T07:02:51Z

☀️ Test successful - checks-actions
Approved by: RalfJung,oli-obk
Pushing f715e43 to master...

bors · 2023-02-21T07:02:51Z

☀️ Test successful - checks-actions
Approved by: RalfJung,oli-obk
Pushing f715e43 to master...

rust-timer · 2023-02-21T08:08:04Z

Finished benchmarking commit (f715e43): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.0%, 2.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

…tic, r=eholk Don't ICE when encountering `dyn*` in statics or consts Since we have properly implemented `dyn*` support in CTFE (rust-lang#107728), let's not ICE here anymore. Fixes rust-lang#105777 r? `@eholk`

rustbot assigned oli-obk Feb 6, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 6, 2023

RalfJung force-pushed the miri-dyn-star branch from b03d793 to f815d0f Compare February 6, 2023 15:18

RalfJung commented Feb 6, 2023

View reviewed changes

RalfJung mentioned this pull request Feb 6, 2023

Initial implementation of dyn* #101212

Merged

RalfJung force-pushed the miri-dyn-star branch 2 times, most recently from 14be9c7 to 47d7047 Compare February 6, 2023 15:59

RalfJung mentioned this pull request Feb 6, 2023

Reentrant RefCell issue with dyn* and Stacked Borrows rust-lang/miri#2779

Closed

RalfJung force-pushed the miri-dyn-star branch 2 times, most recently from 5cb9603 to 345d8d0 Compare February 6, 2023 22:47

RalfJung force-pushed the miri-dyn-star branch from 345d8d0 to 5cbdc7c Compare February 7, 2023 08:05

compiler-errors mentioned this pull request Feb 7, 2023

Make dyn*'s value backend type a pointer #107772

Merged

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Feb 10, 2023

Handle dyn* coercions for values that are represented with OperandValue::Ref #104694

Closed

RalfJung force-pushed the miri-dyn-star branch from 74aca1d to 10eae16 Compare February 10, 2023 21:02

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Feb 11, 2023

Create a new TyKind::DynStar variant #107908

Closed

rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Feb 19, 2023

This comment has been minimized.

Sign in to view

RalfJung added 3 commits February 20, 2023 15:08

basic dyn* support for Miri

b2f5814

slightly beef up dyn-star-to-dyn test

57056d7

fix Stacked Borrows interaction with dyn*

ad6dd60

RalfJung force-pushed the miri-dyn-star branch from 74c45f4 to ad6dd60 Compare February 20, 2023 14:08

oli-obk added 2 commits February 20, 2023 14:11

Deduplicate some logic between dyn* and dyn

472059d

Remove commented out code

82f1e34

RalfJung commented Feb 20, 2023

View reviewed changes

compiler/rustc_const_eval/src/interpret/terminator.rs Outdated Show resolved Hide resolved

Revert most of the custom place computation, it was unnecessary.

054c76d

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2023

bors added merged-by-bors This PR was explicitly merged by bors. labels Feb 21, 2023

bors merged commit f715e43 into rust-lang:master Feb 21, 2023

rustbot added this to the 1.69.0 milestone Feb 21, 2023

RalfJung deleted the miri-dyn-star branch February 26, 2023 18:09

compiler-errors mentioned this pull request Apr 4, 2023

Don't ICE when encountering dyn* in statics or consts #109921

Merged

Alexendoo mentioned this pull request Apr 7, 2023

ICE: dyn_star during const eval #105969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miri: basic dyn* support #107728

Miri: basic dyn* support #107728

RalfJung commented Feb 6, 2023 •

edited

Loading

rustbot commented Feb 6, 2023

RalfJung commented Feb 6, 2023

RalfJung Feb 6, 2023

RalfJung Feb 6, 2023

RalfJung Feb 6, 2023 •

edited

Loading

RalfJung Feb 6, 2023

RalfJung Feb 6, 2023

RalfJung Feb 6, 2023

eholk commented Feb 7, 2023

RalfJung commented Feb 7, 2023

compiler-errors commented Feb 7, 2023

RalfJung commented Feb 7, 2023

eholk commented Feb 7, 2023

This comment has been minimized.

This comment has been minimized.

oli-obk commented Feb 18, 2023

This comment has been minimized.

oli-obk commented Feb 20, 2023 •

edited

Loading

oli-obk commented Feb 20, 2023

RalfJung commented Feb 20, 2023

oli-obk commented Feb 20, 2023

bors commented Feb 20, 2023

bors commented Feb 21, 2023

bors commented Feb 21, 2023

bors commented Feb 21, 2023

rust-timer commented Feb 21, 2023

Miri: basic dyn* support #107728

Miri: basic dyn* support #107728

Conversation

RalfJung commented Feb 6, 2023 • edited Loading

rustbot commented Feb 6, 2023

RalfJung commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eholk commented Feb 7, 2023

RalfJung commented Feb 7, 2023

compiler-errors commented Feb 7, 2023

RalfJung commented Feb 7, 2023

eholk commented Feb 7, 2023

This comment has been minimized.

This comment has been minimized.

oli-obk commented Feb 18, 2023

This comment has been minimized.

oli-obk commented Feb 20, 2023 • edited Loading

oli-obk commented Feb 20, 2023

RalfJung commented Feb 20, 2023

oli-obk commented Feb 20, 2023

bors commented Feb 20, 2023

bors commented Feb 21, 2023

bors commented Feb 21, 2023

bors commented Feb 21, 2023

rust-timer commented Feb 21, 2023

Overall result: ✅ improvements - no action needed

RalfJung commented Feb 6, 2023 •

edited

Loading

RalfJung Feb 6, 2023 •

edited

Loading

oli-obk commented Feb 20, 2023 •

edited

Loading