[MIR] Handle call return values that need to be casted properly. #34054

luqmana · 2016-06-03T01:39:53Z

eefriedman · 2016-06-03T03:48:13Z

src/librustc_trans/mir/block.rs

+                    // does a `PointerCast` and a `Store`. Unfortunately,
+                    // that may run afoul of strict aliasing rules, leading
+                    // to wacky results after optimization since LLVM
+                    // may just replace it with an `undef`.


The problem has nothing to do with aliasing rules... the problem is the buffer overflow.

I'd prefer copying the comment from callee.

eddyb · 2016-06-03T04:42:43Z

LGTM but to be sure r? @dotdash

dotdash · 2016-06-03T09:17:39Z

Seems ok except for the misleading comment. But you might actually want to extract that into some function (not sure if store is the right candidate), because we still need a similar for passing arguments to C ABI functions (either that or tighter type calculations for C ABI functions).

For example:

#![crate_type="lib"]
struct S {
    x: i8,
    y: i8,
}

extern {
    fn foo(s: S);
}

pub fn bla() {
    unsafe {
        foo(S { x: 4, y: 8 });
    }
}

Results in:

define void @_ZN3ffi3bla17h4acc14686285caf0E() unnamed_addr #0 {
entry-block:
  %temp0 = alloca {}
  %temp1 = alloca %S
  %arg = alloca %S
  br label %start

start:                                            ; preds = %entry-block
  %0 = getelementptr inbounds %S, %S* %temp1, i32 0, i32 0
  store i8 4, i8* %0
  %1 = getelementptr inbounds %S, %S* %temp1, i32 0, i32 1
  store i8 8, i8* %1
  %2 = load %S, %S* %temp1
  store %S %2, %S* %arg
  %3 = bitcast %S* %arg to { i64 }*
  %4 = load { i64 }, { i64 }* %3, align 1
  call void @foo({ i64 } %4)
  br label %bb2

end:                                              ; preds = %bb3
  ret void

bb2:                                              ; preds = %start
  br label %bb3

bb3:                                              ; preds = %bb2
  br label %end
}

Notice how we try to load 8 bytes out of a 2 byte allocation.

