rustup to rustc 1.17.0-nightly (60a0edc6c 2017-02-26) #147

oli-obk · 2017-02-28T11:35:13Z

We can now turn non-capturing closures into function pointers (and then back into Fn trait objects)

Diff best viewed without whitespace changes

eddyb · 2017-02-28T11:37:17Z

src/memory.rs

+            // FIXME: why doesn't this compile?
+            //sig: tcx.erase_late_bound_regions(&fn_ty.sig),
+            sig: fn_ty.sig.skip_binder(),
+        }))


I'm not sure this is the way to go. Did you see how this was implemented in librustc_trans?
That is, it relies on an existing shim- oh, drat, you have that extra argument to take care of, nevermind me!

oli-obk · 2017-03-02T12:12:40Z

Added rustup to @eddyb's PR of ~~doom~~ awesomeness

eddyb · 2017-03-02T12:13:40Z

src/memory.rs

-        format!("extern {} ", fn_def.abi)
-    };
-    format!("function pointer: {}: {}{}", name, abi, fn_def.sig)
+    format!("function pointer: {}: {}", name, fn_def.sig.skip_binder())


I don't think skipping the binder is needed here at all!

yes it is :) There's no Display impl for Binder

Oh, right, it's Debug only - should still do the right thing though.

eddyb · 2017-03-02T12:15:56Z

src/eval_context.rs

