Jump to page: 1 2 3
Thread overview
Severe 2.084 regression when using staticArray on array of struct with class and bool member
Feb 12, 2019
Per Nordlöw
Feb 12, 2019
Nicholas Wilson
Feb 12, 2019
ag0aep6g
Feb 12, 2019
jmh530
Feb 12, 2019
John Colvin
Feb 12, 2019
Jacob Carlborg
Feb 12, 2019
Seb
Feb 12, 2019
jmh530
Feb 12, 2019
Basile B.
Feb 12, 2019
Per Nordlöw
Feb 12, 2019
Basile B.
Feb 12, 2019
Basile B.
Feb 12, 2019
Per Nordlöw
Feb 12, 2019
Basile B.
Feb 13, 2019
Per Nordlöw
Feb 13, 2019
Simen Kjærås
Feb 13, 2019
Per Nordlöw
Feb 12, 2019
ag0aep6g
Feb 13, 2019
Walter Bright
Feb 13, 2019
Per Nordlöw
Feb 14, 2019
Temtaime
Feb 14, 2019
Basile B.
Feb 14, 2019
ag0aep6g
Feb 14, 2019
H. S. Teoh
Feb 14, 2019
ag0aep6g
Feb 14, 2019
H. S. Teoh
Feb 15, 2019
Patrick Schluter
Feb 14, 2019
Basile B.
Feb 15, 2019
Seb
February 12, 2019
The following code

class C
{
    this(int i) { this.i = i; }
    int i;
}

struct S
{
    C c;
    bool b;                  // removing this prevents bug
}

void main()
{
    import std.stdio : writeln;
    import std.array : staticArray;

    auto c = new C(42);

    const S[1] s1 = [S(c)].staticArray;
    const S[1] s2 = [S(c)];

    writeln(cast(void*)s1[0].c);
    writeln(cast(void*)s2[0].c);

    assert(s1[0].c is
           s2[0].c);
}

outputs

20
7F6CDAAC0000
core.exception.AssertError@static_array_of_struct_of_class_and_boolean_regression.d(26): Assertion failure

on my Ubuntu 18.04 x86_64 machine.

When I remove the member `b` in `S` the faulty behavior goes away. A regression I presume.

Has anybody already filed this in Bugzilla?

The regression happens on dmd 2.084.1 and master but not in latest ldc beta.
February 12, 2019
On Tuesday, 12 February 2019 at 14:06:44 UTC, Per Nordlöw wrote:
> When I remove the member `b` in `S` the faulty behavior goes away. A regression I presume.
>
> Has anybody already filed this in Bugzilla?
>
> The regression happens on dmd 2.084.1 and master but not in latest ldc beta.

This appears to be a DMD only issue that has never worked (works in both ldc and ldc-beta):

2.079.1 to 2.081.2: Failure with output: onlineapp.d(16): Error: module `std.array` import `staticArray` not found
           2.082.1: Failure with output:
-----
core.exception.AssertError@onlineapp.d(26): Assertion failure
----------------
??:? _d_assertp [0x44b585]
onlineapp.d:26 _Dmain [0x43da4d]
7FFF6ABE6570
7F9104966000
-----

           2.083.1: Failure with output:
-----
core.exception.AssertError@onlineapp.d(26): Assertion failure
----------------
??:? _d_assertp [0x44f21d]
onlineapp.d:26 _Dmain [0x4404cd]
null
7FB5316C5000
-----

Since      2.084.0: Failure with output:
-----
core.exception.AssertError@onlineapp.d(26): Assertion failure
----------------
??:? _d_assertp [0x44f484]
onlineapp.d:26 _Dmain [0x4409fd]
7FFDEE9C7A70
7EFD5A03B000
-----


February 12, 2019
On 12.02.19 15:06, Per Nordlöw wrote:
> class C
> {
>      this(int i) { this.i = i; }
>      int i;
> }
> 
> struct S
> {
>      C c;
>      bool b;                  // removing this prevents bug
> }
> 
> void main()
> {
>      import std.stdio : writeln;
>      import std.array : staticArray;
> 
>      auto c = new C(42);
> 
>      const S[1] s1 = [S(c)].staticArray;
>      const S[1] s2 = [S(c)];
> 
>      writeln(cast(void*)s1[0].c);
>      writeln(cast(void*)s2[0].c);
> 
>      assert(s1[0].c is
>             s2[0].c);
> }

Ouch. That looks bad. A reduction:

----
struct S
{
    ulong c;
    bool b;                  // removing this prevents bug
}

void main()
{
    S[1] a = [S(42)];
    assert(a[0].c == 42); /* Passes. */
    f(a);
}

void f(S[1] a)
{
    assert(a[0].c == 42); /* Fails. */
}
----

Fails since 2.070. https://run.dlang.io/is/LBhn4l
February 12, 2019
On 12.02.19 15:06, Per Nordlöw wrote:
> Has anybody already filed this in Bugzilla?

