Bug 164576 – We probably have some races between how we validate that structures are registered and how we tell AI what structures we produce

• Status: NEW
• Component: JavaScriptCore

[Saam Barati]

For example, consider two threads, the JS thread (JST), and the compiler thread (CT)
Consider this interleaving of execution:

CT: Runs structure registration phase and registers arrayStructureForIndexingTypeDuringAllocation(ArrayWithInt32)
JST: fires having a bad time
CT: We tell AI that this node results in a value with arrayStructureForIndexingTypeDuringAllocation(ArrayWithInt32) structure.

We will no longer properly verify this code. I think this is mostly an innocuous bug, since such array allocation nodes will usually watch the having a bad time node, so if it fires during the compilation, we will eventually invalidate the compilation. However, it's probably worth having a more sound story here. What I'm doing in the patch I'm writing now is always registering and telling AI that I'm producing the original allocation structure if the compilation is watching the having a bad time watchpoint. Otherwise, I register the current allocation structure. The assumption here is that the compilation will always watch the having a bad time watchpoint for this particular node if it hasn't fired. If it's already fired, then using the current allocation structure is OK. However, this probably still leaves us a window to be racy about what structure we say we're producing.

[Filip Pizlo]

(In reply to comment #0)
> For example, consider two threads, the JS thread (JST), and the compiler
> thread (CT)
> Consider this interleaving of execution:
> 
> CT: Runs structure registration phase and registers
> arrayStructureForIndexingTypeDuringAllocation(ArrayWithInt32)
> JST: fires having a bad time
> CT: We tell AI that this node results in a value with
> arrayStructureForIndexingTypeDuringAllocation(ArrayWithInt32) structure.
> 
> We will no longer properly verify this code. I think this is mostly an
> innocuous bug, since such array allocation nodes will usually watch the
> having a bad time node, 

When would they not?

Why not add the watch point for good measure in the StructureReistrtionPhase?  That would address the bug and would be consistent with the sure that any compiler phase makes an assumption based on a watch point should watch that watch point. 

Part of the bug is the arrayStructureForBlahBlah API, which is not explicit about the fact that it's results are only valid at that moment in time. Here this code racily assumes that the value it got is still true even the moment after it got it. 

So you're totally right there's a race. I think the bug is that we aren't using the one true idiom:

if (is watchable && the thing I want is true) {
    Do the thing I want
    Watch the watch point
}

Which in this code translates to:

If (bad time watchpoint is watchable) {
    Register the arrayStructureForBlah
    Watch having a bad time
} else {
    Register both arrayStructureForBlah and its bad time cousin
}

But looking at the above it's clear we should just register both all the time!  Then there isn't even a watchpoint!

> so if it fires during the compilation, we will
> eventually invalidate the compilation. However, it's probably worth having a
> more sound story here. What I'm doing in the patch I'm writing now is always
> registering and telling AI that I'm producing the original allocation
> structure if the compilation is watching the having a bad time watchpoint.

This last part sounds dirty. It's not necessary to predicate this. We want downstream optimizations to be able to just know that they can call arrayStructureForBlah and use that. 

> Otherwise, I register the current allocation structure. The assumption here
> is that the compilation will always watch the having a bad time watchpoint
> for this particular node if it hasn't fired. If it's already fired, then
> using the current allocation structure is OK. However, this probably still
> leaves us a window to be racy about what structure we say we're producing.

Which is why I would have registered both on the slow path. And as soon as I realized that, I realized that doing so is so cheap that we might as well do it unconditionally.