Get rid of the integer allocation #197

oli-obk · 2017-06-19T11:30:16Z

fixes #189

oli-obk · 2017-06-19T11:30:57Z

src/eval_context.rs

@@ -872,7 +871,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
        let pointee_size = self.type_size(pointee_ty)?.expect("cannot offset a pointer to an unsized type") as i64;
        return if let Some(offset) = offset.checked_mul(pointee_size) {
            let ptr = ptr.signed_offset(offset, self.memory.layout)?;
-            self.memory.check_bounds(ptr, false)?;
+            // Why are we doing a bounds check on a pointer?


cc @RalfJung

Pretty sure this is the offset method which is UB on out-of-bounds.

So... the offset intrinsic is illegal on integral pointers?

And the same goes for the Offset MIR-operator?

I think so. The standard library uses arith_offset when using integer pointers.

These are the semantics that miri follows, so we're all good.

oli-obk · 2017-06-19T16:00:55Z

A few bugs snuck in, but they're all fixed now

RalfJung · 2017-06-19T17:48:56Z

src/memory.rs

 }

 pub type TlsKey = usize;

 #[derive(Copy, Clone, Debug)]
 pub struct TlsEntry<'tcx> {
-    data: Pointer, // Will eventually become a map from thread IDs to pointers, if we ever support more than one thread.
+    data: PrimVal, // Will eventually become a map from thread IDs to pointers, if we ever support more than one thread.


The comment needs updating.

RalfJung · 2017-06-19T17:51:24Z

src/memory.rs

-                    return Some((dtor, old_data));
+    pub(crate) fn fetch_tls_dtor(&mut self) -> Option<(ty::Instance<'tcx>, PrimVal)> {
+        for (_, &mut TlsEntry { ref mut data, ref mut dtor }) in self.thread_local.iter_mut() {
+            if let Some(dtor) = dtor.take() {


This changes the behavior, right? Now the dtor becomes None. If the dtor initializes this variable back to something non-NULL, this will mean the dtor is not run again. I think that's wrong, pthread says the dtors are run multiple times until the variables are all NULL.

hmm... The previous code also set data to null. Can the destructor set it back to a value?

Yes it can. The pthreads doc explicitly says that implementations have to check data again after the dtor runs, and repeat until data is NULL for all thread-local variables. (There's also some limit to prevent endless destructing. miri currently doesn't implement that limit.)

The docs are actually pretty good, as they fairly specifically they how to implement this:

An optional destructor function may be associated with each key value. At thread exit, if a key value has a non-NULL destructor pointer, and the thread has a non-NULL value associated with that key, the value of the key is set to NULL, and then the function pointed to is called with the previously associated value as its sole argument. The order of destructor calls is unspecified if more than one destructor exists for a thread when it exits.

If, after all the destructors have been called for all non-NULL values with associated destructors, there are still some non-NULL values with associated destructors, then the process is repeated. If, after at least {PTHREAD_DESTRUCTOR_ITERATIONS} iterations of destructor calls for outstanding non-NULL values, there are still some non-NULL values with associated destructors, implementations may stop calling destructors, or they may continue calling destructors until no non-NULL values with associated destructors exist, even though this might result in an infinite loop.

Seems like infinite looping is valid behaviour. I vote we go with infinite looping, since miri already has an instruction counter

Yeah that's what I implemented -- or at least, it's what I intended to implement.

EDIT: Probably it'd make sense to copy the paragraphs above into the source.

I think we have to do the latter, because executing one dtor could affect the data value of another TLS key.

That's how I read it, too. Now it gets funky: what if a destructor creates a new key? We'll have a classical "collection modified while iterating" situation.

I would think it gets added to the first round, but could be convinced otherwise.^^

I imagine something like the following (horribly inefficient) pseudocode:

while there_is_any_key_with_non_null_dtor_and_data() { let mut i = 0; while i < keys.len() { keys[i].run_dtor_if_non_null_data(); i += 1; } }

Fun fact: the previous impl was with a HashMap, which a motivated developer could have used to write a nondeterministic random number generator in miri.

Ah, right, HashMap per default actually sources from the OS PRNG... nice catch. :D

RalfJung · 2017-06-19T17:52:00Z

src/operator.rs

-            }
-            val
-        }
-        let (left, right) = (normalize(left), normalize(right));


Yay for this being gone :)

RalfJung · 2017-06-19T18:01:33Z

src/value.rs

