Bug 155598 - JSC should use a shadow stack version of CHICKEN so that debuggers have the option of retrieving tail-deleted frames
Summary: JSC should use a shadow stack version of CHICKEN so that debuggers have the o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 155684
  Show dependency treegraph
 
Reported: 2016-03-17 14:25 PDT by Filip Pizlo
Modified: 2016-04-06 13:52 PDT (History)
6 users (show)

See Also:


Attachments
work in progress (11.55 KB, patch)
2016-03-17 14:26 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (15.51 KB, patch)
2016-03-17 16:01 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (23.92 KB, patch)
2016-03-17 19:03 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (33.68 KB, patch)
2016-03-17 20:45 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
oh man so close (47.30 KB, patch)
2016-03-17 21:24 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
I think it's written (56.41 KB, patch)
2016-03-18 11:40 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it compiles! (78.69 KB, patch)
2016-03-18 12:51 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
oh man it's starting to work (89.28 KB, patch)
2016-03-18 14:52 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
getting close! (109.02 KB, patch)
2016-03-18 16:21 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
even more (111.19 KB, patch)
2016-03-18 20:01 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more chickens! (111.19 KB, patch)
2016-03-18 20:33 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
performance when disabled (64.46 KB, text/plain)
2016-03-18 21:02 PDT, Filip Pizlo
no flags Details
OMG it works! (113.17 KB, patch)
2016-03-18 21:20 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (114.31 KB, patch)
2016-03-18 22:22 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (115.63 KB, patch)
2016-03-18 22:44 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (117.62 KB, patch)
2016-03-19 14:15 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (117.67 KB, patch)
2016-03-19 14:21 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (121.01 KB, patch)
2016-03-19 15:22 PDT, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (904.74 KB, application/zip)
2016-03-19 16:39 PDT, Build Bot
no flags Details
the patch (122.11 KB, patch)
2016-03-19 18:19 PDT, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (980.95 KB, application/zip)
2016-03-19 19:38 PDT, Build Bot
no flags Details
the patch (136.69 KB, patch)
2016-03-19 21:13 PDT, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff
rebased patch (139.43 KB, patch)
2016-03-21 12:31 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
rebased patch (139.51 KB, patch)
2016-03-25 12:57 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-03-17 14:25:51 PDT
This will be so much fun.
Comment 1 Filip Pizlo 2016-03-17 14:26:31 PDT
Created attachment 274324 [details]
work in progress
Comment 2 Filip Pizlo 2016-03-17 16:01:47 PDT
Created attachment 274336 [details]
a bit more
Comment 3 Filip Pizlo 2016-03-17 19:03:23 PDT
Created attachment 274354 [details]
a bit more
Comment 4 Filip Pizlo 2016-03-17 20:45:34 PDT
Created attachment 274365 [details]
more
Comment 5 Filip Pizlo 2016-03-17 21:24:48 PDT
Created attachment 274369 [details]
oh man so close
Comment 6 Filip Pizlo 2016-03-18 11:40:45 PDT
Created attachment 274441 [details]
I think it's written
Comment 7 Filip Pizlo 2016-03-18 12:51:12 PDT
Created attachment 274448 [details]
it compiles!
Comment 8 Filip Pizlo 2016-03-18 14:52:37 PDT
Created attachment 274456 [details]
oh man it's starting to work

This algorithm is sort of nuts.
Comment 9 Filip Pizlo 2016-03-18 16:21:40 PDT
Created attachment 274472 [details]
getting close!

I can no longer think of tests with which to break it.
Comment 10 Filip Pizlo 2016-03-18 20:01:01 PDT
Created attachment 274493 [details]
even more
Comment 11 Filip Pizlo 2016-03-18 20:33:34 PDT
Created attachment 274495 [details]
more chickens!
Comment 12 Filip Pizlo 2016-03-18 21:02:27 PDT
Created attachment 274497 [details]
performance when disabled

