Bug 141211 – Remove BytecodeGenerator::preserveLastVar() and replace it with a more robust mechanism for preserving non-temporary registers

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Get rid of it.

[Filip Pizlo]

Created attachment 245969 [details]
the patch

[Mark Lam]

Comment on attachment 245969 [details]
the patch

r=me

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 245969 [details]
> the patch
> 
> r=me

Invalid patch, will upload a new one.

[Filip Pizlo]

Created attachment 245970 [details]
the patch

[Filip Pizlo]

Created attachment 245971 [details]
the patch

[Mark Lam]

Comment on attachment 245971 [details]
the patch

r=me

[Filip Pizlo]

(In reply to comment #6)
> Comment on attachment 245971 [details]
> the patch
> 
> r=me

Lol, my patch is still wrong. ;-)

[Filip Pizlo]

Created attachment 246035 [details]
the patch

For real this time!

[Mark Lam]

Comment on attachment 246035 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:47
> [phone]  Filip Pizlo  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +        
> +        Also removes the localArgumentsRegister local variable in BytecodeGenerator's
> +        constructor. It's not used.
> +
> +        * bytecompiler/BytecodeGenerator.cpp:
> +        (JSC::BytecodeGenerator::BytecodeGenerator):
> +        (JSC::BytecodeGenerator::preserveLastVar): Deleted.
> +        * bytecompiler/BytecodeGenerator.h:
> +        (JSC::BytecodeGenerator::addVar):
> +

Leftover edit that should have been removed?

> Source/JavaScriptCore/bytecompiler/RegisterID.h:75
> -            ASSERT(!m_refCount);
>  #ifndef NDEBUG
>              m_didSetIndex = true;
>  #endif

I think the main reason for the ASSERT(!m_refCount) is to ensure that the index is only set once before use.  Though the previous implementation allows us to “change our minds” and set the index to something else (which I don’t think is likely to happen) as long as we haven’t started using the RegisterID yet, I think the real bug it’s trying to watch out for is that we don’t use the RegisterID’s index in one place and then change it before using it in another (which is also unlikely to happen but if it does, it can cause havoc).

How about you replace this assert with ASSERT(!m_didSetIndex) in the #ifndef NDEBUG section instead?  Would that work?

[Filip Pizlo]

(In reply to comment #9)
> Comment on attachment 246035 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246035&action=review
> 
> > Source/JavaScriptCore/ChangeLog:47
> > [phone]  Filip Pizlo  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +        
> > +        Also removes the localArgumentsRegister local variable in BytecodeGenerator's
> > +        constructor. It's not used.
> > +
> > +        * bytecompiler/BytecodeGenerator.cpp:
> > +        (JSC::BytecodeGenerator::BytecodeGenerator):
> > +        (JSC::BytecodeGenerator::preserveLastVar): Deleted.
> > +        * bytecompiler/BytecodeGenerator.h:
> > +        (JSC::BytecodeGenerator::addVar):
> > +
> 
> Leftover edit that should have been removed?

Yup, I removed it locally.

> 
> > Source/JavaScriptCore/bytecompiler/RegisterID.h:75
> > -            ASSERT(!m_refCount);
> >  #ifndef NDEBUG
> >              m_didSetIndex = true;
> >  #endif
> 
> I think the main reason for the ASSERT(!m_refCount) is to ensure that the
> index is only set once before use.  Though the previous implementation
> allows us to “change our minds” and set the index to something else (which I
> don’t think is likely to happen) as long as we haven’t started using the
> RegisterID yet, I think the real bug it’s trying to watch out for is that we
> don’t use the RegisterID’s index in one place and then change it before
> using it in another (which is also unlikely to happen but if it does, it can
> cause havoc).
> 
> How about you replace this assert with ASSERT(!m_didSetIndex) in the #ifndef
> NDEBUG section instead?  Would that work?

But this does get called when m_didSetIndex is true.

[Mark Lam]

Comment on attachment 246035 [details]
the patch

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

r=me with ChangeLog fix.

>>> Source/JavaScriptCore/bytecompiler/RegisterID.h:75
>>>  #endif
>> 
>> I think the main reason for the ASSERT(!m_refCount) is to ensure that the index is only set once before use.  Though the previous implementation allows us to “change our minds” and set the index to something else (which I don’t think is likely to happen) as long as we haven’t started using the RegisterID yet, I think the real bug it’s trying to watch out for is that we don’t use the RegisterID’s index in one place and then change it before using it in another (which is also unlikely to happen but if it does, it can cause havoc).
>> 
>> How about you replace this assert with ASSERT(!m_didSetIndex) in the #ifndef NDEBUG section instead?  Would that work?
> 
> But this does get called when m_didSetIndex is true.

OK.  For Filip explained offline that for a scenario like:  function(a) { var a; }, the var a overrides argument a.  As such, the index for a’s RegisterID will be changed to match that of the argument’s.  Hence, m_didSetIndex is already true there.

[Mark Lam]

(In reply to comment #11)
> OK.  For Filip explained offline that ...

I meant: For the record, Filip explained ...

Ahh typos.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/179746