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 just an optimisation, it is required because otherwise the memset() call will attempt to mark a single card. (this was the bug I "fixed" last time I touched this code, but I think I might have inadverdently introduced the bug you just fixed)
Yes, that's exactly right. I'll add a comment.
Please go ahead and commit. Note that CgPrimOp is scheduled for demolition very shortly, but the bug will need to be fixed there in the 7.6 branch.
Will do. Thanks for taking a look! Roman