Issue 9449 - Static arrays of 128bit types segfault on initialization. Incorrect calling of memset128ii.
Summary: Static arrays of 128bit types segfault on initialization. Incorrect calling o...
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: x86_64 Linux
: P2 regression
Assignee: Walter Bright
URL:
Keywords: pull, SIMD, wrong-code
: 8518 (view as issue list)
Depends on:
Blocks:
 
Reported: 2013-02-04 03:51 UTC by tbanelwebmin
Modified: 2015-03-10 15:58 UTC (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description tbanelwebmin 2013-02-04 03:51:35 UTC
This small code crashs.

----------------------------------
import core.simd;

void main()
{
  ubyte16 table[1];
}
----------------------------------

It crashes in:
void[] *_memset128ii(void[] *p, void[] value, size_t count);

It seems that a wrong "count" is passed in by the _Dmain() function.

Details:
  DMD64 D Compiler v2.061
  Linux Ubuntu
  x86_64
  AMD Phenom II
Comment 1 hsteoh 2013-02-08 20:36:49 UTC
Something is very screwy with the executable that DMD produces for this code. For example:

$ cat test.d
import core.simd;

void main() {
	ubyte16 table[1];
}
$ dmd -L--export-dynamic -g -m64 test.d
$ gdb test
GNU gdb (GDB) 7.4.1-debian
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /tmp/test...done.
(gdb) break Dmain
Segmentation fault
$


Commenting out the ubyte16 line makes the problem go away (it will correctly set the breakpoint). Looks like the codegen is screwed up somewhere?
Comment 2 hsteoh 2013-02-08 21:39:27 UTC
Actually, this looks like a compiler bug. The ubyte16 alias translates to __vector(ubyte[16]), which is a compiler built-in magic type.

Here's the disassembly of Dmain:

0000000000418620 <_Dmain>:
  418620:       55                      push   %rbp
  418621:       48 8b ec                mov    %rsp,%rbp
  418624:       48 83 ec 10             sub    $0x10,%rsp
  418628:       48 be 01 00 00 00 00    movabs $0x1,%rsi
  41862f:       00 00 00 
  418632:       66 0f 6f 05 e6 77 01    movdqa 0x177e6(%rip),%xmm0        # 42fe20 <_IO_stdin_used+0x10>
  418639:       00 
  41863a:       48 8d 7d f0             lea    -0x10(%rbp),%rdi
  41863e:       e8 a9 07 00 00          callq  418dec <_memset128ii>
  418643:       31 c0                   xor    %eax,%eax
  418645:       c9                      leaveq 
  418646:       c3                      retq   

Here's the disassembly of _memset128ii:

0000000000418dec <_memset128ii>:
  418dec:       55                      push   %rbp
  418ded:       48 8b ec                mov    %rsp,%rbp
  418df0:       48 83 ec 20             sub    $0x20,%rsp
  418df4:       48 89 75 e8             mov    %rsi,-0x18(%rbp)
  418df8:       48 89 55 f0             mov    %rdx,-0x10(%rbp)
  418dfc:       49 89 f8                mov    %rdi,%r8
  418dff:       49 89 fb                mov    %rdi,%r11
  418e02:       49 89 c9                mov    %rcx,%r9
  418e05:       49 c1 e1 04             shl    $0x4,%r9
  418e09:       4c 03 cf                add    %rdi,%r9
  418e0c:       4d 3b c1                cmp    %r9,%r8
  418e0f:       73 18                   jae    418e29 <_memset128ii+0x3d>
  418e11:       48 8b 55 f0             mov    -0x10(%rbp),%rdx
  418e15:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  418e19:       49 89 00                mov    %rax,(%r8)
  418e1c:       49 89 50 08             mov    %rdx,0x8(%r8)
  418e20:       49 83 c0 10             add    $0x10,%r8
  418e24:       4d 39 c8                cmp    %r9,%r8
  418e27:       72 e8                   jb     418e11 <_memset128ii+0x25>
  418e29:       49 8b c3                mov    %r11,%rax
  418e2c:       48 8b e5                mov    %rbp,%rsp
  418e2f:       5d                      pop    %rbp
  418e30:       c3                      retq   

Note that the expected parameters to memset128ii appear to not be passed by Dmain; I traced the execution into memset128ii and found that it was trying to memset an unreasonably large range of memory (2e+15 bytes), probably because the wrong arguments were passed to it.

Since the only druntime code involved is template wrapper around the compiler magic type __vector, the fault must lie with the compiler SIMD intrinsics.
Comment 3 Maxim Fomin 2013-02-09 02:48:23 UTC
_memset128ii expects:

%rcx => size_t count
%rdx => .ptr of value array
$rsi => .length of value array
%rdi => pointer to first argument array 

what _Dmain passes:

%rcx => nothing (garbage)
%rdx => nothing (garbage)
%rsi => size_t count
%rdi => pointer to 16 byte object
%xmm0 => ubyte16[1] array
Comment 4 hsteoh 2013-02-09 22:26:56 UTC
Seems to be related to bug 8518.
Comment 5 Maxim Fomin 2013-02-09 22:40:37 UTC
(In reply to comment #4)
> Seems to be related to bug 8518.

Thanks for founding. The root of the problem (at least this one) is when dmd frontend parses and generates list of expressions, it does not create "hidden" expression which calls _memset128ii. Instead it does this when it executes AssignExpression::toElem() and later calls setArray() which issues call to _memset128ii. However it does not convert ubyte16[1] from static array to dynamic array and passes it as a static array. Since _memset128ii expects dynamic array, the program goes off the rails.
Comment 6 Maxim Fomin 2013-02-15 23:41:19 UTC
It may be a regression. 8 month ago Walter introduced two commits to e2ir.c (https://github.com/D-Programming-Language/dmd/commit/6c2a2878200e0df1c73db976a747abf61b6a5e1a) and src/memset.d (https://github.com/D-Programming-Language/druntime/commit/a405a02394e2c26c6a66c3fc5ef3777bb86cd973) to fix reg pair. However there is no crosstalk between how e2ir.c pass arguments and how _memset128ii takes it. I do not know whether original code was supported before these changes, but if it was, this is a regression.
Comment 7 John Colvin 2013-04-21 09:25:52 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > Seems to be related to bug 8518.
> 
> Thanks for founding. The root of the problem (at least this one) is when dmd
> frontend parses and generates list of expressions, it does not create "hidden"
> expression which calls _memset128ii. Instead it does this when it executes
> AssignExpression::toElem() and later calls setArray() which issues call to
> _memset128ii. However it does not convert ubyte16[1] from static array to
> dynamic array and passes it as a static array. Since _memset128ii expects
> dynamic array, the program goes off the rails.

I'm pretty sure the use of void[] in _memset128ii is simply so as to have a 128bit data type. It's never used as, or expected to be, an array.

_memset128ii doesn't care whether it's being passed a static or dynamic array, it just blindly increments a pointer and writes to it "count" times.

(In reply to comment #3)
> _memset128ii expects:
> 
> %rcx => size_t count
> %rdx => .ptr of value array
> $rsi => .length of value array
> %rdi => pointer to first argument array 

This is incorrect. _memset128 expects:

RCX: size_t count
RDX: higher 64 bits of value
RSI: lower 64 bits of value
RDI: pointer to the 1st element of the destination array.
Comment 8 John Colvin 2013-04-21 09:27:06 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Seems to be related to bug 8518.
> > 
> > Thanks for founding. The root of the problem (at least this one) is when dmd
> > frontend parses and generates list of expressions, it does not create "hidden"
> > expression which calls _memset128ii. Instead it does this when it executes
> > AssignExpression::toElem() and later calls setArray() which issues call to
> > _memset128ii. However it does not convert ubyte16[1] from static array to
> > dynamic array and passes it as a static array. Since _memset128ii expects
> > dynamic array, the program goes off the rails.
> 
> I'm pretty sure the use of void[] in _memset128ii is simply so as to have a
> 128bit data type. It's never used as, or expected to be, an array.
> 
> _memset128ii doesn't care whether it's being passed a static or dynamic array,
> it just blindly increments a pointer and writes to it "count" times.
> 
> (In reply to comment #3)
> > _memset128ii expects:
> > 
> > %rcx => size_t count
> > %rdx => .ptr of value array
> > $rsi => .length of value array
> > %rdi => pointer to first argument array 
> 
> This is incorrect. _memset128 expects:
> 
> RCX: size_t count
> RDX: higher 64 bits of value
> RSI: lower 64 bits of value
> RDI: pointer to the 1st element of the destination array.

/s/_memset128/_memset128ii
Comment 9 Maxim Fomin 2013-04-21 10:52:44 UTC
(In reply to comment #7)
> I'm pretty sure the use of void[] in _memset128ii is simply so as to have a
> 128bit data type. It's never used as, or expected to be, an array.
> 
> _memset128ii doesn't care whether it's being passed a static or dynamic array,
> it just blindly increments a pointer and writes to it "count" times.

I think it does matter whether dynamic array was passed or a static one due to how arguments are passed.

> (In reply to comment #3)
> > _memset128ii expects:
> > 
> > %rcx => size_t count
> > %rdx => .ptr of value array
> > $rsi => .length of value array
> > %rdi => pointer to first argument array 
> 
> This is incorrect. _memset128 expects:
> 
> RCX: size_t count
> RDX: higher 64 bits of value
> RSI: lower 64 bits of value
> RDI: pointer to the 1st element of the destination array.

I see no difference between length dynamic array property and your "lower 64 bits of value" (also between ptr and "higher 64 bits of value"). And passing a pointer to dynamic array is not the same thing as passing pointer to the first element: http://dpaste.dzfl.pl/8f91aed8
Comment 10 John Colvin 2013-04-21 11:56:15 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > I'm pretty sure the use of void[] in _memset128ii is simply so as to have a
> > 128bit data type. It's never used as, or expected to be, an array.
> > 
> > _memset128ii doesn't care whether it's being passed a static or dynamic array,
> > it just blindly increments a pointer and writes to it "count" times.
> 
> I think it does matter whether dynamic array was passed or a static one due to
> how arguments are passed.

It doesn't matter in this case because it is a pointer being passed, not an array at all.

> > (In reply to comment #3)
> > > _memset128ii expects:
> > > 
> > > %rcx => size_t count
> > > %rdx => .ptr of value array
> > > $rsi => .length of value array
> > > %rdi => pointer to first argument array 
> > 
> > This is incorrect. _memset128 expects:
> > 
> > RCX: size_t count
> > RDX: higher 64 bits of value
> > RSI: lower 64 bits of value
> > RDI: pointer to the 1st element of the destination array.
> 
> I see no difference between length dynamic array property and your "lower 64
> bits of value" (also between ptr and "higher 64 bits of value"). 

because value is not an array. As I said before, void[] is just used because conveniently void[].sizeof == 16 (128 bits) on x64 (the check for x64 is done inside e2ir.c)

> And passing a
> pointer to dynamic array is not the same thing as passing pointer to the first
> element: http://dpaste.dzfl.pl/8f91aed8

See my comment above. Each *element* of the array is being represented by a void[]. There is no D style array passing happening here, static or otherwise, it's just pointers.

Imagine replacing void[] with a hypothetical _128BitType and you'll see what I mean.

I'm currently re-implementing memset.d and updating the compiler to interact with the new functions. This should hopefully fix this bug and maybe 9969 also, if there isn't a nasty backend bug hiding behind it all.
Comment 11 John Colvin 2013-04-21 12:11:09 UTC
(In reply to comment #10)
> There is no D style array passing happening here, static or otherwise,
> it's just pointers.

Sorry, mistake.

There is an array being passed as "value", but from the callers point of view (generated in dmd) it's not an array at all, it's just a 128 bit type.
Comment 12 Maxim Fomin 2013-04-22 09:44:52 UTC
(In reply to comment #10)
> > I see no difference between length dynamic array property and your "lower 64
> > bits of value" (also between ptr and "higher 64 bits of value"). 
> 
> because value is not an array. As I said before, void[] is just used because
> conveniently void[].sizeof == 16 (128 bits) on x64 (the check for x64 is done
> inside e2ir.c)
 
Value is actually accepted as an array due to passing conversions and unusual usage inside memset function is no excuse for changing ABI interpretation. Clearly, anyone can pass many different things through some inappropriate parameter but it does not mean that each time callee would adjust passing convention for different types. I don't see point in this dispute further . I argue that value is technically accepted as dynamic array and you argue that it is treated as 128 bit element. These points don't contradict and however arguments are called (lower value or length property) does change the picture - there is no correspondence between what is passed and what is received.

> I'm currently re-implementing memset.d and updating the compiler to interact
> with the new functions. This should hopefully fix this bug and maybe 9969 also,
> if there isn't a nasty backend bug hiding behind it all.

I doubt that it is possible without dmd hacking but good luck.
Comment 13 Ali Cehreli 2013-10-26 11:30:17 UTC
I hit the same bug without any obvious SIMD operations:

struct Point
{
    double f;
    double g;
}

void main()
{
    Point[1] arr;
}

Ali
Comment 14 John Colvin 2014-07-17 12:27:26 UTC
I just ran in to this again with something equivalent to Ali's case above.

My previous attempts to fix this never got anywhere. I could get it working but then other things broke, rinse and repeat... Does anyone have any clue how to do this?

It's a pretty glaring hole to segfault any time someone declares a static array of 128 bit types (excluding cdouble, which has it's own memset overload).

I'm marking this a regression as, whatever the reason, it worked with 2.065.0

Walter I've assigning you because it seems likely that https://github.com/D-Programming-Language/dmd/commit/6c2a2878200e0df1c73db976a747abf61b6a5e1a) and (https://github.com/D-Programming-Language/druntime/commit/a405a02394e2c26c6a66c3fc5ef3777bb86cd973 caused it, but I'm not certain.
Comment 15 John Colvin 2014-07-17 12:29:05 UTC
*** Issue 8518 has been marked as a duplicate of this issue. ***
Comment 16 Walter Bright 2014-07-18 07:14:39 UTC
(In reply to John Colvin from comment #14)
> I'm marking this a regression as, whatever the reason, it worked with 2.065.0
> 
> Walter I've assigning you because it seems likely that
> https://github.com/D-Programming-Language/dmd/commit/
> 6c2a2878200e0df1c73db976a747abf61b6a5e1a) and
> (https://github.com/D-Programming-Language/druntime/commit/
> a405a02394e2c26c6a66c3fc5ef3777bb86cd973 caused it, but I'm not certain.

That seems very unlikely, as those changes were dated June 2012, long before 2.065.
Comment 17 John Colvin 2014-07-18 09:19:32 UTC
(In reply to Walter Bright from comment #16)
> That seems very unlikely, as those changes were dated June 2012, long before
> 2.065.

The issue existed before 2.065, but was somehow masked for a while.
Comment 18 Walter Bright 2014-07-18 09:25:56 UTC
This is not a regression, it never worked. Nevertheless, I'm working on a fix.
Comment 19 Walter Bright 2014-07-18 09:37:05 UTC
partial fix:

https://github.com/D-Programming-Language/druntime/pull/901
Comment 20 Walter Bright 2014-07-18 23:19:46 UTC
Rest of the fix:

https://github.com/D-Programming-Language/dmd/pull/3784
Comment 21 github-bugzilla 2014-07-19 05:10:24 UTC
Commits pushed to master at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/4a8ca65688d2f5be290087c4b1f078c508fc7272
parial Issue 9449 - Static arrays of 128bit types segfault on initialization. Incorrect calling of memset128ii

https://github.com/D-Programming-Language/druntime/commit/3626a88e1dee58a88969f6bbfc5a1b1c74d0dee5
Merge pull request #901 from WalterBright/fix9449

[critical] partial Issue 9449 - Static arrays of 128bit types segfault on initializa...
Comment 22 github-bugzilla 2014-07-21 00:24:49 UTC
Commits pushed to master at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/d9e770828558b9ad5578e6d02bbe5ee54b493326
fix Issue 9449 - Static arrays of 128bit types segfault on initialization. Incorrect calling of memset128ii

https://github.com/D-Programming-Language/dmd/commit/c3ccc4168594b049e3619c2bb5c3016592f8a454
Merge pull request #3784 from WalterBright/fix9449

[critical] fix Issue 9449 - Static arrays of 128bit types segfault on initializatio...
Comment 23 github-bugzilla 2014-07-21 00:34:00 UTC
Commit pushed to dmd-1.x at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/eaef472a9e4ded15360a8a8de9c36b8b3cd07059
fix Issue 9449 - Static arrays of 128bit types segfault on initialization. Incorrect calling of memset128ii
Comment 24 github-bugzilla 2014-07-21 05:47:29 UTC
Commit pushed to 2.066 at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/ff80144f49f7ca8dd376a8ce671c1600d033506a
Merge pull request #901 from WalterBright/fix9449

[critical] partial Issue 9449 - Static arrays of 128bit types segfault on initializa...
Comment 25 github-bugzilla 2014-07-21 06:00:05 UTC
Commit pushed to 2.066 at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/7a12ad9615931b5247c7c1d5fa29d969cc6aa1cc
Merge pull request #3784 from WalterBright/fix9449

[critical] fix Issue 9449 - Static arrays of 128bit types segfault on initializatio...
Comment 26 github-bugzilla 2014-08-22 07:17:26 UTC
Commit pushed to master at https://github.com/D-Programming-Language/druntime

https://github.com/D-Programming-Language/druntime/commit/ff80144f49f7ca8dd376a8ce671c1600d033506a
Merge pull request #901 from WalterBright/fix9449
Comment 27 github-bugzilla 2014-08-22 08:04:43 UTC
Commit pushed to master at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/7a12ad9615931b5247c7c1d5fa29d969cc6aa1cc
Merge pull request #3784 from WalterBright/fix9449
Comment 28 dbr 2014-08-31 07:12:10 UTC
This still segfaults with dmd 2.066 (64-bit linux):

void main() {
    float[4][2] m = [[2.0, 1, 3, 4], [2.0, 1, 3, 4]];
}

Dump of assembler code for function _Dmain:
   0x000000000041a160 <+0>:	push   rbp
   0x000000000041a161 <+1>:	mov    rbp,rsp
   0x000000000041a164 <+4>:	sub    rsp,0x40
   0x000000000041a168 <+8>:	lea    rax,[rbp-0x30]
   0x000000000041a16c <+12>:	movabs rsi,0x2
   0x000000000041a176 <+22>:	movss  xmm0,DWORD PTR [rip+0x183e2]    # 0x432560
   0x000000000041a17e <+30>:	movss  DWORD PTR [rbp-0x10],xmm0
   0x000000000041a183 <+35>:	mov    ecx,0x3f800000
   0x000000000041a188 <+40>:	mov    DWORD PTR [rbp-0x40],ecx
   0x000000000041a18b <+43>:	movss  xmm1,DWORD PTR [rbp-0x40]
   0x000000000041a190 <+48>:	movss  DWORD PTR [rbp-0xc],xmm1
   0x000000000041a195 <+53>:	movss  xmm2,DWORD PTR [rip+0x183c7]    # 0x432564
   0x000000000041a19d <+61>:	movss  DWORD PTR [rbp-0x8],xmm2
   0x000000000041a1a2 <+66>:	movss  xmm3,DWORD PTR [rip+0x183be]    # 0x432568
   0x000000000041a1aa <+74>:	movss  DWORD PTR [rbp-0x4],xmm3
   0x000000000041a1af <+79>:	lea    rdi,[rbp-0x10]
   0x000000000041a1b3 <+83>:	push   QWORD PTR [rdi+0x8]
   0x000000000041a1b6 <+86>:	push   QWORD PTR [rdi]
   0x000000000041a1b8 <+88>:	mov    rdi,rax
   0x000000000041a1bb <+91>:	call   0x41ad5c <_memset128ii>
   0x000000000041a1c0 <+96>:	add    rsp,0x10
   0x000000000041a1c4 <+100>:	xor    eax,eax
   0x000000000041a1c6 <+102>:	leave  
   0x000000000041a1c7 <+103>:	ret
Comment 29 dbr 2014-09-15 05:33:50 UTC
Reopened for this:

void main() {
    float[4][2] m = [[2.0, 1, 3, 4], [2.0, 1, 3, 4]];   // segfault
}
Comment 32 David Nadlinger 2014-10-07 09:50:46 UTC
Can't reproduce on the auto-tester (Git master). Please open a new issue with more details about your system configuration if the problem still persists.
Comment 34 Temtaime 2015-03-07 11:29:47 UTC
Hi !
I've hit this bug with GIT HEAD dmd and vibe.d built with win32driver.

I've disassembled .exe and it crashes with movdqa, the address is misaligned.
I've changed movdqa opcode to movdqu in dmd's code and all works OK now.
Comment 35 Martin Nowak 2015-03-10 15:58:46 UTC
(In reply to Temtaime from comment #34)
> Hi !
> I've hit this bug with GIT HEAD dmd and vibe.d built with win32driver.
> 
> I've disassembled .exe and it crashes with movdqa, the address is misaligned.
> I've changed movdqa opcode to movdqu in dmd's code and all works OK now.

Works with 2.067.0-b4, closing this.
Please open a new ticket if you have this problem on a win32 machine.