This one looks like it might be related, but it doesn't seem to be a true duplicate:

https://issues.dlang.org/show_bug.cgi?id=15075
February 12, 2019
On Tuesday, 12 February 2019 at 14:44:10 UTC, ag0aep6g wrote:
> [snip]
>
> Ouch. That looks bad. A reduction:
>
> ----
> struct S
> {
>     ulong c;
>     bool b;                  // removing this prevents bug
> }
>
> void main()
> {
>     S[1] a = [S(42)];
>     assert(a[0].c == 42); /* Passes. */
>     f(a);
> }
>
> void f(S[1] a)
> {
>     assert(a[0].c == 42); /* Fails. */
> }
> ----
>
> Fails since 2.070. https://run.dlang.io/is/LBhn4l

For whatever strange reason, your post gave me a completely unrelated idea:

When we have issues like this that get posted to bugzilla, there is (almost) always a code snippet, but unittests are only created when there an actual fix has been made. Except for what is in bugzilla DMD's source doesn't really know anything about the interaction of bugs. For instance, if fixing one bug would also fix another, we wouldn't know that unless someone marked a bug as duplicate.

To improve the situation, we could add a unittest for every new bug. The immediate problem with this is that since these are bugs, they would all fail. The first way to fix this is that all the bug unittest would be in a version block, so they would only get tested on purpose. The second way is that something like unit-threaded's @ShouldFail could be added to the language and applied to all these bugs. Then, the unittest will only have an error when the bug is fixed and the @ShouldFail can be removed. It will then be easier to identify when a bug fix resolves multiple, potentially related issues.
February 12, 2019
On Tuesday, 12 February 2019 at 18:26:56 UTC, jmh530 wrote:
> On Tuesday, 12 February 2019 at 14:44:10 UTC, ag0aep6g wrote:
>> [snip]
>>
>> Ouch. That looks bad. A reduction:
>>
>> ----
>> struct S
>> {
>>     ulong c;
>>     bool b;                  // removing this prevents bug
>> }
>>
>> void main()
>> {
>>     S[1] a = [S(42)];
>>     assert(a[0].c == 42); /* Passes. */
>>     f(a);
>> }
>>
>> void f(S[1] a)
>> {
>>     assert(a[0].c == 42); /* Fails. */
>> }
>> ----
>>
>> Fails since 2.070. https://run.dlang.io/is/LBhn4l
>
> For whatever strange reason, your post gave me a completely unrelated idea:
>
> When we have issues like this that get posted to bugzilla, there is (almost) always a code snippet, but unittests are only created when there an actual fix has been made. Except for what is in bugzilla DMD's source doesn't really know anything about the interaction of bugs. For instance, if fixing one bug would also fix another, we wouldn't know that unless someone marked a bug as duplicate.
>
> To improve the situation, we could add a unittest for every new bug. The immediate problem with this is that since these are bugs, they would all fail.

There are plenty of tests of things that fail already in the dmd test suite, the only difference being that those are for things that *should* fail.

I put this sort of idea to Walter 1 or 2 DConfs ago, I think the result was "That sounds interesting, someone should do it".
February 12, 2019
On 2019-02-12 19:33, John Colvin wrote:
> On Tuesday, 12 February 2019 at 18:26:56 UTC, jmh530 wrote:
>> For whatever strange reason, your post gave me a completely unrelated idea:
>>
>> When we have issues like this that get posted to bugzilla, there is (almost) always a code snippet, but unittests are only created when there an actual fix has been made. Except for what is in bugzilla DMD's source doesn't really know anything about the interaction of bugs. For instance, if fixing one bug would also fix another, we wouldn't know that unless someone marked a bug as duplicate.
>>
>> To improve the situation, we could add a unittest for every new bug. The immediate problem with this is that since these are bugs, they would all fail.
> 
> There are plenty of tests of things that fail already in the dmd test suite, the only difference being that those are for things that *should* fail.
> 
> I put this sort of idea to Walter 1 or 2 DConfs ago, I think the result was "That sounds interesting, someone should do it".

Should be pretty straightforward to add to the new test runner [1]. It already looks for UDAs on unittest blocks. The tests look like this [2].

[1] https://github.com/dlang/dmd/blob/master/test/tools/unit_test_runner.d

[2] https://github.com/dlang/dmd/blob/master/test/unit/self_test.d

