Bug 155598 – JSC should use a shadow stack version of CHICKEN so that debuggers have the option of retrieving tail-deleted frames

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

[Filip Pizlo]

This will be so much fun.

[Filip Pizlo]

Created attachment 274324 [details]
work in progress

[Filip Pizlo]

Created attachment 274336 [details]
a bit more

[Filip Pizlo]

Created attachment 274354 [details]
a bit more

[Filip Pizlo]

Created attachment 274365 [details]
more

[Filip Pizlo]

Created attachment 274369 [details]
oh man so close

[Filip Pizlo]

Created attachment 274441 [details]
I think it's written

[Filip Pizlo]

Created attachment 274448 [details]
it compiles!

[Filip Pizlo]

Created attachment 274456 [details]
oh man it's starting to work

This algorithm is sort of nuts.

[Filip Pizlo]

Created attachment 274472 [details]
getting close!

I can no longer think of tests with which to break it.

[Filip Pizlo]

Created attachment 274493 [details]
even more

[Filip Pizlo]

Created attachment 274495 [details]
more chickens!

[Filip Pizlo]

Created attachment 274497 [details]
performance when disabled

It's neutral as expected.

[Filip Pizlo]

Holy shucks, the overhead is small enough that it's justified in the Inspector.

Benchmark report for SunSpider on hodor (MacBookPro11,3).

VMs tested:
"Normal" at /Volumes/Data/quartary/OpenSource/WebKitBuild/Release/jsc (r198446)
    export JSC_alwaysUseShadowChicken=false
"ShadowChicken" at /Volumes/Data/quartary/OpenSource/WebKitBuild/Release/jsc (r198446)
    export JSC_alwaysUseShadowChicken=true

Collected 6 samples per benchmark/VM, with 6 VM invocations per benchmark. Emitted a call to gc() between
sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific
preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95%
confidence intervals in milliseconds.

                                    Normal                ShadowChicken                                   

3d-cube                         4.9215+-0.1697     ?      5.0382+-0.3613        ? might be 1.0237x slower
3d-morph                        5.0916+-0.1348     ?      5.1506+-0.2321        ? might be 1.0116x slower
3d-raytrace                     5.5032+-0.2597     ?      5.6611+-0.2420        ? might be 1.0287x slower
access-binary-trees             2.2430+-0.3222     ?      2.5354+-0.3202        ? might be 1.1304x slower
access-fannkuch                 5.9056+-0.2845     ?      6.0109+-0.2713        ? might be 1.0178x slower
access-nbody                    2.4830+-0.0617     ?      2.5111+-0.1209        ? might be 1.0114x slower
access-nsieve                   3.0358+-0.2501     ?      3.2667+-0.5124        ? might be 1.0761x slower
bitops-3bit-bits-in-byte        1.2239+-0.3024            1.1980+-0.0571          might be 1.0216x faster
bitops-bits-in-byte             2.7260+-0.0284     !      3.1909+-0.1776        ! definitely 1.1706x slower
bitops-bitwise-and              2.0677+-0.0525     ?      2.1081+-0.1563        ? might be 1.0195x slower
bitops-nsieve-bits              3.0645+-0.0457     ?      3.1314+-0.2634        ? might be 1.0218x slower
controlflow-recursive           2.3658+-0.1855     !      2.7710+-0.1463        ! definitely 1.1713x slower
crypto-aes                      3.8422+-0.0929     ?      4.0840+-0.2342        ? might be 1.0629x slower
crypto-md5                      2.4340+-0.0576     !      2.6661+-0.1031        ! definitely 1.0954x slower
crypto-sha1                     2.3486+-0.0945     !      2.7737+-0.0557        ! definitely 1.1810x slower
date-format-tofte               6.7001+-0.3289     ?      7.6587+-1.9373        ? might be 1.1431x slower
date-format-xparb               4.8301+-0.6818            4.7498+-0.1174          might be 1.0169x faster
math-cordic                     2.7450+-0.0478     !      3.0096+-0.1324        ! definitely 1.0964x slower
math-partial-sums               4.7944+-0.2981            4.7477+-0.0947        
math-spectral-norm              2.0393+-0.1797            2.0212+-0.0550        
regexp-dna                      6.2795+-0.2810     ?      6.3438+-0.3490        ? might be 1.0102x slower
string-base64                   4.4591+-0.1782     ?      4.6616+-0.4740        ? might be 1.0454x slower
string-fasta                    5.7286+-0.1556     ?      6.5780+-1.8318        ? might be 1.1483x slower
string-tagcloud                 8.1623+-0.4249     ?      8.3689+-0.5617        ? might be 1.0253x slower
string-unpack-code             18.8666+-1.9965     ?     19.8424+-1.7583        ? might be 1.0517x slower
string-validate-input           4.1469+-0.1437     ?      4.7496+-0.9823        ? might be 1.1454x slower

<arithmetic>                    4.5388+-0.1076     !      4.8011+-0.1274        ! definitely 1.0578x slower

[Filip Pizlo]

Created attachment 274499 [details]
OMG it works!

[WebKit Commit Bot]

