Roman Leshchinskiy | 6 Oct 23:41 2012
Picon
Picon

copyArray# bug

I've been chasing a segfault in the dev version of vector and I think I 
finally traced it to a bug in the implementation of copyArray# and 
copyMutableArray#. More specifically, I think emitSetCards in 
StgCmmPrim.hs (and CgPrimOp.hs) will sometimes fail to mark the last 
card as dirty because in the current implementation, the number of cards 
to mark is computed solely from the number of copied elements while it 
really depends on which cards the first and the last elements belong to. 
That is, the number of elements to copy might be less than the number of 
elements per card but the copied range might still span two cards.

The attached patch fixes this (and the segfault in vector) and also 
makes copyArray# return immediately if the number of elements to copy is 
0. Could someone who is familiar with the code please review it and tell 
me if it looks sensible. If it does, I'll make the same modification to 
CgPrimOp.hs (which has exactly the same code) and commit. Unfortunately, 
I have no idea how to write a testcase for this since the bug is only 
triggered in very specific circumstances.

It seems that all released versions of GHC that implement 
copyArray#/copyMutableArray# have this problem. At least, vector's 
testsuite now segfaults with all of them in roughly the same place after 
recent modifications I've made (which involve calling copyArray# a lot). 
If I'm right then I would suggest not to use copyArray# and 
copyMutableArray# for GHC < 7.8.

Roman

diff --git a/compiler/codeGen/StgCmmPrim.hs b/compiler/codeGen/StgCmmPrim.hs
(Continue reading)

Simon Marlow | 8 Oct 13:38 2012
Picon

Re: copyArray# bug

On 06/10/2012 22:41, Roman Leshchinskiy wrote:
> I've been chasing a segfault in the dev version of vector and I think I
> finally traced it to a bug in the implementation of copyArray# and
> copyMutableArray#. More specifically, I think emitSetCards in
> StgCmmPrim.hs (and CgPrimOp.hs) will sometimes fail to mark the last
> card as dirty because in the current implementation, the number of cards
> to mark is computed solely from the number of copied elements while it
> really depends on which cards the first and the last elements belong to.
> That is, the number of elements to copy might be less than the number of
> elements per card but the copied range might still span two cards.
>
> The attached patch fixes this (and the segfault in vector) and also
> makes copyArray# return immediately if the number of elements to copy is
> 0. Could someone who is familiar with the code please review it and tell
> me if it looks sensible. If it does, I'll make the same modification to
> CgPrimOp.hs (which has exactly the same code) and commit. Unfortunately,
> I have no idea how to write a testcase for this since the bug is only
> triggered in very specific circumstances.
>
> It seems that all released versions of GHC that implement
> copyArray#/copyMutableArray# have this problem. At least, vector's
> testsuite now segfaults with all of them in roughly the same place after
> recent modifications I've made (which involve calling copyArray# a lot).
> If I'm right then I would suggest not to use copyArray# and
> copyMutableArray# for GHC < 7.8.

Nice catch!

Just to make sure I'm understanding: the conditional you added is not 
just an optimisation, it is required because otherwise the memset() call 
(Continue reading)

Roman Leshchinskiy | 8 Oct 17:04 2012
Picon
Picon

Re: copyArray# bug

Simon Marlow wrote:
> On 06/10/2012 22:41, Roman Leshchinskiy wrote:
>> I've been chasing a segfault in the dev version of vector and I think I
>> finally traced it to a bug in the implementation of copyArray# and
>> copyMutableArray#. More specifically, I think emitSetCards in
>> StgCmmPrim.hs (and CgPrimOp.hs) will sometimes fail to mark the last
>> card as dirty because in the current implementation, the number of cards
>> to mark is computed solely from the number of copied elements while it
>> really depends on which cards the first and the last elements belong to.
>> That is, the number of elements to copy might be less than the number of
>> elements per card but the copied range might still span two cards.
>>
>> The attached patch fixes this (and the segfault in vector) and also
>> makes copyArray# return immediately if the number of elements to copy is
>> 0. Could someone who is familiar with the code please review it and tell
>> me if it looks sensible. If it does, I'll make the same modification to
>> CgPrimOp.hs (which has exactly the same code) and commit. Unfortunately,
>> I have no idea how to write a testcase for this since the bug is only
>> triggered in very specific circumstances.
>>
>> It seems that all released versions of GHC that implement
>> copyArray#/copyMutableArray# have this problem. At least, vector's
>> testsuite now segfaults with all of them in roughly the same place after
>> recent modifications I've made (which involve calling copyArray# a lot).
>> If I'm right then I would suggest not to use copyArray# and
>> copyMutableArray# for GHC < 7.8.
>
> Nice catch!
>
> Just to make sure I'm understanding: the conditional you added is not
(Continue reading)

