WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
219501
"done" checkpoint of iterator_next stores the wrong register in the value profile in baseline JIT
https://bugs.webkit.org/show_bug.cgi?id=219501
Summary
"done" checkpoint of iterator_next stores the wrong register in the value pro...
Saam Barati
Reported
2020-12-03 12:03:49 PST
...
Attachments
patch
(3.89 KB, patch)
2020-12-03 12:57 PST
,
Saam Barati
keith_miller
: review+
Details
Formatted Diff
Diff
patch for landing
(4.78 KB, patch)
2020-12-03 14:33 PST
,
Saam Barati
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Saam Barati
Comment 1
2020-12-03 12:57:14 PST
Created
attachment 415330
[details]
patch
Keith Miller
Comment 2
2020-12-03 13:09:33 PST
Comment on
attachment 415330
[details]
patch r=me
Mark Lam
Comment 3
2020-12-03 13:19:41 PST
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
Comment 4
2020-12-03 14:15:54 PST
(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
Comment 5
2020-12-03 14:17:30 PST
(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
Comment 6
2020-12-03 14:33:25 PST
Created
attachment 415348
[details]
patch for landing
Saam Barati
Comment 7
2020-12-03 14:41:01 PST
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
Comment 8
2020-12-03 20:09:12 PST
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
Comment 9
2020-12-03 20:10:22 PST
<
rdar://problem/71964541
>
Caio Lima
Comment 10
2020-12-04 04:40:08 PST
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
Comment 11
2020-12-04 10:53:41 PST
(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
Comment 12
2020-12-04 12:45:11 PST
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug