Bug 123271 – Fix broken C Loop LLINT build

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

[Mark Lam]

Patch coming soon.

[Mark Lam]

Created attachment 215065 [details]
the patch.

[Michael Saboff]

Comment on attachment 215065 [details]
the patch.

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

> Source/JavaScriptCore/ChangeLog:7
> +

Please provide a description of what you did.

[Mark Lam]

Created attachment 215068 [details]
patch 2: with comments

[Michael Saboff]

Comment on attachment 215068 [details]
patch 2: with comments

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

r=me with typo fix.

> Source/JavaScriptCore/ChangeLog:23
> +        - The payByVal() macro reifies a slow path which is never taken in one case.

I think you mean *putByVal()*.

> Source/JavaScriptCore/ChangeLog:40
> +        * bytecode/CodeBlock.cpp:
> +        (JSC::CodeBlock::printGetByIdCacheStatus): Added an UNUSED_PARAM().
> +        (JSC::CodeBlock::dumpBytecode): Added #if ENABLE(JIT) to JIT only code.
> +        * bytecode/GetByIdStatus.cpp:
> +        (JSC::GetByIdStatus::computeFor): Added an UNUSED_PARAM().
> +        * bytecode/PutByIdStatus.cpp:
> +        (JSC::PutByIdStatus::computeFor): Added an UNUSED_PARAM().
> +        * bytecode/StructureStubInfo.h:
> +        - Added a stub StubInfoMap for non-JIT builds. StubInfoMap is still used
> +          in function prototypes even when !ENABLE(JIT). Rather that adding #if's
> +          in many places, we just provide a stub/placeholder implementation that
> +          is unused but keeps the compiler happy.
> +        * jit/JITOperations.h: Added #if ENABLE(JIT).
> +        * llint/LowLevelInterpreter32_64.asm:
> +        * llint/LowLevelInterpreter64.asm:
> +        - The payByVal() macro reifies a slow path which is never taken in one case.
> +          This translates into a label that is never used in the C Loop LLINT. The
> +          C++ compiler doesn't like unused labels. So, we fix this by adding a
> +          cloopUnusedLabel offline asm instruction that synthesizes the following:
> +
> +              if (false) goto unusedLabel;
> +
> +          This keeps the C++ compiler happy without changing code behavior.
> +        * offlineasm/cloop.rb: Implementing cloopUnusedLabel.
> +        * offlineasm/instructions.rb: Declaring cloopUnusedLabel.
> +        * runtime/Executable.cpp:
> +        (JSC::setupJIT): Added UNUSED_PARAM()s.
> +        (JSC::ScriptExecutable::prepareForExecutionImpl):
> +        - run-javascriptcore-tests have phases that forces the LLINT to be off
> +          which in turn asserts that the JIT is enabled. With the C Loop LLINT,
> +          this combination is illegal. So, we override the setup code here to
> +          always use the LLINT if !ENABLE(JIT) regardless of what options are
> +          passed in.

It didn't need to be this verbose.

[Mark Lam]

(In reply to comment #4)
> > Source/JavaScriptCore/ChangeLog:23
> > +        - The payByVal() macro reifies a slow path which is never taken in one case.
> 
> I think you mean *putByVal()*.

Of course.  Will fix.
 
> It didn't need to be this verbose.

No harm in being thorough, and it's only in the ChangeLog.  Glad to add details.

[Mark Lam]

Thanks for the review.  Typo fixed.  Landed in r157932: <http://trac.webkit.org/r157932>.

[Filip Pizlo]

Comment on attachment 215068 [details]
patch 2: with comments

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

Your fix is a really bad hack.

I don't like the idea of constantly adding random instructions to offlineasm to appease the cloop.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1394
> +        if C_LOOP
> +            cloopUnusedLabel slowPath
> +        end

This is ugly.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1253
> +        if C_LOOP
> +            cloopUnusedLabel slowPath
> +        end

This is ugly.

> Source/JavaScriptCore/offlineasm/cloop.rb:1115
> +        when "cloopUnusedLabel"
> +            $asm.putc "if (false) goto #{operands[0].cLabel};"
> +

Why don't you just add such statements for all labels at the top of the generated function, so that the cloop allows any label to be unused?

[Mark Lam]

(In reply to comment #7)
> Why don't you just add such statements for all labels at the top of the generated function, so that the cloop allows any label to be unused?

You, sir, are correct.  That is an excellent idea and I should have thought of it.  Will fix with another patch shortly.

[Mark Lam]

Created attachment 215075 [details]
patch 2+: applied Filip's suggested cleanup.

[Mark Lam]

Comment on attachment 215075 [details]
patch 2+: applied Filip's suggested cleanup.

Forgot the ChangeLog.

[Mark Lam]

Created attachment 215076 [details]
patch 2+: with ChangeLog this time.

[Geoffrey Garen]

Comment on attachment 215076 [details]
patch 2+: with ChangeLog this time.

r=me

[Mark Lam]

Thanks for the review.  Landed in r157937: <http://trac.webkit.org/r157937>.

[ChangSeok Oh]

*** Bug 123317 has been marked as a duplicate of this bug. ***