Also, we perform FCA loads/store :-( (that's unrelated to this PR though).

eddyb · 2016-06-04T04:48:18Z


failures:

---- [codegen] codegen/stores.rs stdout ----

error: verification with 'FileCheck' failed
status: exit code: 1
command: /usr/lib/llvm-3.7/bin/FileCheck -input-file=x86_64-unknown-linux-gnu/test/codegen/stores.ll /build/src/test/codegen/stores.rs
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
/build/src/test/codegen/stores.rs:29:11: error: expected string not found in input
// CHECK: store i32 %{{.*}}, i32* %{{.*}}, align 1
          ^
x86_64-unknown-linux-gnu/test/codegen/stores.ll:8:35: note: scanning from here
define void @small_array_alignment([4 x i8]* dereferenceable(4), i32) unnamed_addr #0 {
                                  ^
x86_64-unknown-linux-gnu/test/codegen/stores.ll:15:2: note: possible intended match here
 store i32 %1, i32* %fn_ret_cast
 ^
/build/src/test/codegen/stores.rs:40:11: error: expected string not found in input
// CHECK: store i32 %{{.*}}, i32* %{{.*}}, align 1
          ^
x86_64-unknown-linux-gnu/test/codegen/stores.ll:44:36: note: scanning from here
define void @small_struct_alignment(%Bytes* dereferenceable(4), i32) unnamed_addr #0 {
                                   ^
x86_64-unknown-linux-gnu/test/codegen/stores.ll:51:2: note: possible intended match here
 store i32 %1, i32* %fn_ret_cast
 ^

r=me with this test failure fixed

luqmana · 2016-06-05T04:27:50Z

@bors r=eddyb

bors · 2016-06-05T04:27:50Z

📌 Commit d14c5cd has been approved by eddyb

bors · 2016-06-05T10:18:35Z

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

eddyb · 2016-06-05T12:16:23Z

FWIW I've included this in #33905, with some extra logic to also work for the generalized Pair operands.
If that doesn't get merged first, the additions are in a separate commit, so it should work out either way.

@Aatch

[MIR] Implement overflow checking The initial set of changes is from @Aatch's #33255 PR, rebased on master, plus: Added an `Assert` terminator to MIR, to simplify working with overflow and bounds checks. With this terminator, error cases can be accounted for directly, instead of looking for lang item calls. It also keeps the MIR slimmer, with no extra explicit blocks for the actual panic calls. Warnings can be produced when the `Assert` is known to always panic at runtime, e.g.: ```rust warning: index out of bounds: the len is 1 but the index is 3 --> <anon>:1:14 1 |> fn main() { &[std::io::stdout()][3]; } |> ^^^^^^^^^^^^^^^^^^^^^^ ``` Generalized the `OperandValue::FatPtr` optimization to any aggregate pair of immediates. This allows us to generate the same IR for overflow checks as old trans, not something worse. For example, addition on `i16` calls `llvm.sadd.with.overflow.i16`, which returns `{i16, i1}`. However, the Rust type `(i16, bool)`, has to be `{i16, i8}`, only an immediate `bool` is `i1`. But if we split the pair into an `i16` and an `i1`, we can pass them around as such for free. The latest addition is a rebase of #34054, updated to work for pairs too. Closes #34054, fixes #33873. Last but not least, the `#[rustc_inherit_overflow_checks]` attribute was introduced to control the overflow checking behavior of generic or `#[inline]` functions, when translated in another crate. It is **not** intended to be used by crates other than `libcore`, which is in the unusual position of being distributed as only an optimized build with no checks, even when used from debug mode. Before MIR-based translation, this worked out fine, as the decision for overflow was made at translation time, in the crate being compiled, but MIR stored in `rlib` has to contain the checks. To avoid always generating the checks and slowing everything down, a decision was made to use an attribute in the few spots of `libcore` that need it (see #33255 for previous discussion): * `core::ops::{Add, Sub, Mul, Neg, Shl, Shr}` implementations for integers, which have `#[inline]` methods and can be used in generic abstractions from other crates * `core::ops::{Add, Sub, Mul, Neg, Shl, Shr}Assign` same as above, for augmented assignment * `pow` and `abs` methods on integers, which intentionally piggy-back on built-in multiplication and negation, respectively, to get overflow checks * `core::iter::{Iterator, Chain, Peek}::count` and `core::iter::Enumerate::{next, nth}`, also documented as panicking on overflow, from addition, counting elements of an iterator in an `usize`

rust-highfive assigned eddyb Jun 3, 2016

eefriedman reviewed Jun 3, 2016
View reviewed changes

rust-highfive assigned dotdash and unassigned eddyb Jun 3, 2016

luqmana force-pushed the 33873-mir_cast_fn_ret branch from fbd57ee to 8434bd7 Compare June 3, 2016 14:57

[MIR] Handle call return values that need to be casted properly.

d14c5cd

luqmana force-pushed the 33873-mir_cast_fn_ret branch from 8434bd7 to d14c5cd Compare June 5, 2016 04:22

eddyb mentioned this pull request Jun 5, 2016

[MIR] Implement overflow checking #33905

Merged

bors closed this in #33905 Jun 5, 2016

luqmana deleted the 33873-mir_cast_fn_ret branch July 8, 2016 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIR] Handle call return values that need to be casted properly. #34054

[MIR] Handle call return values that need to be casted properly. #34054

luqmana commented Jun 3, 2016

eefriedman Jun 3, 2016

eddyb Jun 3, 2016

eddyb commented Jun 3, 2016

dotdash commented Jun 3, 2016

eddyb commented Jun 4, 2016

luqmana commented Jun 5, 2016

bors commented Jun 5, 2016

bors commented Jun 5, 2016

eddyb commented Jun 5, 2016 •

edited

Loading

[MIR] Handle call return values that need to be casted properly. #34054

[MIR] Handle call return values that need to be casted properly. #34054

Conversation

luqmana commented Jun 3, 2016

eefriedman Jun 3, 2016

Choose a reason for hiding this comment

eddyb Jun 3, 2016

Choose a reason for hiding this comment

eddyb commented Jun 3, 2016

dotdash commented Jun 3, 2016

eddyb commented Jun 4, 2016

luqmana commented Jun 5, 2016

bors commented Jun 5, 2016

bors commented Jun 5, 2016

eddyb commented Jun 5, 2016 • edited Loading

eddyb commented Jun 5, 2016 •

edited

Loading