Bug 237207

Created attachment 453209 [details]
Patch

Comment on attachment 453209 [details]
Patch

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

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:358
> -            DisallowMacroScratchRegisterUsage disallowScratch(jit);
>              jit.loadPtr(CCallHelpers::Address(callLinkInfoGPR, offsetOfCallee()), scratchGPR);
> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);

Is this necessary? In theory it shouldn't cause problems, but it feels safer to me to disallow usage for the whole block, load including.

(In reply to Zan Dobersek from comment #2)
> Comment on attachment 453209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453209&action=review
> 
> > Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:358
> > -            DisallowMacroScratchRegisterUsage disallowScratch(jit);
> >              jit.loadPtr(CCallHelpers::Address(callLinkInfoGPR, offsetOfCallee()), scratchGPR);
> > +            DisallowMacroScratchRegisterUsage disallowScratch(jit);
> 
> Is this necessary? In theory it shouldn't cause problems, but it feels safer
> to me to disallow usage for the whole block, load including.

It's necessary for the code to work in its current form, but not strictly necessary. The difference here is that loadPtr (indirectly) does RELEASE_ASSERT(m_allowScratchRegister) because it explicitly invalidates the associated CachedRegister by calling cachedAddressTempRegister().invalidate().

One could have all callers of load*() invalidate the destination register instead, but that felt a bit more error prone. Especially since my base tree didn't include any caller that would need to skip the invalidation (I discovered this new usage when rebasing on Friday). I'm open to changing things around if anyone thinks that would be more appropriate (either in theory or in practice).

Comment on attachment 453209 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:358
>>> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);
>> 
>> Is this necessary? In theory it shouldn't cause problems, but it feels safer to me to disallow usage for the whole block, load including.
> 
> It's necessary for the code to work in its current form, but not strictly necessary. The difference here is that loadPtr (indirectly) does RELEASE_ASSERT(m_allowScratchRegister) because it explicitly invalidates the associated CachedRegister by calling cachedAddressTempRegister().invalidate().
> 
> One could have all callers of load*() invalidate the destination register instead, but that felt a bit more error prone. Especially since my base tree didn't include any caller that would need to skip the invalidation (I discovered this new usage when rebasing on Friday). I'm open to changing things around if anyone thinks that would be more appropriate (either in theory or in practice).

I think the change should be fine. It was actually me changing it to the current form in bug #236064. Even if the scratch register will be needed for address resolution, it will be used as the load destination at the very end of this operation, and it's mostly after where we want to actively disallow the usage.

(In reply to Zan Dobersek from comment #4)
> Comment on attachment 453209 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453209&action=review
> 
> >>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:358
> >>> +            DisallowMacroScratchRegisterUsage disallowScratch(jit);
> >> 
> >> Is this necessary? In theory it shouldn't cause problems, but it feels safer to me to disallow usage for the whole block, load including.
> > 
> > It's necessary for the code to work in its current form, but not strictly necessary. The difference here is that loadPtr (indirectly) does RELEASE_ASSERT(m_allowScratchRegister) because it explicitly invalidates the associated CachedRegister by calling cachedAddressTempRegister().invalidate().
> > 
> > One could have all callers of load*() invalidate the destination register instead, but that felt a bit more error prone. Especially since my base tree didn't include any caller that would need to skip the invalidation (I discovered this new usage when rebasing on Friday). I'm open to changing things around if anyone thinks that would be more appropriate (either in theory or in practice).
> 
> I think the change should be fine. It was actually me changing it to the
> current form in bug #236064. Even if the scratch register will be needed for
> address resolution, it will be used as the load destination at the very end
> of this operation, and it's mostly after where we want to actively disallow
> the usage.

Yah, I tracked down the commit that made this change ;-)

Still, I'm not 100% sure from the above which change you think should be fine. Would you rather I keep the loadPtr out of DisallowMacroScratchRegisterUsage (i.e. keep the submitted patch as is)?

Keep the submitted patch as-is, with the Disallow struct moved after loadPtr, where it used to be.

Committed r290589 (247867@main): <https://commits.webkit.org/247867@main>

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

<rdar://problem/89548061>

(In reply to Zan Dobersek from comment #6)
> Keep the submitted patch as-is, with the Disallow struct moved after
> loadPtr, where it used to be.

See also: https://bugs.webkit.org/show_bug.cgi?id=237888.