Fix ptr-int-casts #267

RalfJung · 2017-07-20T21:04:53Z

(-1i32) as usize as *const i32 as usize would yield 0xff. That's clearly wrong. Not it yields what we would expect.

I also did some more refactoring of casting in general as I wasn't satisfied with how pointers were handled.

solson

The renaming and refactoring looks good. Aside from the * 8 the rest of the diff makes no functional change, right?

solson · 2017-07-20T21:22:46Z

src/cast.rs

@@ -81,33 +78,33 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
            TyChar => Err(EvalError::InvalidChar(v)),

            // No alignment check needed for raw pointers
-            TyRawPtr(_) => Ok(PrimVal::Bytes(v % (1 << self.memory.pointer_size()))),
+            TyRawPtr(_) => Ok(PrimVal::Bytes(v % (1 << (self.memory.pointer_size()*8)))),


Formatting nit: x * 8, not x*8.

It also might be nice to have a comment here saying what we're effectively doing (which is truncating to the target usize width, right?).

I was also worried for a second that we'd have 1 << 64 if the target is 64-bit, which would be an overlong shift if the 1 inferred to anything smaller than a 128-bit integer. But I guess since v: u128, the RHS of % must be u128 also, and if 1 << n is u128, the 1 must be u128, so it won't be an overlong shift? Maybe just throw 1u128 in there explicitly to soothe my brain. :P

RalfJung · 2017-07-20T21:24:08Z

The renaming and refactoring looks good. Aside from the * 8 the rest of the diff makes no functional change, right?

That's the intention.

RalfJung · 2017-07-22T03:04:33Z

Okay I think I fixed the nits.

I would have liked to share the ptr truncation code here with the one in value.rs. However, for that I'd have to change the signature of the pointer_offset functions to take a memory (rather than just a data layout). Then I would put the truncation function into memory. What do you think?

RalfJung · 2017-07-22T06:38:30Z

That error doesn't look like this patch could have caused it?

stderr:

------------------------------------------

error: linking with `cc` failed: exit code: 1

  |

  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/home/travis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/tmp/aux_test.0.o" "-o" "/tmp/aux_test.stage-id" "/tmp/aux_test.crate.allocator.o" "-Wl,--gc-sections" "-pie" "-Wl,-z,relro,-z,now" "-nodefaultlibs" "-L" "/tmp" "-L" "/tmp/aux_test.stage-id.mir-opt.libaux" "-L" "/home/travis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-L" "/tmp/aux_test.stage-id.mir-opt.libaux" "-l" "dep" "-L" "/home/travis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-l" "std-374c2c93f42d00d4" "-Wl,-Bstatic" "/home/travis/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-78c8b0733fc2dac1.rlib" "-Wl,-Bdynamic" "-l" "dl" "-l" "rt" "-l" "pthread" "-l" "pthread" "-l" "gcc_s" "-l" "c" "-l" "m" "-l" "rt" "-l" "pthread" "-l" "util"

  = note: cc: error: /tmp/aux_test.0.o: No such file or directory

oli-obk · 2017-07-22T07:26:22Z

We're getting spurious Travis failures lately. I restarted it

Sharing code sounds good to me.

solson · 2017-07-22T07:33:40Z

However, for that I'd have to change the signature of the pointer_offset functions to take a memory (rather than just a data layout)

@RalfJung Why is this? (Actually, I don't even know why those function have to take the whole TargetDataLayout - it seems like they only use the target bit width, i.e. layout.pointer_size.bits().)

RalfJung · 2017-07-22T07:39:36Z

@solson well they would have to call the memory method that does truncation. Any better idea where to put it? It needs access to a TargetDataLayout. I suppose it could also be frestanding.

oli-obk · 2017-07-22T07:51:56Z

We can reverse the function arguments and stop passing memory or parts of it around. Instead we always call memory.some_action(ptr). That's more in line of how we work with the EvalContext

oli-obk · 2017-07-22T10:40:29Z

I would have liked to share the ptr truncation code here with the one in value.rs.

Do you want to do this in this PR or a future PR?

RalfJung · 2017-07-22T16:58:11Z

I guess I'll add it here so I don't forget :)

RalfJung · 2017-07-22T18:30:20Z

Okay I did the refactoring. Unfortunately running the test suite doesn't work any more since #258, and I have to rush now so I can't debug that. I guess Travis will tell me where tests break.^^

eddyb · 2017-07-22T18:49:53Z

src/librustc_mir/interpret/memory.rs

@@ -73,26 +73,26 @@ impl MemoryPointer {
        MemoryPointer { alloc_id, offset }
    }

-    pub fn wrapping_signed_offset<'tcx>(self, i: i64, layout: &TargetDataLayout) -> Self {
-        MemoryPointer::new(self.alloc_id, value::wrapping_signed_offset(self.offset, i, layout))
+    pub(crate) fn wrapping_signed_offset<'a, L: HasDataLayout<'a>>(self, i: i64, l: L) -> Self {


Isn't this 'a actually 'tcx?

Also, in general, if you have a non-trait impl, you can put lifetimes on it instead of individual methods.

The layout is borrowed from the tcx, and I'm fairly sure it needs the 'a lifetime from the tcx

Memory has two lifetime parameters, 'a and 'tcx. This is the 'a.

You want me to factor out the 'a?

Ohhh I know why I'm confused, you're not using the tools in ty::layout!

E.g. https://github.com/rust-lang/rust/blob/ef19bd1a339212f77160f12af6d5d85d8bb36ea4/src/librustc/ty/layout.rs#L536-L537

Ah, I didn't realize this trait already exists. :)

However, when I tried sth. similar, I came into trouble in the generic impl from HasMemory because of lifetimes not lasting long enough. The trait was slightly different though, maybe it works. (I will only be able to try this on Monday though.)

FWIW there are a bunch of things in miri that can now be replaced with some trans-like setup plus using the TyLayout methods - and they'll be guaranteed to work in any situation the trans code already does.

I used some of the code from rustc. The rest is a refactoring we should do, so I opened an issue for it

oli-obk · 2017-07-23T13:03:50Z

Feel free to merge, the refactoring is a little too involved to include here imo.

eddyb · 2017-07-23T13:28:35Z

src/librustc_mir/interpret/memory.rs

    pub fn new(alloc_id: AllocId, offset: u64) -> Self {
        MemoryPointer { alloc_id, offset }
    }

-    pub(crate) fn wrapping_signed_offset<'a, L: HasDataLayout<'a>>(self, i: i64, l: L) -> Self {
+    pub(crate) fn wrapping_signed_offset<L: PointerArithmetic>(self, i: i64, l: L) -> Self {


It'd also be nice to use the cx: C (like I did in rustc::ty::layout) instead of l: L.

Done. Also shortened all use sites.

RalfJung · 2017-07-24T17:39:31Z

src/librustc_mir/interpret/memory.rs

    #[inline]
-    fn data_layout(self) -> &'a TargetDataLayout {
-        self
+    fn data_layout(&self) -> &TargetDataLayout {


Ah, that's the difference to what I tried -- you are passing &&Memory here, which seems a little weird. Whatever^^

RalfJung changed the title ~~FIx ptr-int-casts~~ Fix ptr-int-casts Jul 20, 2017

RalfJung mentioned this pull request Jul 20, 2017

(0-1) as *const T triggers horribly stupid behavior rust-lang/rust#43291

Closed

solson reviewed Jul 20, 2017

View reviewed changes

RalfJung added 3 commits July 21, 2017 20:02

fix casting integers to pointers

f960f4c

refactor casting code to more clearly handle pointers

b174b78

fix nits

40950b2

RalfJung force-pushed the cast branch from 95dcf89 to 40950b2 Compare July 22, 2017 03:03

solson approved these changes Jul 22, 2017

View reviewed changes

move pointer truncation to a common method in memory.rs

4d38f8d

eddyb reviewed Jul 22, 2017

View reviewed changes

Use rustc traits instead of our own

9c07f42

oli-obk mentioned this pull request Jul 23, 2017

Use functions from rustc to simplify our layout and type operations. #271

Closed

eddyb reviewed Jul 23, 2017

View reviewed changes

Pass HasDataLayout instead of the data layout itself

4a34a1b

oli-obk merged commit 03ca15e into rust-lang:master Jul 24, 2017

RalfJung commented Jul 24, 2017

View reviewed changes

RalfJung deleted the cast branch August 29, 2017 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ptr-int-casts #267

Fix ptr-int-casts #267

RalfJung commented Jul 20, 2017

solson left a comment

solson Jul 20, 2017

RalfJung commented Jul 20, 2017

RalfJung commented Jul 22, 2017

RalfJung commented Jul 22, 2017

oli-obk commented Jul 22, 2017

solson commented Jul 22, 2017

RalfJung commented Jul 22, 2017

oli-obk commented Jul 22, 2017

oli-obk commented Jul 22, 2017

RalfJung commented Jul 22, 2017

RalfJung commented Jul 22, 2017

eddyb Jul 22, 2017

eddyb Jul 22, 2017

oli-obk Jul 22, 2017

RalfJung Jul 22, 2017 •

edited

Loading

eddyb Jul 23, 2017

RalfJung Jul 23, 2017

eddyb Jul 23, 2017

oli-obk Jul 23, 2017

oli-obk commented Jul 23, 2017

eddyb Jul 23, 2017

oli-obk Jul 24, 2017

RalfJung Jul 24, 2017

Fix ptr-int-casts #267

Fix ptr-int-casts #267

Conversation

RalfJung commented Jul 20, 2017

solson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jul 20, 2017

RalfJung commented Jul 22, 2017

RalfJung commented Jul 22, 2017

oli-obk commented Jul 22, 2017

solson commented Jul 22, 2017

RalfJung commented Jul 22, 2017

oli-obk commented Jul 22, 2017

oli-obk commented Jul 22, 2017

RalfJung commented Jul 22, 2017

RalfJung commented Jul 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jul 22, 2017 • 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

oli-obk commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jul 22, 2017 •

edited

Loading