Get rid of the `Scalar` and `ScalarPair` variants of `ConstValue`... #55260

oli-obk · 2018-10-22T11:12:00Z

... by always using ByRef and reading the scalars from the backing Allocation where desired

Oh god this uncovered so many bugs and bugs-just-waiting-to-happen. I'm still not fully done, but I don't want to grow this PR any further. It is in a sane state except for the const_field function, which was hacky to begin with.

fixes #54738

r? @RalfJung

oli-obk · 2018-10-22T12:13:14Z

@bors try

for perf run

bors · 2018-10-22T18:36:13Z

⌛ Trying commit f3100df with merge 621ca4e...

@RalfJung

Get rid of the `Scalar` and `ScalarPair` variants of `ConstValue`... ... by always using `ByRef` and reading the scalars from the backing `Allocation` where desired Oh god this uncovered so many bugs and bugs-just-waiting-to-happen. I'm still not fully done, but I don't want to grow this PR any further. It is in a sane state except for the `const_field` function, which was hacky to begin with. fixes #54738 r? @RalfJung

oli-obk · 2018-10-22T20:19:41Z

@rust-timer build f3100df

rust-timer · 2018-10-22T21:11:27Z

Bors try commit f3100df unexpectedly has 1 parents.

bors · 2018-10-22T21:19:08Z

💔 Test failed - status-travis

rust-highfive · 2018-10-22T21:19:09Z

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_fold:end:services

travis_fold:start:git.checkout
travis_time:start:340bfcc0
$ git clone --depth=2 --branch=try https://github.com/rust-lang/rust.git rust-lang/rust
---
Resolving deltas: 100% (7654/7654), done.
travis_time:end:340bfcc0:start=1540239404934436557,finish=1540239412039311187,duration=7104874630
$ cd rust-lang/rust
$ git checkout -qf 621ca4e381532a956260abc1675160714542804f
fatal: reference is not a tree: 621ca4e381532a956260abc1675160714542804f
The command "git checkout -qf 621ca4e381532a956260abc1675160714542804f" failed and exited with 128 during .
Your build has been stopped.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

RalfJung · 2018-10-23T06:43:10Z

@rust-timer build 621ca4e

(Need to give the merge commit.)

rust-timer · 2018-10-23T06:43:11Z

Success: Queued 621ca4e with parent a66dc8a, comparison URL.

oli-obk · 2018-10-23T08:56:14Z

@bors try

