Miri api refactor#50967

oli-obk · 2018-05-22T15:36:25Z

r? @eddyb

cc @Zoxc

based on #50916

eddyb · 2018-05-22T17:26:17Z

src/librustc/mir/interpret/value.rs

Should this be Pointer? Or is there a real usecase of ByRef pointing to a non-allocation?

yes, it's used to preserve zst identity

But this is Value, which means the address shouldn't be observable, only the value behind the pointer.

eddyb · 2018-05-22T17:26:58Z

src/librustc/mir/interpret/value.rs

Scalar::Undef comment outdated?

Doesn't seem changed to me.

eddyb · 2018-05-22T17:28:05Z

src/librustc/mir/interpret/value.rs

Should primval be replaced with scalar?

eddyb · 2018-05-22T18:46:46Z

src/librustc/mir/interpret/value.rs

You could at least wrap layout::Integer for signed and unsigned integers, if you want to keep ScalarKind at all (as opposed to reusing layout::Primitive).

eddyb · 2018-05-22T18:47:42Z

src/librustc/ty/sty.rs

I'm not sure this is a good idea...

and it's gone!

eddyb · 2018-05-22T18:52:36Z

src/librustc/mir/mod.rs

Since this is for printing, could you pass in 128? There are certainly 128 defined bits, trivially.

Oh this is used for sign extension o_O.

I will still be passing this as a Scalar, because the function is also called elsewhere with other variants, but I fixed the sign extension thing

eddyb · 2018-05-22T18:57:12Z

src/librustc_mir/interpret/eval_context.rs

Can't this use .to_value_with_len?

bors · 2018-05-23T14:35:16Z