It's neutral as expected.
Comment 13 Filip Pizlo 2016-03-18 21:15:11 PDT
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
Comment 14 Filip Pizlo 2016-03-18 21:20:14 PDT
Created attachment 274499 [details]
OMG it works!
Comment 15 WebKit Commit Bot 2016-03-18 21:23:23 PDT
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.
Comment 16 Filip Pizlo 2016-03-18 22:22:06 PDT
Created attachment 274503 [details]
the patch

Fixes for CMake and WebCore
Comment 17 WebKit Commit Bot 2016-03-18 22:33:07 PDT
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.
Comment 18 Filip Pizlo 2016-03-18 22:44:01 PDT
Created attachment 274508 [details]
the patch

More fixes
Comment 19 WebKit Commit Bot 2016-03-18 22:46:19 PDT
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.
Comment 20 Filip Pizlo 2016-03-19 14:03:55 PDT
(In reply to comment #18)
> Created attachment 274508 [details]
> the patch
> 
> More fixes

Wow, this patch got a perfect score on EWS!
Comment 21 Filip Pizlo 2016-03-19 14:04:28 PDT
Comment on attachment 274508 [details]
the patch

I'm still working through uses of the stack walking functor in WebCore.  New patch coming soon.
Comment 22 Filip Pizlo 2016-03-19 14:15:05 PDT
Created attachment 274521 [details]
the patch

More build fixes.  Let's see if this pleases EWS.
Comment 23 WebKit Commit Bot 2016-03-19 14:17:34 PDT
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.
Comment 24 Filip Pizlo 2016-03-19 14:21:31 PDT
Created attachment 274522 [details]
the patch
Comment 25 WebKit Commit Bot 2016-03-19 14:24:34 PDT
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.
Comment 26 Filip Pizlo 2016-03-19 15:22:04 PDT
Created attachment 274525 [details]
the patch

Annotated some FIXMEs with links to bugs.
Comment 27 WebKit Commit Bot 2016-03-19 15:29:19 PDT
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.
Comment 28 Build Bot 2016-03-19 16:38:58 PDT
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
Comment 29 Build Bot 2016-03-19 16:39:02 PDT
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
Comment 30 Filip Pizlo 2016-03-19 18:18:36 PDT
(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.
Comment 31 Filip Pizlo 2016-03-19 18:19:08 PDT
Created attachment 274529 [details]
the patch
Comment 32 WebKit Commit Bot 2016-03-19 18:21:39 PDT
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.
Comment 33 Build Bot 2016-03-19 19:38:15 PDT
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
Comment 34 Build Bot 2016-03-19 19:38:19 PDT
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
Comment 35 Filip Pizlo 2016-03-19 20:33:16 PDT
(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.
Comment 36 Filip Pizlo 2016-03-19 21:13:20 PDT
Created attachment 274533 [details]
the patch

Fixes NativeCallFrameTracer misuse.
Comment 37 WebKit Commit Bot 2016-03-19 21:16:44 PDT
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.
Comment 38 Saam Barati 2016-03-21 08:05:06 PDT
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?
Comment 39 Filip Pizlo 2016-03-21 10:08:37 PDT
(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.
Comment 40 Saam Barati 2016-03-21 10:48:13 PDT
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.
Comment 41 Filip Pizlo 2016-03-21 12:21:16 PDT
(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;
Comment 42 Filip Pizlo 2016-03-21 12:22:24 PDT
(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.
Comment 43 Filip Pizlo 2016-03-21 12:31:51 PDT
Created attachment 274617 [details]
rebased patch
Comment 44 WebKit Commit Bot 2016-03-21 12:34:49 PDT
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.
Comment 45 Filip Pizlo 2016-03-25 12:57:13 PDT
Created attachment 274930 [details]
rebased patch
Comment 46 WebKit Commit Bot 2016-03-25 13:01:15 PDT
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.
Comment 47 Filip Pizlo 2016-04-05 15:18:02 PDT
Landed in http://trac.webkit.org/changeset/199076