Thread overview | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
February 12, 2019 Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to Per Nordlöw | 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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to Per Nordlöw | 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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to Per Nordlöw | 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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to ag0aep6g | 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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to jmh530 | 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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to John Colvin | 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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to jmh530 | 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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to Seb | 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 Re: Severe 2.084 regression when using staticArray on array of struct with class and bool member | ||||
---|---|---|---|---|
| ||||
Posted in reply to ag0aep6g | 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 ;------------------------------------- |
Copyright © 1999-2021 by the D Language Foundation