@@ -196,7 +194,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {

            Struct(_)    => unimplemented!(),
            Tuple(_)     => unimplemented!(),
-            Function(_)  => unimplemented!(),
+            Function(_, _)  => unimplemented!(),


Tempted to make this the way to refer to a TyFnDef (now that it has Substs), what do you think?

it's unreachable ^^ At least in the run-pass tests that we run. We only use this function in mir::Literal::Value, and I think that can only contain primitives

Oh, I know, it's because we use Item right now, and work around the awkwardness of it everywhere.

Oh... you mean changing it that Literal::Item stops referring to functions. Sounds good.

solson

Sorry for the delay!

solson · 2017-03-13T20:38:46Z

src/lvalue.rs

@@ -137,9 +137,9 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
            Local(mir::RETURN_POINTER) => self.frame().return_lvalue,
            Local(local) => Lvalue::Local { frame: self.stack.len() - 1, local, field: None },

-            Static(def_id) => {
+            Static(ref statik) => {


s/statik/static_/

I prefer to use a deterministic keyword renaming (append _) rather than coming up with weird unique replacements.

sgtm, I was just following the style seen in rustc (krate)

solson · 2017-03-13T20:38:56Z

src/step.rs

@@ -242,7 +242,8 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for ConstantExtractor<'a, 'b, 'tcx> {
        location: mir::Location
    ) {
        self.super_lvalue(lvalue, context, location);
-        if let mir::Lvalue::Static(def_id) = *lvalue {
+        if let mir::Lvalue::Static(ref statik) = *lvalue {


s/statik/static_/

solson · 2017-03-13T20:48:51Z

src/terminator/mod.rs

-                            return Err(EvalError::FunctionPointerTyMismatch(abi, sig, bare_fn_ty));
+                        match fn_def {
+                            Function::Concrete(fn_def) => {
+                                // transmuting function pointers in miri is fine as long as the number of


Even if we check ABI, number of arguments, and argument and return type sizes, we might still not match true target calling convention behaviour because of weird differences between, for example, T and struct Foo { x: T }. I think maybe we should be more conservative and prevent the calling of transmuted function pointers.

Specifically, error out at call time when the call-site function type doesn't match the type of the function actually being dynamically called, since Miri can remember that. We should still allow round-tripping transmutes that never call a fn ptr with the wrong type, just like we allow round trips with other transmutes.

Differences like unsafe vs. non-unsafe should be fine, though. Anything that clearly doesn't affect the calling convention.

yea... I tried to do that check, but since we also don't care about lifetime changes I tried to erase the lifetimes, which kept causing rustc to panic in weird corner cases.

I'll change the FIXME text

oh... I remember sth else. Rustc tests whether we can transmute fn foo(*const i32) to fn foo(Option<&i32>) (or maybe the other direction).

I'd prefer to be more conservative than rustc about function pointer casts until a more formal definition comes along, unless it becomes essential for evaluating std code.

reran on rust test suite:

1 tried to call a function with sig fn(&[u8]) -> std::option::Option<u8> through a function pointer of type fn(&[u8]) -> std::option::Option<u8> 1 tried to call a function with sig fn(&A<'_>) through a function pointer of type fn(&A) 1 tried to call a function with sig extern "C" fn(usize) -> Foo through a function pointer of type extern "C" fn(usize) -> u32 1 tried to call a function with sig extern "C" fn(&isize) -> std::option::Option<&isize> through a function pointer of type extern "C" fn(&isize) -> &isize

fn(&[u8]) -> std::option::Option<u8>
fn(&[u8]) -> std::option::Option<u8>

fn(&A<'_>)
fn(&A)

extern "C" fn(usize) -> Foo
extern "C" fn(usize) -> u32

extern "C" fn(&isize) -> std::option::Option<&isize>
extern "C" fn(&isize) -> &isize

Of these, I think we should make sure 1 and 2 work (ignore lifetimes). I don't care about supporting 3 and 4 for now.

solson · 2017-03-14T10:09:47Z

src/eval_context.rs

+    pub fn erase_lifetimes<T>(self, value: &Binder<T>) -> T
+        where T : TypeFoldable<'tcx>
+    {
+        let value = self.tcx.erase_late_bound_regions(value)


Missing semicolon here.

oli-obk · 2017-03-14T11:18:33Z

fixed

rustup to rustc 1.17.0-nightly (60a0edc6c 2017-02-26)

8878a40

eddyb reviewed Feb 28, 2017

View reviewed changes

Rustup to rustc 1.17.0-nightly (be760566c 2017-02-28)

8405770

eddyb reviewed Mar 2, 2017

View reviewed changes

oli-obk added 2 commits March 7, 2017 12:49

Rustup to rustc 1.17.0-nightly (b1e31766d 2017-03-03)

41d59b1

Rustup to rustc 1.17.0-nightly (824c9ebbd 2017-03-12)

4cb1f63

solson reviewed Mar 13, 2017

View reviewed changes

solson mentioned this pull request Mar 14, 2017

implement the write_bytes() intrinsic #148

Merged

oli-obk added 2 commits March 14, 2017 09:23

Use deterministic keyword renaming (append _)

64d196a

I say we take off and nuke the lifetimes from orbit

da6f136

solson reviewed Mar 14, 2017

View reviewed changes

Add a method that hides the lifetime erasing boilerplate

adb3fbb

oli-obk force-pushed the rustup branch from 3369494 to adb3fbb Compare March 14, 2017 10:13

Fix warnings in unit test

80be25e

solson merged commit 6ee8595 into rust-lang:master Mar 14, 2017

oli-obk deleted the rustup branch March 17, 2017 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustup to rustc 1.17.0-nightly (60a0edc6c 2017-02-26) #147

rustup to rustc 1.17.0-nightly (60a0edc6c 2017-02-26) #147

oli-obk commented Feb 28, 2017 •

edited

Loading

eddyb Feb 28, 2017

oli-obk commented Mar 2, 2017

eddyb Mar 2, 2017

oli-obk Mar 2, 2017 •

edited

Loading

eddyb Mar 2, 2017

eddyb Mar 2, 2017

oli-obk Mar 2, 2017

eddyb Mar 2, 2017

oli-obk Mar 2, 2017

solson left a comment

solson Mar 13, 2017

oli-obk Mar 14, 2017

solson Mar 13, 2017

solson Mar 13, 2017

oli-obk Mar 14, 2017

oli-obk Mar 14, 2017

solson Mar 14, 2017

oli-obk Mar 14, 2017

solson Mar 14, 2017 •

edited

Loading

solson Mar 14, 2017

oli-obk commented Mar 14, 2017

rustup to rustc 1.17.0-nightly (60a0edc6c 2017-02-26) #147

rustup to rustc 1.17.0-nightly (60a0edc6c 2017-02-26) #147

Conversation

oli-obk commented Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

oli-obk commented Mar 2, 2017

Choose a reason for hiding this comment

oli-obk Mar 2, 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

Choose a reason for hiding this comment

solson left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solson Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 14, 2017

oli-obk commented Feb 28, 2017 •

edited

Loading

oli-obk Mar 2, 2017 •

edited

Loading

solson Mar 14, 2017 •

edited

Loading