[webkit-reviews] review denied: [Bug 122332] FTL: Crash in OSRExit::convertToForward() using VirtualRegister.offset() as array index : [Attachment 213381] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied  review:
Bug 122332: FTL: Crash in OSRExit::convertToForward() using
VirtualRegister.offset() as array index
https://bugs.webkit.org/show_bug.cgi?id=122332

Attachment 213381: Patch
https://bugs.webkit.org/attachment.cgi?id=213381&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213381&action=review


> Source/JavaScriptCore/ftl/FTLOSRExit.cpp:90
> -    if (m_values[overriddenOperand.offset()].isArgument()) {
> -	   ExitArgument exitArgument =
m_values[overriddenOperand.offset()].exitArgument();
> +    if (m_values[overriddenOperand.toLocal()].isArgument()) {
> +	   ExitArgument exitArgument =
m_values[overriddenOperand.toLocal()].exitArgument();
>	   arguments[exitArgument.argument()] = value.value();
> -	   m_values[overriddenOperand.offset()] = ExitValue::exitArgument(
> +	   m_values[overriddenOperand.toLocal()] = ExitValue::exitArgument(

I think that this is still wrong.  m_values is an Operands<>, which can be
indexed in two ways:

Operands<>::operator[]: this is only meant for iterating over all of the
entries, and the index is meaningless. it is neither the offset nor the local -
it's nothing.  Hence this code was totally wrong to begin with for using
m_values[...], and only worked by accident.

Operands<>::operand(): this takes a VirtualRegister.

There are other indexing modes like Operands<>::local() and
Operands<>::argument(), but those are less useful since usually in the DFG and
FTL we don't know if something is a local or an argument and we don't really
care.

In this code, overriddenOperand could be an argument, maybe if you did
something like:

function foo(x) { x = x | 0 }

I don't know if you'll actually get a MovHint into the argument here, but that
would be valid IR for this byte code so we should be able to handle it.

So, the correct thing to do here is to say:

if (m_values.operand(overriddenOperand).isArgument()) { ...

And:

m_values.operand(overriddenOperand) = ExitValue::exitArgument( ...

That way, it'll handle both locals and arguments and there will be no confusion
over what to use as indices.