Bug 123271 - Fix broken C Loop LLINT build
Summary: Fix broken C Loop LLINT build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
: 123317 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-10-24 08:12 PDT by Mark Lam
Modified: 2013-10-31 23:13 PDT (History)
6 users (show)

See Also:


Attachments
the patch. (7.29 KB, patch)
2013-10-24 08:18 PDT, Mark Lam
msaboff: review-
Details | Formatted Diff | Diff
patch 2: with comments (8.68 KB, patch)
2013-10-24 08:51 PDT, Mark Lam
msaboff: review+
Details | Formatted Diff | Diff
patch 2+: applied Filip's suggested cleanup. (2.89 KB, patch)
2013-10-24 09:51 PDT, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
patch 2+: with ChangeLog this time. (3.64 KB, patch)
2013-10-24 09:55 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-10-24 08:12:54 PDT
Patch coming soon.
Comment 1 Mark Lam 2013-10-24 08:18:03 PDT
Created attachment 215065 [details]
the patch.
Comment 2 Michael Saboff 2013-10-24 08:23:41 PDT
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.
Comment 3 Mark Lam 2013-10-24 08:51:17 PDT
Created attachment 215068 [details]
patch 2: with comments
Comment 4 Michael Saboff 2013-10-24 08:56:34 PDT
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.
Comment 5 Mark Lam 2013-10-24 08:58:51 PDT
(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.
Comment 6 Mark Lam 2013-10-24 09:10:08 PDT
Thanks for the review.  Typo fixed.  Landed in r157932: <http://trac.webkit.org/r157932>.
Comment 7 Filip Pizlo 2013-10-24 09:34:22 PDT
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?
Comment 8 Mark Lam 2013-10-24 09:47:47 PDT
(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.
Comment 9 Mark Lam 2013-10-24 09:51:54 PDT
Created attachment 215075 [details]
patch 2+: applied Filip's suggested cleanup.
Comment 10 Mark Lam 2013-10-24 09:52:24 PDT
Comment on attachment 215075 [details]
patch 2+: applied Filip's suggested cleanup.

Forgot the ChangeLog.
Comment 11 Mark Lam 2013-10-24 09:55:35 PDT
Created attachment 215076 [details]
patch 2+: with ChangeLog this time.
Comment 12 Geoffrey Garen 2013-10-24 10:01:45 PDT
Comment on attachment 215076 [details]
patch 2+: with ChangeLog this time.

r=me
Comment 13 Mark Lam 2013-10-24 10:05:30 PDT
Thanks for the review.  Landed in r157937: <http://trac.webkit.org/r157937>.
Comment 14 ChangSeok Oh 2013-10-31 23:13:28 PDT
*** Bug 123317 has been marked as a duplicate of this bug. ***