588364 - Incorrect code and comments around Array/Vector shift/unshift

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Lars T Hansen]

Attachment: Patch

I discovered some comments in the shift/unshift code that explained that some array-clearing code was there "for RC purposes".  That was arguably not the best explanation - any kind of GC would benefit from stale pointers being cleared, though RC could go seriously wrong, I suppose.  (Anyway, upon removing RC this code should not be removed.)  Following up it turns out that there's a right way (nullAtomRange) and a wrong way (memset) to clear the contents, and the wrong way was used in a couple of places.  So this patch (a) changes the comments and (b) factors the "right way" by placing a utility method in AvmCore and (c) uses the utility method uniformly.

Comment 1

[Lars T Hansen]

It's probably the case that the Array and Vector code should be vetted for this problem generally.

Comment 2

[Lars T Hansen]

(In reply to comment #1)
> It's probably the case that the Array and Vector code should be vetted for
> this problem generally.

There is only one case left, namely AtomArray::checkCapacity.  This one seems a little strange: it allocates a new GCObject, calls setAtoms() to do the necessary write barrier song and dance, and then copies atoms from an old array into a new one:

     // use a memcpy to skip ref counting
     VMPI_memcpy(m_atoms, oldAtoms, m_length*sizeof(Atom));
     VMPI_memset(oldAtoms, 0, m_length*sizeof(Atom));
     gc->Free(oldAtoms);

This code appears to be correct, but the VMPI_memset is redundant unless the object can't be freed.  The only reason it wouldn't be freed is if we're in a callback handler from within the GC, or if the object is on the mark stack.

I will add a comments to that effect in the code when I land the patch, although I guess the correct thing to do would be to not use memset but to use nullAtomRange here too - to benefit exact tracing.

Opinions?

Comment 3

[Steven Johnson]

Yow, yeah, wacky. I had forgotten about the "free might not free immediately in obscure situations". (Perhaps, in such situations, Free() should at least zero/null out its contents, to avoid having to do it pre-emptively at the call site?)

Comment 4

[Lars T Hansen]

(In reply to comment #3)
> Perhaps, in such situations, Free() should at least
> zero/null out its contents, to avoid having to do it pre-emptively at the call
> site?

Written up as bug #589102, I will insert a comment in checkCapacity to that effect.

Comment 5

[Tommy Reilly]

Comment on attachment 466988 [details] [diff] [review]
Patch

- A little inconsistent that we start out zero'd but then later zero'd memory is actually nullAtom'd, not a show-stopper though.   
- Also nullAtomRange needs a comment that it doesn't do anything smart wrt DRC and its only for nullAtom'ing undefined memory.

Comment 6

[Lars T Hansen]

On IM Tommy also pointed out that nullAtomRange is being used to clear vector storage before it's freed, like the Array storage in comment #2.

Altogether I think I'll be looking for a consistent story here before I try to land anything.  It may be that nullAtomRange should just zero out its memory (with memset) if the Box code is not active, but I don't know if that's appropriate for typed vectors (I'm guessing not).

Comment 7

[Andre Kruetzfeldt]

Can we land this?

Comment 8

[Lars T Hansen]

No, we cannot land this, see my last comment - we need a consistent story.

Comment 9

[Tommy Reilly]

I'm not working on this, resetting assignee to default

Comment 10

[Steven Johnson]

poaching, as work on Bug 570608 should neatly resolve this

Comment 11

[Steven Johnson]

Bug 605324 would likely make this bug moot

Comment 12

[Steven Johnson]

fixed by way of bug 605324 in 5420:821faaf8d7ff