Issue 17220 - invalid code with -m32 -inline and struct that's size_t.sizeof x the size of an assigned enum value
Summary: invalid code with -m32 -inline and struct that's size_t.sizeof x the size of ...
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All Linux
: P1 major
Assignee: No Owner
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2017-02-24 02:23 UTC by Martin Nowak
Modified: 2017-03-12 21:59 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Martin Nowak 2017-02-24 02:23:43 UTC
cat > bug.d << CODE
void emitArithInstruction2(BCValue lhs)
{
    if (lhs.type != BCTypeEnum.i32) // type get's overwritten
        assert(0);
}

enum BCTypeEnum : ubyte
{
    Undef,
    i32 = 8, // value must be >= 8
}

struct BCValue // size must be 4 x BCTypeEnum.i32
{
    BCTypeEnum type; // position doesn't matter
    ubyte[4 * BCTypeEnum.i32 - type.sizeof] more;
}
static assert(BCValue.sizeof == 4 * BCTypeEnum.i32);

BCValue i32()
{
    BCValue result; // must be default 0 initialized
    result.type = BCTypeEnum.i32; // set value
    return result;
}

void main()
{
    auto val = i32();
    emitArithInstruction2(val);
}
CODE
dmd -m32 -inline -run bug
----
        mov     cl, 8                                   ; 0806B698 _ B1, 08
        mov     byte ptr [ebp-40H], cl                  ; 0806B69A _ 88. 4D, C0
        lea     esi, [ebp-40H]                          ; 0806B69D _ 8D. 75, C0
        lea     edi, [ebp-20H]                          ; 0806B6A0 _ 8D. 7D, E0
        cmp     byte ptr [ebp-20H], 8                   ; 0806B6A3 _ 80. 7D, E0, 08
        jz      ?_0170                                  ; 0806B6A7 _ 74, 07
        mov     al, 4                                   ; 0806B6A9 _ B0, 04
        call    _D3bug8__assertFiZv                     ; 0806B6AB _ E8, 00000028
----

The cmp instruction uses a wrong memory location for the struct.

----
Same with i32 = 10 and BCValue.sizeof == 40, so it's not dependent on 32 size of the struct, but below 32-byte struct initialization is done differently (and the bug vanishes).

        mov     cl, 10                                  ; 0806B698 _ B1, 0A
        mov     byte ptr [ebp-58H], cl                  ; 0806B69A _ 88. 4D, A8
        lea     esi, [ebp-58H]                          ; 0806B69D _ 8D. 75, A8
        lea     edi, [ebp-28H]                          ; 0806B6A0 _ 8D. 7D, D8
        cmp     byte ptr [ebp-28H], 10                  ; 0806B6A3 _ 80. 7D, D8, 0A
        jz      ?_0170                                  ; 0806B6A7 _ 74, 07

----

This reduced test-case is not reproducible with -O, but the original bug instance is.
Comment 1 Stefan Koch 2017-02-24 03:56:51 UTC
Happens on 64bit as well
Slightly modified:

void emitArithInstruction2(BCValue lhs)
{
    if (lhs.type != BCTypeEnum.i32) // type get's overwritten
        assert(0);
}

enum BCTypeEnum : ubyte
{
    Undef,
    i32 = 8, // value must be >= 8
}

struct BCValue // size must be size_t.sizeof x BCTypeEnum.i32
{
    BCTypeEnum type; // position doesn't matter
    ubyte[size_t.sizeof * BCTypeEnum.i32 - type.sizeof] more;
}
static assert(BCValue.sizeof == size_t.sizeof * BCTypeEnum.i32);

BCValue i32()
{
    BCValue result; // must be default 0 initialized
    result.type = BCTypeEnum.i32; // set value
    return result;
}

void main()
{
    auto val = i32();
    emitArithInstruction2(val);
}
CODE
Comment 2 Martin Nowak 2017-02-24 15:20:01 UTC
Ah thanks for size_t.sizeof times enum value finding.
This is easily the weirdest backend bug I've seen so far.
Comment 3 Martin Nowak 2017-02-24 17:13:21 UTC
The cmp runs on the `lhs` symbol, while only the `val` symbol get's initialized.
Not sure, but seems like the copy got elided.

Here is the assembly for when the size of BCValue != size_t.sizeof * BCTypeEnum.i32

        mov     cl, 8                                   ; 0018 _ B1, 08
        mov     byte ptr [rbp-78H], cl                  ; 001A _ 88. 4D, 88
        lea     rsi, [rbp-78H]                          ; 001D _ 48: 8D. 75, 88
        lea     rdi, [rbp-38H]                          ; 0021 _ 48: 8D. 7D, C8
        mov     cl, 7                                   ; 0025 _ B1, 07
        rep movsq                                       ; 0027 _ F3 48: A5
        cmp     byte ptr [rbp-38H], 8                   ; 002A _ 80. 7D, C8, 08
        jz      ?_003                                   ; 002E _ 74, 0A

Apparently the `mov cl, 8` instruction is a CSE in the bug case, no idea why the rep movsq get's omitted b/c of that though.
Comment 4 Martin Nowak 2017-02-24 17:51:38 UTC
This got accidentally fixed by https://github.com/dlang/dmd/pull/6410/files#diff-0630e1297bdecef97aded9af2ddb879cL4041.

Seems like the problem was indeed a CSE which caused that no code was generated for `mov cl, 8`, but the following `gen1(c3, 0xF3)` didn't deal with c3 being CNIL, hence the code wasn't appended/emitted.
Comment 5 github-bugzilla 2017-03-12 21:59:34 UTC
Commit pushed to newCTFE at https://github.com/dlang/dmd

https://github.com/dlang/dmd/commit/3dc0fa957ef362ba42779e3dd953931f9a908f45
workaround Issue #17220