591999 - rememberedStackTop logic inversion

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P4)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

From Bug 568615, comment 14 and Bug 568615, comment 25

Look at changeset:1975 : 

  http://hg.mozilla.org/tamarin-redux/rev/079b86c8954a#l1.131

It inverted the logic of the update to rememberedStackTop.  I believe this was a mistake; stacks grow down, but for the state being maintained here, "Top" really does mean "the tip of the stack", so the original logic was correct.

Tommy and I discussed this when he got back from his sabbatical; he and I agreed that the inversion introduced by changeset:1975 was probably a bug but I forgot to follow up on it afterwards.  Opening this bug now to address it independently of Bug 568615, which I am about to close.

Comment 1

[Lars T Hansen]

I insist that we come up with new terms for the stack limits - unless we go through the code and make sure that every single use of "stack top" and "stack base" refer to that, without any reference to the direction of growth, and do that as part of the work to fix this bug.

Currently we assume that the stack grows down everywhere but I see no point in embedding that assumption more deeply than necessary, and it should be documented whereever we make use of it.

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #1)
> I insist that we come up with new terms for the stack limits - unless we go
> through the code and make sure that every single use of "stack top" and "stack
> base" refer to that, without any reference to the direction of growth, and do
> that as part of the work to fix this bug.

Any suggestions for what the new terms should be?

Up above, I went with "stack tip" for the edge of the most recently pushed frame.  I would go with "stack base" for opposite end, but I'm inferring that that's not good enough (since its  what we're already using).  So, what then, "stack origin"?  Or perhaps just getting rid of uses of the words "top" and "bottom" would suffice?

Comment 3

[Lars T Hansen]

My previous comment presented a choice: either find new terms, or audit the code.  A code audit would be fine with me--that may not have been clear from my phrasing--with "stack base" (oldest frame) and "stack top" (youngest frame) being the terms, clearly defined somewhere very visible.

("Stack oldest end" and "stack youngest end", or similar, would be more precise because they more closely approximate what is the case, but they're probably unfamiliar to many.  On the other hand, given the confusion about "top" it might be worth trying.   cc'ing Edwin.)

Comment 4

[Dan Smith]

Bug scope creep: can we separate this fix from the terminology change or the code audit? The fix itself is easy -- might as well fix it now.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #0)
> From Bug 568615, comment 14 and Bug 568615, comment 25
> 
> Look at changeset:1975 : 
> 
>   http://hg.mozilla.org/tamarin-redux/rev/079b86c8954a#l1.131
> 
> It inverted the logic of the update to rememberedStackTop.  I believe this
> was a mistake; stacks grow down, but for the state being maintained here,
> "Top" really does mean "the tip of the stack", so the original logic was
> correct.

I don't know where the line number in the above link came from.  The correct line number to look at to see the context of the paragraph above is line 204 of the diff:

  http://hg.mozilla.org/tamarin-redux/rev/079b86c8954a#l1.204

1.204 -		if(rememberedStackTop == 0 || rememberedStackTop > item.ptr) {
1.205 +		if(rememberedStackTop < item.ptr) {
1.206  			rememberedStackTop = item.ptr;

Comment 6

[Lars T Hansen]

If there's a bug then yes, find and fix first.

Comment 7

[Tony Printezis]

Attachment: Patch 0: fix for the bug

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 632830 [details] [diff] [review]
Patch 0: fix for the bug

Review of attachment 632830 [details] [diff] [review]:
-----------------------------------------------------------------

R+.  ship it!

Comment 9

[Tony Printezis]

Attachment: Patch 1: fix for the bug (parent CL: 1078824)

The fix is the same as the previous patch, I just added an extra assert (after consulting with Felix) to ensure that the stack grows "downwards". So, if that assumption is ever broken, we'll at least know about it.

Testing: AVM acceptance tests, shelve build

Comment 10

[Tamarin Bot]

changeset: 7448:3c0be1a38dac
user:      Antonios Printezis <[e-mail]>
summary:   Bug 591999: Fixed the logic for rememberedStackTop (r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/3c0be1a38dac