-- 
/Jacob Carlborg
February 12, 2019
On Tuesday, 12 February 2019 at 18:26:56 UTC, jmh530 wrote:
> On Tuesday, 12 February 2019 at 14:44:10 UTC, ag0aep6g wrote:
>> [...]
>
> For whatever strange reason, your post gave me a completely unrelated idea:
>
> When we have issues like this that get posted to bugzilla, there is (almost) always a code snippet, but unittests are only created when there an actual fix has been made. Except for what is in bugzilla DMD's source doesn't really know anything about the interaction of bugs. For instance, if fixing one bug would also fix another, we wouldn't know that unless someone marked a bug as duplicate.
>
> To improve the situation, we could add a unittest for every new bug. The immediate problem with this is that since these are bugs, they would all fail. The first way to fix this is that all the bug unittest would be in a version block, so they would only get tested on purpose. The second way is that something like unit-threaded's @ShouldFail could be added to the language and applied to all these bugs. Then, the unittest will only have an error when the bug is fixed and the @ShouldFail can be removed. It will then be easier to identify when a bug fix resolves multiple, potentially related issues.

This has already existed for several years now:

https://github.com/CyberShadow/DBugTests

However, as currently bugs are reported in a non-structured way I think the curation is only partially automated and thus a lot of work (which needs to be done by someone).
February 12, 2019
On Tuesday, 12 February 2019 at 19:31:56 UTC, Seb wrote:
> [snip]
>
> This has already existed for several years now:
>
> https://github.com/CyberShadow/DBugTests
>
> However, as currently bugs are reported in a non-structured way I think the curation is only partially automated and thus a lot of work (which needs to be done by someone).

Cool. I did not know that. It's always good when you think you have an interesting idea and somebody thought to do it years ago!


February 12, 2019
On Tuesday, 12 February 2019 at 14:44:10 UTC, ag0aep6g wrote:
> On 12.02.19 15:06, Per Nordlöw wrote:
>> class C
>> {
>>      this(int i) { this.i = i; }
>>      int i;
>> }
>> 
>> struct S
>> {
>>      C c;
>>      bool b;                  // removing this prevents bug
>> }
>> 
>> void main()
>> {
>>      import std.stdio : writeln;
>>      import std.array : staticArray;
>> 
>>      auto c = new C(42);
>> 
>>      const S[1] s1 = [S(c)].staticArray;
>>      const S[1] s2 = [S(c)];
>> 
>>      writeln(cast(void*)s1[0].c);
>>      writeln(cast(void*)s2[0].c);
>> 
>>      assert(s1[0].c is
>>             s2[0].c);
>> }
>
> Ouch. That looks bad. A reduction:
>
> ----
> struct S
> {
>     ulong c;
>     bool b;                  // removing this prevents bug
> }
>
> void main()
> {
>     S[1] a = [S(42)];
>     assert(a[0].c == 42); /* Passes. */
>     f(a);
> }
>
> void f(S[1] a)
> {
>     assert(a[0].c == 42); /* Fails. */
> }
> ----
>
> Fails since 2.070. https://run.dlang.io/is/LBhn4l

It's a bad codegen, or what i would call trivially "an ABI disagreement", explaining why LDC doesn't exhibits the bug.

As far as I can see the problem is that the static array is passed by the registers, so under linux, `c` in RDI, and `b` in RSI but then in `f()` the backend thinks that it's passed using the stack.

The bug goes away when S size is over or equal to 33 bytes because in this case the parameter is well copied.

material leading to this conclusion:

```
struct S
{
    ulong c;
    bool b;                  // removing this prevents bug
}

import disassembler, std.stdio; // godbolt is too noisy with dmd so i use mine

void main()
{
    S[1] a = [S(42, true)];
    showThatRegsAreUsed(a);
    showDmdBadCodegen(a);
    writeln(prettyDisasm(&showThatRegsAreUsed));
    writeln(prettyDisasm(&showDmdBadCodegen));
}

void showDmdBadCodegen(S[1] a)
{
    writeln(a[0].c);
}

void showThatRegsAreUsed(S[1] a)
{
    alias writeul = writeln!ulong;
    asm
    {
        naked;
        push RSI;
        call writeul; // 1st param, so 42
        pop RSI;
        mov RDI, RSI;
        call writeul; // 2nd so RSI
        ret;
    }
}
```

output:

compiling /tmp/temp_7F711D03FA50.d
using /usr/bin/dmd (dmd)
/tmp/temp_7F711D03FA50.d successfully compiled
Runnable build duration: 0 minutes, 0 seconds and 399 msecs
42
1
0
;------- SUB 00000000004511ACh -------; showThatRegsAreUsed
00000000004511ACh  push rsi
00000000004511ADh  call 00000000004516FCh
00000000004511B2h  pop rsi
00000000004511B3h  mov rdi, rsi
00000000004511B6h  call 00000000004516FCh
00000000004511BBh  ret
;-------------------------------------


;------- SUB 000000000045119Ch -------; showDmdBadCodegen
000000000045119Ch  push rbp
000000000045119Dh  mov rbp, rsp
00000000004511A0h  mov rdi, qword ptr [rbp+10h] ; ouch 42 is erased here !
00000000004511A4h  call 00000000004516FCh
00000000004511A9h  pop rbp
00000000004511AAh  ret
;-------------------------------------

« First   ‹ Prev
1 2 3