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

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: jonco)

Tracking

Trunk
mozilla29
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8346474 [details] [diff] [review]
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.
Assignee: general → jcoppeard
(Assignee)

Comment 2

4 years ago
Created attachment 8346502 [details] [diff] [review]
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.
Attachment #8346474 - Attachment is obsolete: true
Attachment #8346502 - Flags: review?(terrence)
(Reporter)

Comment 3

4 years ago
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."
Attachment #8346502 - Flags: review?(terrence) → review+
(Assignee)

Comment 4

4 years ago
(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
(Assignee)

Comment 5

4 years ago
Created attachment 8346688 [details] [diff] [review]
bug948162-dependent-addptr-v2

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

Comment 6

4 years ago
(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?
(Reporter)

Comment 7

4 years ago
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.
(Assignee)

Comment 8

4 years ago
(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.
(Reporter)

Comment 9

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/a6a38ef6d142
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.