☔ The latest upstream changes (presumably #50866) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2018-05-24T07:52:56Z

src/librustc/mir/mod.rs

Please call this bits, or, better, keep the name but do size.bits() as u8 at use sites.

eddyb · 2018-05-24T07:53:29Z

src/librustc/mir/mod.rs

Can you remove all the defined pattern-matching in here?

eddyb · 2018-05-24T07:53:48Z

src/librustc/mir/mod.rs

Also two more cases here.

In fact, this specific one can ignore even whether the value is a Value::Scalar.

eddyb · 2018-05-24T07:55:12Z

src/librustc/ty/layout.rs

Please use impl<'a, 'tcx> TyCtxt<'a, 'tcx, '_> if you don't want to name the lifetime.

I tried that, that doesn't compile

Doesn't it need a feature-gate? in_band_lifetimes I think?

eddyb · 2018-05-24T07:55:24Z

src/librustc/ty/layout.rs

eddyb · 2018-05-24T08:01:54Z

src/librustc_codegen_llvm/mir/constant.rs

Looking at this, this sort of thing probably needs to always use bitcast after LLVMConstPtrToInt, in case the latter is needed at all. You should add a test for an union used to convert a reference into a f32 / f64 (depending on #[cfg(target_pointer_width)]), which I highly suspect trips a LLVM assertion in the current state of the code.

eddyb · 2018-05-24T08:03:37Z

src/librustc_mir/hair/cx/mod.rs

Can't this use some truncate function from miri?

won't fix in this PR, there's an issue open for cleaning up this kind of code: #49937

eddyb · 2018-05-24T08:04:24Z

src/librustc_mir/hair/cx/mod.rs

Can't this use with_len or something?

eddyb · 2018-05-24T08:05:08Z

src/librustc_mir/hair/cx/mod.rs

Is this a true clamping operation (i.e. saturating), or a truncation? I actually expect truncation.

eddyb · 2018-05-24T08:07:11Z

src/librustc_mir/hair/pattern/mod.rs

Note that this unwrap can fail - we should be matching on Option<Ordering> below and explicitly list all the possible cases - currently the _ success at the end of the match would catch None which would be bad.

eddyb · 2018-05-24T08:09:10Z

src/librustc_mir/hair/pattern/mod.rs

You're comparing defined again. At most these should <= relationships, not ==.

That is, each of defined_a and defined_b should be larger or equal to pointer_size.bits() - why not use to_bits with usize instead?

eddyb · 2018-05-24T08:11:30Z

src/librustc_mir/hair/pattern/mod.rs

Would be better to cast alloc_a.bytes.len() to u128.

eddyb · 2018-05-24T08:21:13Z

src/librustc_mir/interpret/memory.rs

This seems wrong, shouldn't be <?

Also, please cast the small type to the large one for comparisons.

eddyb · 2018-05-24T08:22:22Z

src/librustc_mir/interpret/memory.rs

It's a truncation, shouldn't be necessary nowadays.

eddyb · 2018-05-24T08:25:12Z

src/librustc_mir/interpret/memory.rs

I'd write size.bits() != 0 here instead.

eddyb · 2018-05-24T08:29:47Z

src/librustc_mir/interpret/memory.rs

How is it possible to get a Value::Scalar(Scalar::undef()) for a type with an Abi::ScalarPair?

by reading a fat pointer from undefined bytes I'd presume

That should produce a ScalarPair with two undefs.

eddyb · 2018-05-24T08:31:34Z

src/librustc_target/abi/mod.rs

For the record, I disagree with this, in general rustc's typesystem should not include rustc_target::abi types.

oli-obk · 2018-05-25T10:09:29Z

@bors r=eddyb

bors · 2018-05-25T10:09:30Z

📌 Commit 50d3783 has been approved by eddyb

bors · 2018-05-25T12:14:50Z

⌛ Testing commit 50d3783 with merge 53fc7286e4ea16cc5909e0ad96d16e714501132c...

bors · 2018-05-25T13:07:08Z

💔 Test failed - status-appveyor

rust-highfive · 2018-05-25T13:08:27Z

The job armhf-gnu of 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.

[00:50:58] 
[00:50:58] ---- [ui] ui/const-eval/ref_to_int_match.rs stdout ----
[00:50:58] diff of stderr:
[00:50:58] 
[00:50:58] - error[E0030]: lower range bound must be less than or equal to upper
[00:50:58] + error[E0308]: mismatched types
[00:50:58] 2   --> $DIR/ref_to_int_match.rs:14:9
[00:50:58] 3    |
[00:50:58] 4 LL |         10...BAR => {}, //~ ERROR lower range bound must be less than or equal to upper
[00:50:58] 
[00:50:58] -    |         ^^ lower bound larger than upper bound
[00:50:58] +    |         ^^^^^^^^ expected u64, found u32
[00:50:58] 7 error: aborting due to previous error
[00:50:58] 8 
[00:50:58] 
[00:50:58] - For more information about this error, try `rustc --explain E0030`.
[00:50:58] - For more information about this error, try `rustc --explain E0030`.
[00:50:58] + For more information about this error, try `rustc --explain E0308`.
[00:50:58] 10 
[00:50:58] 
[00:50:58] 
[00:50:58] The actual stderr differed from the expected stderr.
[00:50:58] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/const-eval/ref_to_int_match/ref_to_int_match.stderr
[00:50:58] To update references, rerun the tests and pass the `--bless` flag
[00:50:58] To only update this specific test, also pass `--test-args const-eval/ref_to_int_match.rs`
[00:50:58] error: 1 errors occurred comparing output.
[00:50:58] status: exit code: 101
[00:50:58] status: exit code: 101
[00:50:58] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/const-eval/ref_to_int_match.rs" "--target=arm-unknown-linux-gnueabihf" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/const-eval/ref_to_int_match/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/arm-unknown-linux-gnueabihf/native/rust-test-helpers" "-Clinker=arm-linux-gnueabihf-gcc" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/const-eval/ref_to_int_match/auxiliary" "-A" "unused"
[00:50:58] ------------------------------------------
[00:50:58] 
[00:50:58] ------------------------------------------
[00:50:58] stderr:
[00:50:58] stderr:
[00:50:58] ------------------------------------------
[00:50:58] {"message":"mismatched types","code":{"code":"E0308","explanation":"\nThis error occurs when the compiler was unable to infer the concrete type of a\nvariable. It can occur for several cases, the most common of which is a\nmismatch in the expected type that the compiler inferred for a variable's\ninitializing expression, and the actual type explicitly assigned to the\nvariable.\n\nFor example:\n\n```compile_fail,E0308\nlet x: i32 = \"I am not a number!\";\n//     ~~~   ~~~~~~~~~~~~~~~~~~~~\n//      |             |\n//      |    initializing expression;\n//      |    compiler infers type `&str`\n//      |\n//    type `i32` assigned to variable `x`\n```\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/const-eval/ref_to_int_match.rs","byte_start":527,"byte_end":535,"line_start":14,"line_end":14,"column_start":9,"column_end":17,"is_primary":true,"text":[{"text":"        10...BAR => {}, //~ ERROR lower range bound must be less than or equal to upper","highlight_start":9,"highlight_end":17}],"label":"expected u64, found u32","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0308]: mismatched types\n  --> /checkout/src/test/ui/const-eval/ref_to_int_match.rs:14:9\n   |\nLL |         10...BAR => {}, //~ ERROR lower range bound must be less than or equal to upper\n   |         ^^^^^^^^ expected u64, found u32\n\n"}
[00:50:58] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[00:50:58] {"message":"For more information about this error, try `rustc --explain E0308`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0308`.\n"}
[00:50:58] ------------------------------------------
[00:50:58] 
[00:50:58] thread '[ui] ui/const-eval/ref_to_int_match.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3053:9
[00:50:58] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:50:58] 
[00:50:58] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[00:50:58] 
[00:50:58] 
[00:50:58] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/arm-unknown-linux-gnueabihf/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-arm-unknown-linux-gnueabihf" "--mode" "ui" "--target" "arm-unknown-linux-gnueabihf" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--linker" "arm-linux-gnueabihf-gcc" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/arm-unknown-linux-gnueabihf/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--llvm-version" "6.0.1\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--remote-test-client" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:50:58] 
[00:50:58] 
[00:50:58] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target arm-unknown-linux-gnueabihf
[00:50:58] Build completed unsuccessfully in 0:48:01

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-05-25T13:14:31Z

@bors r=eddyb

bors · 2018-05-25T13:14:32Z

📌 Commit 5f599bb has been approved by eddyb

bors · 2018-05-25T13:59:55Z

⌛ Testing commit 5f599bb with merge 990d8aa...

@eddyb

Miri api refactor r? @eddyb cc @Zoxc based on #50916

bors · 2018-05-25T16:09:59Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 990d8aa to master...

bjorn3 · 2018-05-30T14:08:11Z

src/librustc_mir/interpret/eval_context.rs

                let place = self.eval_place(place)?;
                let discr_val = self.read_discriminant_value(place, ty)?;
-                self.write_primval(dest, PrimVal::Bytes(discr_val), dest_ty)?;
+                let defined = self.layout_of(ty).unwrap().size.bits() as u8;


I believe this causes #51086. ty is the enum of which the discriminant is taken. This should have been dest_ty

@oli-obk

rustc_target: inline abi::FloatTy into abi::Primitive. This effectively undoes a small part of @oli-obk's rust-lang#50967, now that the rest of the compiler doesn't use the `FloatTy` definition from `rustc_target`, post-rust-lang#65884.

rust-highfive assigned eddyb May 22, 2018

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

eddyb reviewed May 22, 2018

View reviewed changes

oli-obk force-pushed the miri_api_refactor branch 3 times, most recently from 85638ee to ca34069 Compare May 23, 2018 16:45