Attachment 274499 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:250:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 274503 [details]
the patch

Fixes for CMake and WebCore

[WebKit Commit Bot]

Attachment 274503 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:250:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 274508 [details]
the patch

More fixes

[WebKit Commit Bot]

Attachment 274508 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:250:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #18)
> Created attachment 274508 [details]
> the patch
> 
> More fixes

Wow, this patch got a perfect score on EWS!

[Filip Pizlo]

Comment on attachment 274508 [details]
the patch

I'm still working through uses of the stack walking functor in WebCore.  New patch coming soon.

[Filip Pizlo]

Created attachment 274521 [details]
the patch

More build fixes.  Let's see if this pleases EWS.

[WebKit Commit Bot]

Attachment 274521 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:250:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 274522 [details]
the patch

[WebKit Commit Bot]

Attachment 274522 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:161:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:250:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 274525 [details]
the patch

Annotated some FIXMEs with links to bugs.

[WebKit Commit Bot]

Attachment 274525 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:162:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:162:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:265:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 274525 [details]
the patch

Attachment 274525 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1007256

New failing tests:
inspector/debugger/call-frame-this-nonstrict.html

[Build Bot]

Created attachment 274526 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Filip Pizlo]

(In reply to comment #29)
> Created attachment 274526 [details]
> Archive of layout-test-results from ews116 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

I know why.  I forgot to store the CallSiteIndex before calling the ShadowChicken log flusher.

[Filip Pizlo]

Created attachment 274529 [details]
the patch

[WebKit Commit Bot]

Attachment 274529 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:162:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:162:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:265:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 66 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 274529 [details]
the patch

Attachment 274529 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1007872

New failing tests:
webgl/1.0.2/conformance/glsl/misc/shader-uniform-packing-restrictions.html

[Build Bot]

Created attachment 274532 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Filip Pizlo]

