FTL: Crash in OSRExit::convertToForward() using VirtualRegister.offset() as array index

[Michael Saboff]

There are several instances in OSRExit::convertToForward() where we are using the offset() method on a virtual register as an array index.  The should be toLocal() calls instead.

[Nadav Rotem]

Looks Good To Me (LGTM).

[Michael Saboff]

Created attachment 213381 [details]
Patch

[Michael Saboff]

Committed r156900: <http://trac.webkit.org/changeset/156900>

[Filip Pizlo]

Comment on attachment 213381 [details]
Patch

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.

[Michael Saboff]

I'll make the changes that Phil suggests and submit a new patch.

[Filip Pizlo]

(In reply to comment #5)
> I'll make the changes that Phil suggests and submit a new patch.

Can you fix it in: https://bugs.webkit.org/show_bug.cgi?id=122336

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #5)
> > I'll make the changes that Phil suggests and submit a new patch.
> 
> Can you fix it in: https://bugs.webkit.org/show_bug.cgi?id=122336

(or dup appropriately if you already created your own bug.)

[Michael Saboff]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > I'll make the changes that Phil suggests and submit a new patch.
> > 
> > Can you fix it in: https://bugs.webkit.org/show_bug.cgi?id=122336
> 
> (or dup appropriately if you already created your own bug.)

I'll fix it in 122336.  Testing now.