Bug 129657 – AbstractMacroAssembler::CachedTempRegister should start out invalid

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

[Michael Saboff]

Currently AbstractMacroAssembler::CachedTempRegister() are initialized with a value of 0 and that the contents are valid.  Instead they should be mark as invalid as we don't know the prior contents before the first use.

[Michael Saboff]

<rdar://problem/16214527>

[Michael Saboff]

Created attachment 225738 [details]
Patch

[Mark Lam]

Comment on attachment 225738 [details]
Patch

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

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:881
> +            , m_validBit(0 << static_cast<unsigned>(registerID))

Why not just "m_validBit(0)”?

[Michael Saboff]

(In reply to comment #3)
> (From update of attachment 225738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225738&action=review
> 
> > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:881
> > +            , m_validBit(0 << static_cast<unsigned>(registerID))
> 
> Why not just "m_validBit(0)”?

Actually, the is the wrong value to clear.  Updated patch coming.

[Geoffrey Garen]

Comment on attachment 225738 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        - Changed the setting of the valid bit to 0 (not valid) in the constructor as
> +          we don't know the contents of the register at the entry to the block we
> +          are going to generate code for.

Are you assuming that all JITs -- including the baseline JIT, the DFG, and the CSS selector JIT -- instantiate a new MacroAssembler at the head of each basic block?

[Michael Saboff]

Created attachment 225741 [details]
Updated patch

Updated after Mark caught that I was invalidating incorrectly.

(In reply to comment #5)
> (From update of attachment 225738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225738&action=review
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        - Changed the setting of the valid bit to 0 (not valid) in the constructor as
> > +          we don't know the contents of the register at the entry to the block we
> > +          are going to generate code for.
> 
> Are you assuming that all JITs -- including the baseline JIT, the DFG, and the CSS selector JIT -- instantiate a new MacroAssembler at the head of each basic block?

Poor choice of words.  I changed "block" to "code".  This fix has nothing to do with basic blocks.

Whenever we create a MacroAssembler, we need to start with the cached register values as invalid.  In the case that triggered this issue, we were generating small stubs and used the temporary registers to materialize constants that fit in 16 bits.

[Michael Saboff]

Committed r165038: <http://trac.webkit.org/changeset/165038>

[Darin Adler]

Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.