Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Beta segfault regression #37222

Closed
alexcrichton opened this issue Oct 16, 2016 · 6 comments · Fixed by #37281
Closed

Beta segfault regression #37222

alexcrichton opened this issue Oct 16, 2016 · 6 comments · Fixed by #37281
Labels
I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Running this code in either release or debug mode will yield a segfault:

#[derive(Debug)]
pub struct Binding {
    action: Action,
    mode: u32,
}

#[derive(Debug)]
pub enum Action {
    Esc(&'static str),
    Paste,
    Char(char)
}

static V_BINDINGS: &'static [Binding] = &[
    Binding { action: Action::Paste, mode: 0 },
    Binding { action: Action::Char('v'), mode: 1 },
];

fn main() {
    println!("{:?}", V_BINDINGS);
}

It looks like this also runs successfully on stable, just failing on beta/nightly

cc @jwilm

@alexcrichton alexcrichton added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Oct 16, 2016
@TimNN
Copy link
Contributor

TimNN commented Oct 16, 2016

This looks very similar to #36401.

@TimNN
Copy link
Contributor

TimNN commented Oct 16, 2016

This was introduced between nightly-2016-09-22 and nightly-2016-09-27 (Changes).

I'm no llvm ir expert, however I believe the following excerpts show the problem:

; GOOD
%Binding = type { %Action, i8 }
%Action = type { i8, [7 x i8], [2 x i64] }

@_ZN5newsf10V_BINDINGS17h3669347bdb3d8fc8E = internal constant { %Binding*, i64 } { %Binding* bitcast ({
    { { i8, [23 x i8] }, i8, [7 x i8] },
    { { i8, [23 x i8] }, i8, [7 x i8] }
}* @ref8290 to %Binding*), i64 2 }, align 8


; BAD
%Binding = type { %Action, i8 }
%Action = type { i32, [1 x i32], [2 x i64] }

@_ZN5newsf10V_BINDINGS17h490c5b52b95657e8E = internal constant { %Binding*, i64 } { %Binding* bitcast ({
    { { i32, [20 x i8] }, i8 },
    { { i32, [20 x i8] }, i8 }
}* @ref8293 to %Binding*), i64 2 }, align 8

The definition in the BAD case is missing the [7 x i8] padding between the slice elements.

Edit: Note that this ir was obtained from a slightly modified code, the V_BINDINGS definition was changed to

static V_BINDINGS: &'static [Binding] = &[
    Binding { action: Action::Paste, mode: 0 },
    Binding { action: Action::Paste, mode: 1 },
];

And the struct changed to

pub struct Binding {
    action: Action,
    mode: u8, // <-- Change is here
}

Also, some manually edited Debug impl's revealed that (in the modified case) the first Binding is correctly handled as a Paste, however the second is Esc (which then leads to the segfault when trying read the string literal from an invalid address.

@TimNN
Copy link
Contributor

TimNN commented Oct 17, 2016

The following code reproduces the problem without the segfault:

#![feature(core_intrinsics)]

use std::{mem, intrinsics};

pub struct Binding {
    action: Action,
    mode: u8,
}

pub enum Action {
    Esc(i64),
    Char(u32),
    Paste,
}

static V_BINDINGS: &'static [Binding] = &[
    Binding { action: Action::Paste, mode: 0xff },
    Binding { action: Action::Paste, mode: 0xff },
];

fn main() {
    let first = unsafe { intrinsics::discriminant_value(&V_BINDINGS[0].action) };
    let second = unsafe { intrinsics::discriminant_value(&V_BINDINGS[1].action) };

    // Edit: added the next three asserts.
    assert_eq!(mem::size_of::<Action>(), 16);
    assert_eq!(mem::size_of::<Binding>(), 24);
    assert_eq!(&V_BINDINGS[1] as *const _ as usize - &V_BINDINGS[0] as *const _ as usize, 24); 

    // Edit: added hexdump
    let start = &V_BINDINGS[0] as *const _ as *const u8;
    for i in 0 .. 48 {
        print!("{:02X} ", unsafe { *start.offset(i) });
        if i % 24 == 23 { println!(""); }
    }

    assert_eq!(first, 2);
    assert_eq!(second, 2); // <-- This fails
}

The expected hexdump would be (from the working nightly):

02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 FF 00 00 00 00 00 00 00
02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 FF 00 00 00 00 00 00 00

The actual hexdump is:

02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 FF 00 00 00 02 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 FF 00 00 00 73 65 67 32 2E 72 73 00

@brson brson added I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. P-high High priority labels Oct 20, 2016
@brson
Copy link
Contributor

brson commented Oct 20, 2016

Waiting on backport.

bors added a commit that referenced this issue Oct 21, 2016
trans: pad const structs to aligned size

Fixes #37222.

I'm not sure if that is the *correct* way to fix this, but it *does* work.
bors added a commit that referenced this issue Oct 22, 2016
trans: pad const structs to aligned size

Fixes #37222.

I'm not sure if that is the *correct* way to fix this, but it *does* work.
@TimNN
Copy link
Contributor

TimNN commented Oct 22, 2016

Reopened because this still affects beta.

@alexcrichton
Copy link
Member Author

Backported in #37344, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants