D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 13952 - [REG2.067a] change in struct ctor lowering triggers codegen bug
Summary: [REG2.067a] change in struct ctor lowering triggers codegen bug
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P1 regression
Assignee: No Owner
URL:
Keywords: pull, wrong-code
Depends on:
Blocks:
 
Reported: 2015-01-08 10:41 UTC by Martin Nowak
Modified: 2015-03-11 08:41 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 2015-01-08 10:41:21 UTC
It's still a bit unclear what exactly causes the bug.
I was able to bisect an error in Higgs unittests down to this change.

https://github.com/D-Programming-Language/dmd/pull/3885#issuecomment-69158463
Comment 1 Walter Bright 2015-01-31 22:27:55 UTC
Please provide a reproducible example.
Comment 2 Martin Nowak 2015-02-01 03:57:30 UTC
(In reply to Walter Bright from comment #1)
> Please provide a reproducible example.

I already spend almost a day on this and couldn't reduce it to less than compiling Higgs and running the tests. Will try again, but it's definitely cause by the mentioned commit.
Comment 3 Martin Nowak 2015-02-02 01:24:27 UTC
After spending yet another day on this :(, I finally found it.

It's a bug in Higgs that was triggered by the change to the struct literal code.
https://github.com/higgsjs/Higgs/pull/177

I'm wondering why this worked before.
Was the struct previously fully initialized before calling the ctor, or did this just work by accidentally?

Anyhow, I filed bug 14107 because this should never have compiled.
Comment 4 Martin Nowak 2015-02-05 23:34:24 UTC
struct X86Imm
{
    ulong imm;
}

struct X86Opnd
{
    union
    {
        X86Reg reg;
        X86Imm imm;
    }

    ubyte tag;

    this(X86Reg r) { reg = r; }
}

struct X86Reg
{
    /// Register type
    ubyte type;

    /// Register index number
    ubyte regNo;

    /// Size in bits
    ushort size;
}

X86Opnd opnd(X86Reg reg)
{
    return X86Opnd(reg);
}

Here is the asm that for the opnd function with 2.066.1

_D4bug24opndFS4bug26X86RegZS4bug27X86Opnd PROC
        push    rbp                                     ; 0000 _ 55
        mov     rbp, rsp                                ; 0001 _ 48: 8B. EC
        sub     rsp, 32                                 ; 0004 _ 48: 83. EC, 20
        lea     rax, [rbp-20H]                          ; 0008 _ 48: 8D. 45, E0
        xor     rcx, rcx                                ; 000C _ 48: 31. C9
        mov     qword ptr [rax], rcx                    ; 000F _ 48: 89. 08
        mov     qword ptr [rax+8H], rcx                 ; 0012 _ 48: 89. 48, 08
        mov     rsi, rdi                                ; 0016 _ 48: 89. FE
        mov     rdi, rax                                ; 0019 _ 48: 89. C7
        call    _D4bug27X86Opnd6__ctorMFNcS4bug26X86RegZS4bug27X86Opnd; 001C _ E8, 00000000(rel)
        mov     rdx, qword ptr [rax+8H]                 ; 0021 _ 48: 8B. 50, 08
        mov     rax, qword ptr [rax]                    ; 0025 _ 48: 8B. 00
        mov     rsp, rbp                                ; 0028 _ 48: 8B. E5
        pop     rbp                                     ; 002B _ 5D
        ret                                             ; 002C _ C3
_D4bug24opndFS4bug26X86RegZS4bug27X86Opnd ENDP

That's the same function now.

_D4bug24opndFS4bug26X86RegZS4bug27X86Opnd PROC
        push    rbp                                     ; 0000 _ 55
        mov     rbp, rsp                                ; 0001 _ 48: 8B. EC
        sub     rsp, 32                                 ; 0004 _ 48: 83. EC, 20
        xor     eax, eax                                ; 0008 _ 31. C0
        mov     dword ptr [rbp-18H], eax                ; 000A _ 89. 45, E8
        mov     dword ptr [rbp-14H], eax                ; 000D _ 89. 45, EC
        xor     ecx, ecx                                ; 0010 _ 31. C9
        mov     byte ptr [rbp-10H], cl                  ; 0012 _ 88. 4D, F0
        mov     byte ptr [rbp-0FH], cl                  ; 0015 _ 88. 4D, F1
        mov     word ptr [rbp-0EH], ax                  ; 0018 _ 66: 89. 45, F2
        mov     edx, dword ptr [rbp-10H]                ; 001C _ 8B. 55, F0
        mov     dword ptr [rbp-20H], edx                ; 001F _ 89. 55, E0
        mov     byte ptr [rbp-18H], cl                  ; 0022 _ 88. 4D, E8
        mov     rsi, rdi                                ; 0025 _ 48: 89. FE
        lea     rdi, [rbp-20H]                          ; 0028 _ 48: 8D. 7D, E0
        call    _D4bug27X86Opnd6__ctorMFNcS4bug26X86RegZS4bug27X86Opnd; 002C _ E8, 00000000(rel)
        mov     rdx, qword ptr [rax+8H]                 ; 0031 _ 48: 8B. 50, 08
        mov     rax, qword ptr [rax]                    ; 0035 _ 48: 8B. 00
        mov     rsp, rbp                                ; 0038 _ 48: 8B. E5
        pop     rbp                                     ; 003B _ 5D
        ret                                             ; 003C _ C3
_D4bug24opndFS4bug26X86RegZS4bug27X86Opnd ENDP

Just focusing on the X86Opnd init code

        lea     rax, [rbp-20H]
        xor     rcx, rcx
        mov     qword ptr [rax], rcx
        mov     qword ptr [rax+8H], rcx

In the new code the 4 bytes at [rbp-1CH] are never initialized.

        xor     eax, eax
        mov     dword ptr [rbp-18H], eax
        mov     dword ptr [rbp-14H], eax
        xor     ecx, ecx
        mov     byte ptr [rbp-10H], cl
        mov     byte ptr [rbp-0FH], cl
        mov     word ptr [rbp-0EH], ax
        mov     edx, dword ptr [rbp-10H]
        mov     dword ptr [rbp-20H], edx
        mov     byte ptr [rbp-18H], cl
        mov     rsi, rdi
        lea     rdi, [rbp-20H]


Without a constructor it's the same for both dmd versions and also only  initializes the first union field of X86Opnd.

        xor     eax, eax
        mov     dword ptr [rbp-18H], eax
        mov     dword ptr [rbp-14H], eax
        mov     dword ptr [rbp-20H], edi

So this isn't really a regression and it was a bug in Higgs to rely on memory compare.
I filed https://issues.dlang.org/show_bug.cgi?id=14107 to disallow default comparison for structs with unions.

The amount of code generated for the constructor is still worrisome.
Especially the part that initializes X86Reg, because it initializes every field individually, it constructs a temporary on the stack only to end up loading zero into edx.

        xor     ecx, ecx
        mov     byte ptr [rbp-10H], cl
        mov     byte ptr [rbp-0FH], cl
        mov     word ptr [rbp-0EH], ax
        mov     edx, dword ptr [rbp-10H]

So we should still consider to revert that part of the change.
Comment 5 Walter Bright 2015-02-06 03:29:32 UTC
Changed to reflect your last comment.
Comment 6 Kenji Hara 2015-02-06 13:40:38 UTC
(In reply to Walter Bright from comment #5)
> Changed to reflect your last comment.

No, this is still a regression issue around the struct constructor call.

https://github.com/D-Programming-Language/dmd/pull/4387
Comment 7 Kenji Hara 2015-02-06 13:54:57 UTC
(In reply to Kenji Hara from comment #6)
> (In reply to Walter Bright from comment #5)
> > Changed to reflect your last comment.
> 
> No, this is still a regression issue around the struct constructor call.
> 
> https://github.com/D-Programming-Language/dmd/pull/4387

I opened a new performance issue: https://issues.dlang.org/show_bug.cgi?id=14133
Comment 8 github-bugzilla 2015-02-07 06:24:44 UTC
Commits pushed to master at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/e5f21beb82acade73900aa50a0e092b3935e980b
fix Issue 13952 - change in struct ctor lowering triggers codegen bug

https://github.com/D-Programming-Language/dmd/commit/2ebb60303208768adfa29d29f11f7fc8c95fffa7
Merge pull request #4387 from 9rnsr/fix13952

[REG2.067a] Issue 13952 - change in struct ctor lowering triggers codegen bug