948162 - Re-enable js/src/jit-test/tests/baseline/bug857580.js on GGC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Something broke this test silently in the last few weeks and it is was failing intermittently. We decided to disable the test until someone has the time to investigate.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug948162-dependent-addptr

The AddPtr contained in DependentAddPtr is not updated if we find that a GC has happened.  That's what caused this test to fail, because the assertion uses pointer later on.

Further to that, I found that in most of the uses of DependentAddPtr, a GC can mutate the hash table even if we're not using generational GC.  The pattern is often used for adding weakly-held wrappers to hash tables, and the entries can be removed if the wrapped thing dies.  So I think we should make the part of DependentAddPtr that checks for GC and recomputes the hash non-GGC-specific.  It only happens in the case of GC, so should be rare in practice.

Finally, I changed DependentAddPtr to not store the context but take it as a parameter, as the context is always going to be available anyway and this seems more in keeping with the way everything else works.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug948162-dependent-addptr

On second thoughts, the second paragraph above is wrong - it doesn't matter that the hash table can be mutated, this is why we use relookupOrAdd()!

I was seeing this test fail on exact rooting builds with the fix for the first issue above applied.  But that turns out to be because DebuggerWeakMap::relookupOrAdd() asserts !p.found(), where p may be out of date due to a hash table mutation.  So if we fix that assert then everything works.

I updated the test code too use gczeal(2, 10) as this triggered the problem every time.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8346502 [details] [diff] [review]
bug948162-dependent-addptr

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

Great! I think this will fix at least one other fuzz bug I was looking at as well. r=me

::: js/src/jshashutil.h
@@ +45,5 @@
>  
>      bool found() const                 { return addPtr.found(); }
>      operator ConvertibleToBool() const { return found() ? &DependentAddPtr::nonNull : 0; }
>      const Entry &operator*() const     { return *addPtr; }
>      const Entry *operator->() const    { return &*addPtr; }

Could you add an assertion to *, -> and ConvertibleToBool that !gcHappened? File a follow-up if it breaks all-the-things, but I expect it should "just work."

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #3)

> Could you add an assertion to *, -> and ConvertibleToBool that !gcHappened?
> File a follow-up if it breaks all-the-things, but I expect it should "just
> work."

I'm not sure about this.  The hashtable generation will already detect if GC modified the hash table.  Detecting use of the pointer after GC moved the key it was based on but before we did the insertion is a win.  But I think it will break use of the pointer after successful insertion if a GC happened, for example at the end of Debugger::wrapScript().  Of course we could update the GC  number on add, but then this might fire if there were subsequent GC

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug948162-dependent-addptr-v2

Here's an updated patch anyway.  Slightly more complicated but I guess it's better to have those assertions.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #4)
> (In reply to Terrence Cole [:terrence] from comment #3)
> 
> > Could you add an assertion to *, -> and ConvertibleToBool that !gcHappened?
> > File a follow-up if it breaks all-the-things, but I expect it should "just
> > work."
> 
> I'm not sure about this.  The hashtable generation will already detect if GC
> modified the hash table.

Good point! Up until yesterday, AddPtr only checked mutationCount and not generation. Before, this was okay because generation-changed implied mutationCount-changed; with rekey, however, this is no longer true. We do have those assertions now, so this is less necessary.

>  Detecting use of the pointer after GC moved the
> key it was based on but before we did the insertion is a win.

Generation only changes when the table gets reallocated, so just asserting the generation may not even work.

> But I think
> it will break use of the pointer after successful insertion if a GC
> happened, for example at the end of Debugger::wrapScript().  Of course we
> could update the GC  number on add, but then this might fire if there were
> subsequent GC

Right! I didn't think of that. Maybe we want to relookup instead of asserting?

Comment 7

[Terrence Cole [:terrence]]

Okay, with a bit more thought: I think we should re-lookup in the accessors if a gc happened. I guess we need to hold |cx| for that, but I don't think it's a big deal.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #7)
We also need to hold the key if we're going to relookup on GC, which seems a bit overcomplicated.

Comment 9

[Terrence Cole [:terrence]]

Okay, I haven't managed to come up with a way to assert exactly what we want in the mean time. Lets go ahead with the r+ on v1 of the patch and land as-is.

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6a38ef6d142

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6a38ef6d142