Closed Bug 1101823 Opened 10 years ago Closed 9 years ago

DOM getter can be reordered to before the global shape guard it depends on

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

At least as far as I can tell that's the outcome of the code added in bug 1073766 as far as I can tell: since the DOM instruction doesn't depend on the guard, seems like they can get mis-ordered.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Blocks: 1100757
Comment on attachment 8525738 [details] [diff] [review]
part 1.  Refactor testCommonGetterSetter to just return a boolean indicating whether it's OK to proceed with the common getter/setter, and put the shape guards, into outparams

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

r=me. Comment preexisting, but I think the change is warranted.

::: js/src/jit/IonBuilder.cpp
@@ +9037,5 @@
>  
>      // Add a shape guard on the prototype we found the property on. The rest of
>      // the prototype chain is guarded by TI freezes, except when name is a global
>      // name. In this case, we also have to guard on the globals shape to be able
>      // to optimize. Note that a shape guard is good enough here, even in the proxy

While we're in here, can we fix up this comment to include a notion that the reason we need the shape guard is because of the way global property sets are handled, so freezing won't work?
Attachment #8525738 - Flags: review?(efaustbmo) → review+
Comment on attachment 8525740 [details] [diff] [review]
part 2.  Make sure to add the guard on the global shape to the operands of our DOM getter, if we have such a guard at all

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

Thanks for doing this. r=me

::: js/src/jit/MIR.h
@@ +10416,5 @@
> +        if (globalGuard) {
> +            operandCount = 3;
> +        } else {
> +            operandCount = 2;
> +        }

nit: Unlike in Gecko, single line if-else pairs in SM don't take braces. As an aside, I probably would gotten too cute abd written this |operandCount = 2 + globalGuard|, but this is much easier to read.
Attachment #8525740 - Flags: review?(efaustbmo) → review+
You need to log in before you can comment on or make changes to this bug.