[webkit-reviews] review requested: [Bug 170215] WebAssembly: Air::Inst::generate crashes on large binary on A64 : [Attachment 308890] patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has asked  for review:
Bug 170215: WebAssembly: Air::Inst::generate crashes on large binary on A64
https://bugs.webkit.org/show_bug.cgi?id=170215

Attachment 308890: patch

https://bugs.webkit.org/attachment.cgi?id=308890&action=review




--- Comment #23 from JF Bastien <jfbastien at apple.com> ---
Created attachment 308890

  --> https://bugs.webkit.org/attachment.cgi?id=308890&action=review

patch

> > Source/JavaScriptCore/b3/B3Common.cpp:76
> > +	 return static_cast<GPRReg>(+MacroAssembler::dataTempRegister);
> 
> Why the +?

You found the 💎 gem 💎 hidden in this patch! 🎉

dataTempRegister in the header is actually a *declaration* and not a
*definition*. Even adding constexpr (as I did) doesn't make it a definition.
This causes the linker to crap itself on missing dataTempRegister  symbol. Sad
indeed! Why does it work elsewhere? ✨ The magic of C++ ✨ One fix is to switch
to C++17 and use inline variables, another is to just declare dataTempRegister
in the header and then add a definition in the .cpp file which is super ugly
and **is deprecated in C++17** in favor of inline variables. But the other
fix... the beautiful fix... is to turn the use of dataTempRegister into an
rvalue! This beautiful usage of unary plus magically makes the problem go away
because now we're not asking the linker to get this value from oh-so-far away
it would surely need a relocation! No, we're just rvalue-ing, nothing to see
here linker. So there you have it, unary plus isn't totally useless. It should
be useless, but here it's useful in compensating for something ridiculous.

Fun related fact! Unary plus is also valid (and useful!) for C++ lambdas:
  +[](){}


#cplusplus #awesome #hashtag


> > Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:128
> > +bool CCallSpecial::admitsExtendedOffsetAddr(Inst&, unsigned argIndex)
> > +{
> > +	 // The callee can be on the stack.
> > +	 if (argIndex == calleeArgOffset)
> > +	     return true;
> > +
> > +	 return false;
> > +}
> 
> I think this is outdated w.r.t ToT, where this should be false on ARM for
> the callee.

Ah right, fixed. I rebased and the issue is indeed gone!


> > Source/JavaScriptCore/b3/air/AirCCallSpecial.h:64
> > +	 void dumpImpl(PrintStream&) const final;
> 
> I think it's webkit style to just use override even if it's final, but I'm
> not 100% sure

Seems super weird. I added it because, reading the code, this wasn't obvious to
me. There's a tiny optimization argument to be made too, but it coms with the
world's tiniest violin for those missed cycles.


> > Source/JavaScriptCore/b3/air/AirGenerate.cpp:221
> > +		     AllowMacroScratchRegisterUsage allowScratch(jit);
> 
> Shouldn't this be AllowScratchIf(arm64())?

Yeah that's good, done.