Johan Tibell | 8 Oct 18:32 2012
Picon

Re: copyArray# bug

Hi,

I did quite a bit of work to make sure copyArray# and friends get
unrolled if the number of elements to copy is a constant. Does this
still work with the extra branch?

-- Johan
Roman Leshchinskiy | 9 Oct 10:26 2012
Picon
Picon

Re: copyArray# bug

Johan Tibell wrote:
> Hi,
>
> I did quite a bit of work to make sure copyArray# and friends get
> unrolled if the number of elements to copy is a constant. Does this
> still work with the extra branch?

I would expect it to but I don't know. Does the testsuite check for this?

Roman
Johan Tibell | 9 Oct 16:58 2012
Picon

Re: copyArray# bug

On Tue, Oct 9, 2012 at 1:26 AM, Roman Leshchinskiy <rl <at> cse.unsw.edu.au> wrote:
> Johan Tibell wrote:
>> Hi,
>>
>> I did quite a bit of work to make sure copyArray# and friends get
>> unrolled if the number of elements to copy is a constant. Does this
>> still work with the extra branch?
>
> I would expect it to but I don't know. Does the testsuite check for this?

Simon, the assembly testing support I added would be very useful now.
It would tell us if this change preserved unrolling or not.

-- Johan
Herbert Valerio Riedel | 9 Oct 11:39 2012
Picon

Re: copyArray# bug

Roman Leshchinskiy <rl <at> cse.unsw.edu.au> writes:

[...]

> If I'm right then I would suggest not to use copyArray# and
> copyMutableArray# for GHC < 7.8.

I've grepped today's

 http://hackage.haskell.org/cgi-bin/hackage-scripts/archive.tar

for occurences of those two primitives, and this resulted in the
following matches:

--8<---------------cut here---------------start------------->8---
./unordered-containers-0.2.2.1/Data/HashMap/Array.hs:        case copyArray# (unArray src) sidx#
(unMArray dst) didx# n# s# of
./unordered-containers-0.2.2.1/Data/HashMap/Array.hs:    case copyMutableArray# (unMArray src)
sidx# (unMArray dst) didx# n# s# of
./persistent-vector-0.1.0.1/src/Data/Vector/Persistent/Array.hs:        case copyArray# (unArray src)
sidx# (unMArray dst) didx# n# s# of
./persistent-vector-0.1.0.1/src/Data/Vector/Persistent/Array.hs:    case copyMutableArray#
(unMArray src) sidx# (unMArray dst) didx# n# s# of
./primitive-0.5/Data/Primitive/Array.hs:  = primitive_ (copyArray# src# soff# dst# doff# len#)
./primitive-0.5/Data/Primitive/Array.hs:  = primitive_ (copyMutableArray# src# soff# dst# doff# len#)
./trifecta-0.53/src/Text/Trifecta/Util/Array.hs:  ST $ \ s# -> case copyArray# (unArray src) sidx#
(unMArray dst) didx# n# s# of
./trifecta-0.53/src/Text/Trifecta/Util/Array.hs:  ST $ \ s# -> case copyMutableArray# (unMArray src)
sidx# (unMArray dst) didx# n# s# of
--8<---------------cut here---------------end--------------->8---
(Continue reading)

Roman Leshchinskiy | 9 Oct 13:31 2012
Picon
Picon

Re: copyArray# bug

Herbert Valerio Riedel wrote:
> Roman Leshchinskiy <rl <at> cse.unsw.edu.au> writes:
>
>
> [...]
>
>> If I'm right then I would suggest not to use copyArray# and
>> copyMutableArray# for GHC < 7.8.
>
> I've grepped today's
>
>  http://hackage.haskell.org/cgi-bin/hackage-scripts/archive.tar
>
> for occurences of those two primitives, and this resulted in the
> following matches:
>
> [...]
>
> ...so, are you saying, that those packages above are dangerous to use
> with GHC<=7.6.1?

I don't know about the other packages (it depends entirely on what kind of
arrays they copy and how) but these particular functions in primitive
definitely are dangerous to use. I'll release a hotfix shortly.

Roman

Gmane