(In reply to comment #34)
> Created attachment 274532 [details]
> Archive of layout-test-results from ews114 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

I know the issue.  Some code was forgetting to use NativeCallFrameTracer.

[Filip Pizlo]

Created attachment 274533 [details]
the patch

Fixes NativeCallFrameTracer misuse.

[WebKit Commit Bot]

Attachment 274533 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:162:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:162:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:265:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 274533 [details]
the patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3073
> +void BytecodeGenerator::emitLogShadowChickenPrologueIfNecessary()
> +{
> +    if (!m_shouldEmitDebugHooks && !Options::alwaysUseShadowChicken())
> +        return;
> +    emitOpcode(op_log_shadow_chicken_prologue);
> +}
> +
> +void BytecodeGenerator::emitLogShadowChickenTailIfNecessary()
> +{
> +    if (!m_shouldEmitDebugHooks && !Options::alwaysUseShadowChicken())
> +        return;
> +    emitOpcode(op_log_shadow_chicken_tail);
> +}

I think we should be logging currentScope() too to make this compatible with the inspector. Maybe this can be done as a follow up patch?

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:229
> +    case op_log_shadow_chicken_prologue:
> +    case op_log_shadow_chicken_tail:

I wonder if this should just be CanCompile for now with a FIXME to add inlining support.
I'm guessing you run your tests with DFGJIT=false so inlining isn't a problem.
If we prevent inlining then the tests should be able to run in all tiers.

> Source/JavaScriptCore/interpreter/ShadowChicken.cpp:83
> +    *m_logCursor++ = packet;

Style: Can we write this as two lines?

> Source/JavaScriptCore/interpreter/ShadowChicken.cpp:96
> +    // that by figuring out how much of the shadow stack top pop. We apply three different rules.

"top pop" => "to pop"

> Source/JavaScriptCore/runtime/VM.h:683
> +    std::unique_ptr<ShadowChicken> m_shadowChicken;

Why not just "ShadowChucken m_shadowChicken"?

> Tools/Scripts/run-jsc-stress-tests:851
> +    run("shadow-chicken", "--useDFGJIT=false", "--alwaysUseShadowChicken=true")

Why useDFGJIT=false?

[Filip Pizlo]

(In reply to comment #38)
> Comment on attachment 274533 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274533&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3073
> > +void BytecodeGenerator::emitLogShadowChickenPrologueIfNecessary()
> > +{
> > +    if (!m_shouldEmitDebugHooks && !Options::alwaysUseShadowChicken())
> > +        return;
> > +    emitOpcode(op_log_shadow_chicken_prologue);
> > +}
> > +
> > +void BytecodeGenerator::emitLogShadowChickenTailIfNecessary()
> > +{
> > +    if (!m_shouldEmitDebugHooks && !Options::alwaysUseShadowChicken())
> > +        return;
> > +    emitOpcode(op_log_shadow_chicken_tail);
> > +}
> 
> I think we should be logging currentScope() too to make this compatible with
> the inspector. Maybe this can be done as a follow up patch?

Filed: https://bugs.webkit.org/show_bug.cgi?id=155722

I added a FIXME that references this bug.

> 
> > Source/JavaScriptCore/dfg/DFGCapabilities.cpp:229
> > +    case op_log_shadow_chicken_prologue:
> > +    case op_log_shadow_chicken_tail:
> 
> I wonder if this should just be CanCompile for now with a FIXME to add
> inlining support.
> I'm guessing you run your tests with DFGJIT=false so inlining isn't a
> problem.
> If we prevent inlining then the tests should be able to run in all tiers.

We can run ShadowChicken with inlining enabled even now.  It'll just give janky results.  I sort of like this because it means that we can measure, and even track, the ShadowChicken overhead when inlining is enabled.  This is an interesting thing to measure since inlining obviates the need for prologue and tail packets at the inlining boundary, which will reduce overhead further.

> 
> > Source/JavaScriptCore/interpreter/ShadowChicken.cpp:83
> > +    *m_logCursor++ = packet;
> 
> Style: Can we write this as two lines?

We could, but is this a WK style rule?  This is a common C idiom, and I think we do things like this in other places.

> 
> > Source/JavaScriptCore/interpreter/ShadowChicken.cpp:96
> > +    // that by figuring out how much of the shadow stack top pop. We apply three different rules.
> 
> "top pop" => "to pop"

Fixed.

> 
> > Source/JavaScriptCore/runtime/VM.h:683
> > +    std::unique_ptr<ShadowChicken> m_shadowChicken;
> 
> Why not just "ShadowChucken m_shadowChicken"?

If we do it that way, then we have to #include "ShadowChicken.h" in VM.h.  That would be a compile-time regression since VM.h is included from everywhere.  I think it's better to have VM use unique_ptr for its helper objects, to reduce the number of headers that VM.h needs to include.

> 
> > Tools/Scripts/run-jsc-stress-tests:851
> > +    run("shadow-chicken", "--useDFGJIT=false", "--alwaysUseShadowChicken=true")
> 
> Why useDFGJIT=false?

ShadowChicken will "run" with DFG and FTL but it will report inaccurate results in the presence of inlining.  Also, this test doesn't really put the DFG or FTL through its paces - the only looping is the tail-recursion.  So, rather than disabling inlining, I just disabled optimizations for this test.

In this patch, the DFG and FTL support for ShadowChicken is only tested by a new .yaml file (sorry, this patch accidentally excluded it) that runs SunSpider and V8Spider with ShadowChicken.  It doesn't validate the ShadowChicken stack - it's just a test to see if we don't crash.

[Saam Barati]

Comment on attachment 274533 [details]
the patch

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

>>> Source/JavaScriptCore/interpreter/ShadowChicken.cpp:83
>>> +    *m_logCursor++ = packet;
>> 
>> Style: Can we write this as two lines?
> 
> We could, but is this a WK style rule?  This is a common C idiom, and I think we do things like this in other places.

I have no idea if it is or isn't WK style. I just personally dislike it even though it's a common C idiom
It's up to you if you want to change or not.

[Filip Pizlo]

(In reply to comment #40)
> Comment on attachment 274533 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274533&action=review
> 
> >>> Source/JavaScriptCore/interpreter/ShadowChicken.cpp:83
> >>> +    *m_logCursor++ = packet;
> >> 
> >> Style: Can we write this as two lines?
> > 
> > We could, but is this a WK style rule?  This is a common C idiom, and I think we do things like this in other places.
> 
> I have no idea if it is or isn't WK style. I just personally dislike it even
> though it's a common C idiom
> It's up to you if you want to change or not.

I think you're right in this case.  That code is misleading.  In fact, we know that after a call to update, m_logCursor == m_log.  I'll change it to:

ASSERT(m_log == m_logCursor);
m_log[0] = packet;
m_logCursor = m_log + 1;

[Filip Pizlo]

(In reply to comment #41)
> (In reply to comment #40)
> > Comment on attachment 274533 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=274533&action=review
> > 
> > >>> Source/JavaScriptCore/interpreter/ShadowChicken.cpp:83
> > >>> +    *m_logCursor++ = packet;
> > >> 
> > >> Style: Can we write this as two lines?
> > > 
> > > We could, but is this a WK style rule?  This is a common C idiom, and I think we do things like this in other places.
> > 
> > I have no idea if it is or isn't WK style. I just personally dislike it even
> > though it's a common C idiom
> > It's up to you if you want to change or not.
> 
> I think you're right in this case.  That code is misleading.  In fact, we
> know that after a call to update, m_logCursor == m_log.  I'll change it to:
> 
> ASSERT(m_log == m_logCursor);
> m_log[0] = packet;
> m_logCursor = m_log + 1;

Ha, I can't believe I just said that, because it's wrong!  There's actually a corner case where m_logCursor will not be exactly at m_log, because we sometimes defer tail packets.

I'll keep it the way it was.

[Filip Pizlo]

Created attachment 274617 [details]
rebased patch

[WebKit Commit Bot]

Attachment 274617 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:165:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:165:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:265:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 274930 [details]
rebased patch

[WebKit Commit Bot]

Attachment 274930 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:71:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/interpreter/StackVisitor.h:66:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:165:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.h:165:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/interpreter/ShadowChicken.cpp:265:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

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