"done" checkpoint of iterator_next stores the wrong register in the value profile in baseline JIT

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 415330 [details]
patch

[Keith Miller]

Comment on attachment 415330 [details]
patch

r=me

[Mark Lam]

Comment on attachment 415330 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415330&action=review

Looks like this code was always broken for 32-bit.  Can you pass a JSValueRegs to emitValueProfilingSite(), and at least add a FIXME for the 32-bit folks to fix JIT::emit_op_iterator_next()?

> Source/JavaScriptCore/jit/JITInlines.h:312
> +inline void JIT::emitValueProfilingSite(ValueProfile& valueProfile, GPRReg value)

Use JSValueRegs instead of GPRReg.

> Source/JavaScriptCore/jit/JITInlines.h:317
>      const RegisterID valueTag = regT1;

This is going to be a bug because doneGPR above is regT1.  I think the right thing to do is to pass value using the JSValueRegs abstraction instead.

[Saam Barati]

(In reply to Mark Lam from comment #3)
> Comment on attachment 415330 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415330&action=review
> 
> Looks like this code was always broken for 32-bit.  Can you pass a
> JSValueRegs to emitValueProfilingSite(), and at least add a FIXME for the
> 32-bit folks to fix JIT::emit_op_iterator_next()?
> 
> > Source/JavaScriptCore/jit/JITInlines.h:312
> > +inline void JIT::emitValueProfilingSite(ValueProfile& valueProfile, GPRReg value)
> 
> Use JSValueRegs instead of GPRReg.
> 
> > Source/JavaScriptCore/jit/JITInlines.h:317
> >      const RegisterID valueTag = regT1;
> 
> This is going to be a bug because doneGPR above is regT1.  I think the right
> thing to do is to pass value using the JSValueRegs abstraction instead.

I don't care about 32-bits

[Saam Barati]

(In reply to Saam Barati from comment #4)
> (In reply to Mark Lam from comment #3)
> > Comment on attachment 415330 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=415330&action=review
> > 
> > Looks like this code was always broken for 32-bit.  Can you pass a
> > JSValueRegs to emitValueProfilingSite(), and at least add a FIXME for the
> > 32-bit folks to fix JIT::emit_op_iterator_next()?
> > 
> > > Source/JavaScriptCore/jit/JITInlines.h:312
> > > +inline void JIT::emitValueProfilingSite(ValueProfile& valueProfile, GPRReg value)
> > 
> > Use JSValueRegs instead of GPRReg.
> > 
> > > Source/JavaScriptCore/jit/JITInlines.h:317
> > >      const RegisterID valueTag = regT1;
> > 
> > This is going to be a bug because doneGPR above is regT1.  I think the right
> > thing to do is to pass value using the JSValueRegs abstraction instead.
> 
> I don't care about 32-bits

I suppose we can use the abstraction anyways

[Saam Barati]

Created attachment 415348 [details]
patch for landing

[Saam Barati]

Comment on attachment 415330 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415330&action=review

>> Source/JavaScriptCore/jit/JITInlines.h:317
>>      const RegisterID valueTag = regT1;
> 
> This is going to be a bug because doneGPR above is regT1.  I think the right thing to do is to pass value using the JSValueRegs abstraction instead.

to clarify, this wouldn't have been a bug because the other code you're reading is 64-bit only. However, JSValueRegs is a nicer abstraction, so I went with it as you suggest

[EWS]

Committed r270423: <https://trac.webkit.org/changeset/270423>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415348 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71964541>

[Caio Lima]

Comment on attachment 415348 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=415348&action=review

> Source/JavaScriptCore/jit/JIT.h:377
> +        void emitValueProfilingSite(ValueProfile&, JSValueRegs value = JSValueRegs { regT0 });

I think it would be better to require an explicit JSValueRegs to avoid future mistakes. I also found the same issue on `JIT::emit_op_get_prototype_of`. If you agree, I can include such change on https://bugs.webkit.org/show_bug.cgi?id=219535.

[Saam Barati]

(In reply to Caio Lima from comment #10)
> Comment on attachment 415348 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415348&action=review
> 
> > Source/JavaScriptCore/jit/JIT.h:377
> > +        void emitValueProfilingSite(ValueProfile&, JSValueRegs value = JSValueRegs { regT0 });
> 
> I think it would be better to require an explicit JSValueRegs to avoid
> future mistakes. I also found the same issue on
> `JIT::emit_op_get_prototype_of`. If you agree, I can include such change on
> https://bugs.webkit.org/show_bug.cgi?id=219535.

I agree.

[Caio Lima]

(In reply to Saam Barati from comment #11)
> (In reply to Caio Lima from comment #10)
> > Comment on attachment 415348 [details]
> > patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=415348&action=review
> > 
> > > Source/JavaScriptCore/jit/JIT.h:377
> > > +        void emitValueProfilingSite(ValueProfile&, JSValueRegs value = JSValueRegs { regT0 });
> > 
> > I think it would be better to require an explicit JSValueRegs to avoid
> > future mistakes. I also found the same issue on
> > `JIT::emit_op_get_prototype_of`. If you agree, I can include such change on
> > https://bugs.webkit.org/show_bug.cgi?id=219535.
> 
> I agree.

Since https://bugs.webkit.org/show_bug.cgi?id=219535 already landed, I created https://bugs.webkit.org/show_bug.cgi?id=219550 to handle it.