+    pub fn wrapping_signed_offset(self, i: i64, layout: &TargetDataLayout) -> EvalResult<'tcx, Self> {
+        match self {
+            PrimVal::Bytes(b) if b as u64 as u128 == b => Ok(PrimVal::Bytes(wrapping_signed_offset(b as u64, i, layout) as u128)),
+            PrimVal::Bytes(_) => Err(EvalError::OverflowingMath),


This is explicitly intended to wrap, how can it ever make sense to complain about overflow here?

This should be an unreachable probably. I don't see how you can get a u128 to go through this path. That's the only overflow that can happen...

Yeah it seems like the types should prevent that. unreachable or bug both seem appropriate.

Btw, does the same apply to the non-wrapping offset?

I can't think of a way to reach it. So let's just make it unreachable and fix it if it becomes a problem.

I think the condition in the first branch should be assert_eq!(b as u64 as u128, b) because it's a misuse of this method on a PrimVal::Bytes that is not a pointer.

RalfJung · 2017-06-19T22:00:01Z

src/step.rs

@@ -33,21 +32,21 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
        self.memory.clear_packed();
        self.inc_step_counter_and_check_limit(1)?;
        if self.stack.is_empty() {


Instead of triggering this on "stack empty" (and duplicating code), what about making the main function's cleanup StackPopCleanup::Tls(0) (or StackPopCleanup::Tls(None))?

RalfJung · 2017-06-19T22:03:04Z

src/memory.rs

+            Some(key) => Excluded(key),
+            None => Unbounded,
+        };
+        for (&key, &mut TlsEntry { ref mut data, dtor }) in self.thread_local.range_mut((start, Unbounded)) {


Where does the "start all over again when we reached the end of the table" happen?

When the stack is empty, just like before

Ah, I see. That also explains why you kept the "stack cleanup" to be None for the initial main.

oli-obk · 2017-06-20T11:24:31Z

I've run this PR on the rustc test suite, and we get a bunch of failures, because sometimes integer pointers are used for zst pointers, which used to be fine, since the pointers were passed on until they reached the Memory struct. Inside the Memory struct a zst check would usually prevent any action from being taken on the pointers. To fix this we would have to change most Memory method args from Pointer to PrimVal.

The alternative is to sprinkle around zst checks everywhere, which gets tedious fast and is quite error prone

RalfJung · 2017-06-20T16:40:31Z

src/eval_context.rs

@@ -893,6 +893,10 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
        }
        // FIXME: assuming here that type size is < i64::max_value()
        let pointee_size = self.type_size(pointee_ty)?.expect("cannot offset a pointer to an unsized type") as i64;
+        if pointee_size == 0 {
+            // rustc relies on offsetting pointers to zsts to be a nop


Where does this occur? So far I've only observed "offset-by-0" on ZSTs. If the offset is >0, this sounds very fishy from an LLVM perspective.

RalfJung · 2017-06-20T16:49:24Z

To fix this we would have to change most Memory method args from Pointer to PrimVal.
The alternative is to sprinkle around zst checks everywhere, which gets tedious fast and is quite error prone

Is there even anything left that takes the Pointer type?^^
One thing I noticed while working on my patches on top of your branch is that it sometimes leads to funny situations like x.read_ptr(...).to_ptr().

Inside the Memory struct a zst check would usually prevent any action from being taken on the pointers.

Alignment should usually still be checked on integer pointers though, shouldn't it? rustc went from using "1 as *const _" to using "align as *const _" to ensure that things are properly aligned.

oli-obk · 2017-06-20T18:27:22Z

tests/run-pass/slice-of-zero-size-elements.rs

+    Some(it.into_slice())
+}
+
+pub fn main() {


I think it was this test @RalfJung that triggered offset for zsts

All right I will look into this. This is interesting from an unsafe code guidelines standpoint.

Indeed, something fishy may be going on, but it's not miri's fault. ;)
See rust-lang/rust-memory-model#38 (comment).

oli-obk · 2017-06-20T18:29:44Z

it sometimes leads to funny situations like x.read_ptr(...).to_ptr().

There are a few, but they should often not exist and rather require changing our api

oli-obk · 2017-06-20T18:31:11Z

Alignment should usually still be checked on integer pointers though, shouldn't it? rustc went from using "1 as *const _" to using "align as *const _" to ensure that things are properly aligned

Sounds reasonable.

oli-obk · 2017-06-20T18:33:46Z

Hmm I think we should stop testing without full mir

RalfJung · 2017-06-20T18:38:04Z

Ideally we could mark tests to be ignored without full MIR -- feel that makes some of the debugging simpler if one can start focusing on small things, without having to support the entire libstd boilerplate. (I am currently using that in the "detect UB"-checks I am developing.)

Maybe have two folders, "run-pass" and "run-pass-simple" or so?

oli-obk · 2017-06-21T06:58:46Z

whoops...

reopened as #200

impl ParallelDrainRange

oli-obk commented Jun 19, 2017

View reviewed changes

oli-obk force-pushed the master branch 2 times, most recently from ce810a8 to d53d154 Compare June 19, 2017 15:06

RalfJung reviewed Jun 19, 2017

View reviewed changes

src/operator.rs

}

val

}

let (left, right) = (normalize(left), normalize(right));

Copy link

Member

RalfJung Jun 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay for this being gone :)

RalfJung reviewed Jun 19, 2017

View reviewed changes

oli-obk and others added 7 commits June 20, 2017 14:28

Update to latest nightly

f5ca91e

Get rid of the integer allocation

fcf4958

Add pthread docs

43afa20

Run the tls destructors in the correct order

a2baeb5

Simplify the return lvalue

75fddee

Fix unions

a6734cd

Use PrimVal instead of Pointer where applicable

ea6f607

oli-obk force-pushed the master branch from d1196da to ea6f607 Compare June 20, 2017 12:28

Enable more zst writes and reads

a82fe9a

RalfJung reviewed Jun 20, 2017

View reviewed changes

oli-obk commented Jun 20, 2017

View reviewed changes

oli-obk merged commit a82fe9a into rust-lang:master Jun 21, 2017

erickt pushed a commit to erickt/miri that referenced this pull request Feb 4, 2022

Merge pull request rust-lang#197 from cuviper/par_drain

4d6dde3

impl ParallelDrainRange

Get rid of the integer allocation #197

Get rid of the integer allocation #197

Uh oh!

Conversation

oli-obk commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Jun 19, 2017 •

edited

Loading

RalfJung Jun 19, 2017 •

edited

Loading

RalfJung Jun 19, 2017 •

edited

Loading

oli-obk commented Jun 21, 2017 •

edited

Loading