[webkit-reviews] review denied: [Bug 132333] [CLOOP] Operand in PutToScope and GetFromScope is not set right causing crashes on big endian arches : [Attachment 311689] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Tomas Popela <tpopela at redhat.com>'s
request for review:
Bug 132333: [CLOOP] Operand in PutToScope and GetFromScope is not set right
causing crashes on big endian arches
https://bugs.webkit.org/show_bug.cgi?id=132333

Attachment 311689: Patch

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




--- Comment #22 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 311689
  --> https://bugs.webkit.org/attachment.cgi?id=311689
Patch

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

I understand Filip's concern and generally agree with him that we should have a
less fragile solution in the long term.  But, I see value in this patch in that
it documents some of the idiosyncrasies of our bytecode.  That said, there are
issues that need to be resolved, and you need to do some extra homework first
to resolve these.  See below ...

r- until issues are resolved because we don't want to land something that may
silently destabilize the VM.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:665
> +	       if (op.type == ClosureVar || op.type ==
ClosureVarWithVarInjectionChecks || op.type == GlobalProperty || op.type ==
GlobalPropertyWithVarInjectionChecks || op.type == ModuleVar)
> +		   instructions[i + 6].u.operand = op.operand;
> +	       else
> +		   instructions[i + 6].u.pointer =
reinterpret_cast<void*>(op.operand);

Looking at LowLevelInterpreter64.asm, I see that ClosureVar,
ClosureVarWithVarInjectionChecks, GlobalProperty, and
GlobalPropertyWithVarInjectionChecks calls macros that do loadis from the 6th
operand.  However, I didn't see ModuleVar do the same.	Can you provide details
of why ModuleVar should be included in this set?

Secondly, I also see that GlobalProperty, GlobalPropertyWithVarInjectionChecks,
and ModuleVar also calls getConstantScope() which does loadp from the 6th
operand.  This is in disagreement with the above.  Can you research why this is
ok?  Based on this info, your solution may be incomplete, or in error, or
perhaps operand 6 is overloaded and used 2 ways depending on context.  Please
get the data that shows us what the solution should be.