(checkout failure, I don't think perfbot has anything to run perf on)

rust-highfive · 2018-10-23T09:20:57Z

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_fold:end:services

travis_fold:start:git.checkout
travis_time:start:094871d6
$ git clone --depth=2 --branch=try https://github.com/rust-lang/rust.git rust-lang/rust

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

oli-obk · 2018-10-23T09:58:04Z

Everything seems stuck, not sure what to do, cycling the PR

…e type

RalfJung · 2018-10-23T11:24:54Z

I wanted to do this separately, when I make memory.rs actually use any of that code. Right now there's loads of duplication

I'd be more confident that all this code actually does the right thing if it was used by memory, and if you verified that miri still works on top of it.

RalfJung · 2018-10-23T11:26:10Z

src/librustc/mir/interpret/value.rs

+    /// if this is ByRef, return the same thing but with the offset increased by `n`
+    pub fn try_offset(&self, n: Size) -> Option<Self> {
+        let (id, alloc, offset) = self.try_as_by_ref()?;
+        Some(ConstValue::ByRef(id, alloc, offset + n))


We have complex and subtle code to do overflow-aware (which is architecture-dependent) pointer arithmetic. We surely shouldn't just do plain addition here.

RalfJung · 2018-10-23T11:26:36Z

src/librustc/mir/interpret/value.rs

+    }
+
+    #[inline]
+    pub fn try_get_bytes(&self, hdl: impl HasDataLayout, n: Size, align: Align) -> Option<&[u8]> {


hdl? We usually called this cx, didn't we?

RalfJung · 2018-10-23T11:29:48Z

src/librustc/mir/interpret/value.rs

+                tcx.data_layout().endian,
+                &mut alloc.bytes[i..j],
+                int.into(),
+            ).unwrap();


Uh, so here we are duplicating write_sclar? Not good. You moved so many methods here, including read_scalar, why not also write_scalar?

src/librustc_mir/interpret/operand.rs

RalfJung · 2018-10-23T11:40:53Z

src/librustc/mir/interpret/mod.rs

+        ty::tls::with(|tcx| {
+            let (s, o) = {
+                let map = tcx.alloc_map.lock();
+                (map.get(*self), map.get(*other))


Uh, wait, what? That doesn't seem right to me.

Certainly, ptr comparison in miri should compare the actual IDs, the the content that's stored behind these IDs. What is the type of s and o here?

This only applies to AllocIds that refer to global allocations. Comparing by id is inherently wrong. We never ran into any trouble, because we never used a ConstValue containing an AllocId in comparisons that mattered. Now that everything is ByRef, pattern match exhaustiveness checks failed because they couldn't figure out that two const patterns are the same if their AllocId differs.

This only applies to AllocIds that refer to global allocations.

It is still grossly wrong in the context of the miri engine. Ptr equality wants to compare the IDs, not the allocations they point to, and the IDs should be newtyped to not mix them up with other integers.

pattern match exhaustiveness checks failed because they couldn't figure out that two const patterns are the same if their AllocId differs.

Then it shouldn't use the same equality as the miri engine. If you want to overwrite equality and hashing for ConstValue, I can support that, but doing it for a type used inside the engine makes me really nervous.

RalfJung · 2018-10-23T11:41:34Z

src/librustc/mir/interpret/mod.rs

+    fn cmp(&self, other: &Self) -> cmp::Ordering {
+        match self.partial_cmp(other) {
+            Some(ord) => ord,
+            None => self.0.cmp(&other.0)


Why is it okay to fall back to comparing the integers here?

This happens only for non-interned allocations

RalfJung · 2018-10-23T11:42:24Z

src/librustc/mir/interpret/mod.rs

@@ -440,7 +488,7 @@ pub enum AllocType<'tcx, M> {

 pub struct AllocMap<'tcx, M> {
    /// Lets you know what an AllocId refers to
-    id_to_type: FxHashMap<AllocId, AllocType<'tcx, M>>,
+    id_to_type: FxHashMap<u64, AllocType<'tcx, M>>,


Now we lose the newtype protection for not mixing up IDs.

I don't understand the purpose of this entire commit, at all. It also suffers from an astute lack of comments :)

RalfJung · 2018-10-23T11:45:11Z

src/librustc/mir/interpret/mod.rs

@@ -736,22 +736,23 @@ impl<Tag: Copy, Extra: Default> Allocation<Tag, Extra> {
        &self,
        hdl: impl HasDataLayout,
        offset: Size,
+        size: Size,
+        align: Align,
    ) -> EvalResult<'tcx, Scalar<Tag>> {


Wait, why does get_bytes return a Scalar<Tag>? That doesn't make any sense, a method of that name should return a &[u8]. Now I am even more puzzled about what it does.

(See, a doc comment would have been useful. ;)

this function is called read_scalar?

Hm did the diff view fool me? Yes it did.

I am still quite confused why this looks so different from what we do in memory.rs?

RalfJung · 2018-10-23T11:47:29Z

src/librustc_codegen_llvm/mir/operand.rs

+                        );
+                        OperandValue::Immediate(llval)
+                    },
+                    layout::Abi::ScalarPair(ref a_scalar, ref b_scalar) => {


This is a duplicate of... read_value, I guess? Why can't we use that instead?

RalfJung · 2018-10-23T11:50:24Z

src/librustc/ty/sty.rs

+            // we produce a weird dummy layout with somewhat sane values
+            let mut layout = tcx.layout_of(ParamEnv::reveal_all().and(tcx.types.u128)).unwrap();
+            layout.ty = ty.value;
+            layout


Woah.

Your comment consists of two sentences, right, despite there not being a . nor the first word of the second sentence being capitalized? I am not sure but so far I haven't found another way to parse these sentences. Please add punctuation :)

And also please add that delay_span_bug. I mean I have no clue when and where and how this method is used, but just using u128 clearly cannot be right. Maybe add a comment saying why this even remotely makes sense -- i.e., is the result of this method unused in the error case, or so?

(I read it as one sentence. "weird" and "dummy" are adjectives modifying "layout", and "with somewhat sane values" is also attached to "layout.")

GitHub cut away half the comment. I cannot parse the following as one sentence:

add delay_span_bug call, we can only get here if there are errors we produce a weird dummy layout with somewhat sane values

RalfJung · 2018-10-23T11:50:53Z

src/librustc_mir/interpret/operand.rs

@@ -359,7 +359,7 @@ pub(super) fn from_known_layout<'tcx>(
 impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
    /// Try reading a value in memory; this is interesting particularily for ScalarPair.
    /// Return None if the layout does not permit loading this as a value.
-    pub(super) fn try_read_value_from_mplace(
+    pub(crate) fn try_read_value_from_mplace(


These were meant to be internal to the engine :(

RalfJung · 2018-10-23T11:51:58Z

src/librustc_mir/transform/const_prop.rs

-                    "could not evaluate constant",
-                );
-                None
+        let error = match self.ecx.const_to_mplace(c.literal) {


Why not use read_value?

src/test/ui/consts/const-eval/const-pointer-values-in-various-types.rs

src/librustc/mir/interpret/mod.rs

src/librustc_codegen_llvm/mir/operand.rs

bors · 2018-10-23T12:16:48Z

☀️ Test successful - status-travis
State: approved= try=True

RalfJung · 2018-10-23T12:28:35Z

src/librustc_mir/const_eval.rs

+    // as this function is essentially copying the value
+    // out of the larger allocation, so we lose all information about
+    // potential surrounding types with different alignment.
+    alloc.align = mplace.layout.align;


This looks very wrong. Couldn't there still be other ConstValue around that refer to the same allocation?

We should never, ever change the align of an existing allocation. Even making a copy seems more sane, and that's already a rather insane option.^^

oh yea, this code was broken to begin with. That's one of the uncovered bugs I was talking about.

A few commits down I'm actually creating a new Allocation from scratch instead of this wrongness. I can't make it not copy right now, that would need large changes to ByRef that I didn't want to do in this PR

RalfJung · 2018-10-23T12:29:02Z

src/librustc/mir/interpret/value.rs

@@ -50,6 +50,7 @@ impl<'tcx> ConstValue<'tcx> {
    #[inline]
    pub fn try_get_bytes(&self, hdl: impl HasDataLayout, n: Size, align: Align) -> Option<&[u8]> {
        let (_, alloc, offset) = self.try_as_by_ref()?;
+        trace!("{:?}", alloc.get_bytes(hdl, offset, n, align));


Please add the function name to this, or it'll flood my screen one day and I won't even know where it comes from.

RalfJung · 2018-10-23T12:29:45Z

src/librustc/ty/sty.rs

@@ -2008,12 +2008,12 @@ impl<'tcx> Const<'tcx> {

    #[inline]
    pub fn from_bool(tcx: TyCtxt<'_, '_, 'tcx>, v: bool) -> &'tcx Self {
-        Self::from_bits(tcx, v as u128, ParamEnv::empty().and(tcx.types.bool))
+        Self::from_bits(tcx, v as u128, ParamEnv::reveal_all().and(tcx.types.bool))


@eddyb won't like that, they taught me that reveal_all is usually wrong. I don't have any clue what this does, though, so I'll defer to your judgement here @oli-obk.

I can change it back. But it's actually irrelevant here, since it gets used in layout_of which (I checked) directly turns any ParamEnv into a reveal_all.

directly turns any ParamEnv into a reveal_all.

Oh. Is there a way to make it not take a ParamEnv then if it gets ignored anway?

RalfJung · 2018-10-23T12:32:40Z

src/librustc_mir/const_eval.rs

+        // out of the larger allocation, so we lose all information about
+        // potential surrounding types with different alignment.
+        align: mplace.layout.align,
+    };


All this memory-modifying code outside of memory.rs makes me really sad. :( The (subtle and correctness-critical!) relocations invariant was already scattered across that entire file, can we avoid scattering it over the entire rustc codebase?

Mark-Simulacrum · 2018-10-23T16:41:28Z

@bors try

something went wrong with the previous try commit build so we'll want to re-queue perf with a new one

oli-obk · 2018-10-23T16:56:39Z

I thought I already did that with #55260 (comment) which went through fine

rust-timer · 2018-10-24T05:43:50Z

Finished benchmarking try commit 3e4570cd5f53a5ad28f9a74e7ea1599cd22e5470

oli-obk · 2018-10-24T08:02:46Z

OOOOKAY: avg: 33219.2% | min: 22801.4% | max: 62508.3%

I really need to rethink this strategy

oli-obk · 2018-10-24T15:52:57Z

I created a separate PR for the memory -> allocation function moves: #55293

I'll split out things of this PR until it becomes manageable. Next up: removing only ConstValue::ScalarPair

TimNN · 2018-11-06T18:18:14Z

Ping from triage! If I understand correctly, this PR won't be merged in its current state, so I'm closing it for the time being. Feel free to re-open if you want to merge it in the future.

rust-highfive assigned RalfJung Oct 22, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 22, 2018

oli-obk closed this Oct 23, 2018

oli-obk reopened this Oct 23, 2018

oli-obk added 12 commits October 23, 2018 11:59

Update function names in comments

edf94c5

Simplify ConstValue

856eaed

Make the HashStable impl of Allocation safer for future changes of th…

fe4e950

…e type

The Hash and Eq impls of ConstValue were inherently broken

04fc561

Fixup a bunch of things for "simplify const value"

b11b3cb

Fix some simd intrinsics

3aecb0d

Fix an ICE that can occur while errors are already being emitted

8d42376

Const prop is only interested in immediate constants right now

847f5b9

Update a few tests with their improved diagnostic messages

e52644c

Alignment check facepalm

8246dd4

Enums can produce scalar pairs with undefs in one of the elements

5620d68

Hide a constructor function that is unused outside this module

fc6472c

RalfJung reviewed Oct 23, 2018

View reviewed changes

src/librustc_mir/interpret/operand.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 23, 2018

View reviewed changes

src/test/ui/consts/const-eval/const-pointer-values-in-various-types.rs Show resolved Hide resolved

RalfJung reviewed Oct 23, 2018

View reviewed changes

src/librustc/mir/interpret/mod.rs Show resolved Hide resolved

RalfJung reviewed Oct 23, 2018

View reviewed changes

src/librustc_codegen_llvm/mir/operand.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 23, 2018

View reviewed changes

oli-obk mentioned this pull request Oct 23, 2018

Self managing allocations #55293

Closed

TimNN closed this Nov 6, 2018

oli-obk deleted the by_ref_all_the_consts branch June 15, 2020 15:29

Get rid of the Scalar and ScalarPair variants of ConstValue... #55260

Get rid of the Scalar and ScalarPair variants of ConstValue... #55260

Conversation

oli-obk commented Oct 22, 2018 • edited Loading

oli-obk commented Oct 22, 2018

bors commented Oct 22, 2018

oli-obk commented Oct 22, 2018

rust-timer commented Oct 22, 2018

bors commented Oct 22, 2018

rust-highfive commented Oct 22, 2018

RalfJung commented Oct 23, 2018

rust-timer commented Oct 23, 2018

oli-obk commented Oct 23, 2018

rust-highfive commented Oct 23, 2018

oli-obk commented Oct 23, 2018

RalfJung commented Oct 23, 2018

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

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

RalfJung Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

RalfJung Oct 23, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 23, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Oct 23, 2018

oli-obk commented Oct 23, 2018

rust-timer commented Oct 24, 2018

oli-obk commented Oct 24, 2018

oli-obk commented Oct 24, 2018

TimNN commented Nov 6, 2018

Get rid of the `Scalar` and `ScalarPair` variants of `ConstValue`... #55260

Get rid of the `Scalar` and `ScalarPair` variants of `ConstValue`... #55260

oli-obk commented Oct 22, 2018 •

edited

Loading

RalfJung Oct 23, 2018 •

edited

Loading

RalfJung Oct 23, 2018 •

edited

Loading

RalfJung Oct 23, 2018 •

edited

Loading