WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
123271
Fix broken C Loop LLINT build
https://bugs.webkit.org/show_bug.cgi?id=123271
Summary
Fix broken C Loop LLINT build
Mark Lam
Reported
2013-10-24 08:12:54 PDT
Patch coming soon.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Mark Lam
Comment 1
2013-10-24 08:18:03 PDT
Created
attachment 215065
[details]
the patch.
Michael Saboff
Comment 2
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.
Mark Lam
Comment 3
2013-10-24 08:51:17 PDT
Created
attachment 215068
[details]
patch 2: with comments
Michael Saboff
Comment 4
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.
Mark Lam
Comment 5
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.
Mark Lam
Comment 6
2013-10-24 09:10:08 PDT
Thanks for the review. Typo fixed. Landed in
r157932
: <
http://trac.webkit.org/r157932
>.
Filip Pizlo
Comment 7
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?
Mark Lam
Comment 8
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.
Mark Lam
Comment 9
2013-10-24 09:51:54 PDT
Created
attachment 215075
[details]
patch 2+: applied Filip's suggested cleanup.
Mark Lam
Comment 10
2013-10-24 09:52:24 PDT
Comment on
attachment 215075
[details]
patch 2+: applied Filip's suggested cleanup. Forgot the ChangeLog.
Mark Lam
Comment 11
2013-10-24 09:55:35 PDT
Created
attachment 215076
[details]
patch 2+: with ChangeLog this time.
Geoffrey Garen
Comment 12
2013-10-24 10:01:45 PDT
Comment on
attachment 215076
[details]
patch 2+: with ChangeLog this time. r=me
Mark Lam
Comment 13
2013-10-24 10:05:30 PDT
Thanks for the review. Landed in
r157937
: <
http://trac.webkit.org/r157937
>.
ChangSeok Oh
Comment 14
2013-10-31 23:13:28 PDT
***
Bug 123317
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug