run-pass/coerce-expected-unsized.rs : jemalloc aborts in enable-debug builds

[pnkfelix]

Configure flags (I suspect --enable-debug is the only relevant one, though of course I recommend passing --enable-optimize for bootstrapping):

% ../configure --enable-debug --enable-optimize --enable-ccache --enable-clang --disable-optimize-tests --disable-llvm-assertions`

Steps to reproduce:

% ./x86_64-apple-darwin/stage1/bin/rustc  ../src/test/run-pass/coerce-expect-unsized.rs
% ./coerce-expect-unsized 
<jemalloc>: ../../../../src/jemalloc/src/jemalloc.c:2520: Failed assertion: "usize == isalloc(ptr, config_prof)"
Abort trap: 6
%

[pnkfelix]

git bisect indicates that this was injected by 3e313d9 (PR #36027); cc @eddyb

[eddyb]

I didn't fully check the validity of the rest of the code, I thought it was correct modulo initial starting data.
Maybe it doesn't round up to the struct's own alignment? Resulting in a smaller size passed to jemalloc.

[pnkfelix]

Stepping through a variant of the code, I'm certainly observing a discrepancy where we pass a small size to allocate and a larger size to deallocate.

[pnkfelix]

Code: https://is.gd/3BpBuR

some data from rr:

[...]

Breakpoint 4, alloc_jemalloc::__rust_allocate (size=128, align=8)
    at /home/fklock/Dev/Mozilla/rust.git/src/liballoc_jemalloc/lib.rs:98
(rr) finish
Run till exit from #0  alloc_jemalloc::__rust_allocate (size=128, align=8)
    at /home/fklock/Dev/Mozilla/rust.git/src/liballoc_jemalloc/lib.rs:98
0x000055d8f114849d in alloc::heap::allocate::h4983eab5bdfc0c37 ()
Value returned is $18 = (u8 *) 0x7fd2faa1b000 '\245' <repeats 128 times>
(rr) c
Continuing.

[...]

size: 24 val: 24
size: 32 val: 32
Dropping y

Breakpoint 3, alloc_jemalloc::__rust_deallocate (ptr=0x7fd2faa1b000 "", old_size=136,
    align=8) at /home/fklock/Dev/Mozilla/rust.git/src/liballoc_jemalloc/lib.rs:124
(rr)

In particular, the input to __rust_allocate is 128, while the old_size at deallocation is claimed to be 136.

[pnkfelix]

(note that the code I posted is not printing the actually relevant sizes, since it seems to me like our issue is with the size of the array in the RcBox, but that code is only printing the size of the Rc itself. Whoops.)

After fixing that, size_of_val reports 112 for the size of the RefCell<[u8]> and the RefCell<[u8; 101]>; of course the actually value being allocated is an RcBox<RefCell<[u8]>>, which adds an additional pair of Cell<usize> fields to the structure, so we end up with 112 + 16 = 128.

Not sure yet where the 136 is coming from; perhaps we are adding another 8 somewhere?

[pnkfelix]

On the other hand, manually breaking on core::mem::size_of_val (instantiated to RcBox<RefCell<[u8]>>), and walking through it indicates that it is definitely returning 136.

Here's the LLVM IR for the generated code for that routine:

; Function Attrs: inlinehint uwtable
define internal i64 @_ZN4core3mem11size_of_val17hd726290847184a20E(%"5.std::rc::RcBox<std::cell::RefCell<[u8]>>"* nonnull, i64) unnamed_addr #1 {
entry-block:
  %tmp_ret = alloca i64
  br label %start

start:                                            ; preds = %entry-block
  %2 = mul i64 %1, 1
  %3 = add i64 0, %2
  %4 = add i64 %3, 0
  %5 = add i64 8, %4
  %6 = add i64 %5, 7
  %7 = and i64 %6, -8
  %8 = add i64 24, %7
  %9 = add i64 %8, 7
  %10 = and i64 %9, -8
  store i64 %10, i64* %tmp_ret
  %11 = load i64, i64* %tmp_ret
  br label %bb1

bb1:                                              ; preds = %start
  ret i64 %11
}

(I haven't yet validated where its getting 8 and 24 from in that computation, but if nothing else:

(rr) p (((101 + 8 + 8-1) & -8) + 24 + 8-1) & -8
$47 = 136

[pnkfelix]

Hmm maybe we are double-counting the RefCell contribution when we calculate RcBox<RefCell<[u8]>> ?

[pnkfelix]

In particular, this line where we recurse:

        let last_field = def.struct_variant().fields.last().unwrap();
        let field_ty = monomorphize::field_ty(bcx.tcx(), substs, last_field);
        let (unsized_size, unsized_align) = size_and_align_of_dst(bcx, field_ty, info);

that is recurring on the last field that is immediately present in the struct, causing us to add 8. But the already computed prefix size (24 in this case) actually includes the prefix that is in that last field, I think

[eddyb]

@pnkfelix Does struct_tail instead of the last field work?
If the intention is to find the nested-most field, that is.
Although, I'm not entirely sure that would work well due to alignment.

Come to think of it, is this because min_size includes nested prefixes, and we don't want that?
It should be easy enough to completely ignore the last field, instead of variant.min_size() it'd be

let fields = variant.offset_after_field.len();
if fields >= 2 {
    variant.offset_after_field[fields - 2].bytes()
} else {
    0
}

[pnkfelix]

@eddyb yeah that sounds promising (that is, to not use variant.min_size() at all)

Update: Initial results look good, at least in terms of fixing this particular bug.