Bug 182155 – Apply poisoning to TypedArray vector pointers.

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

[Mark Lam]

<rdar://problem/36286266>

[Mark Lam]

Created attachment 332330 [details]
work in progress for EWS testing.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


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

[Mark Lam]

Created attachment 332332 [details]
work in progress for EWS testing.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


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

[Mark Lam]

Comment on attachment 332332 [details]
work in progress for EWS testing.

I think I've found bugs that I need to work on.  No need to continue testing.

[Mark Lam]

Created attachment 332355 [details]
work in progress for EWS testing.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


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

[EWS Watchlist]

Comment on attachment 332355 [details]
work in progress for EWS testing.

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

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332356 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332355 [details]
work in progress for EWS testing.

Attachment 332355 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6218730

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332357 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332355 [details]
work in progress for EWS testing.

Attachment 332355 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6219345

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332361 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Mark Lam]

Created attachment 332389 [details]
work in progress for EWS testing.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


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

[EWS Watchlist]

Comment on attachment 332389 [details]
work in progress for EWS testing.

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

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332400 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332389 [details]
work in progress for EWS testing.

Attachment 332389 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6223760

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332404 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332389 [details]
work in progress for EWS testing.

Attachment 332389 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6223948

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332405 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332389 [details]
work in progress for EWS testing.

Attachment 332389 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6224335

New failing tests:
stress/array-message-passing.js.ftl-eager-no-cjit

[EWS Watchlist]

Comment on attachment 332389 [details]
work in progress for EWS testing.

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

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332431 [details]
Archive of layout-test-results from ews113 for mac-sierra

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

[Mark Lam]

Created attachment 332584 [details]
work in progress for EWS testing.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 21 files


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

[EWS Watchlist]

Comment on attachment 332584 [details]
work in progress for EWS testing.

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

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332595 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332584 [details]
work in progress for EWS testing.

Attachment 332584 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6253901

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332596 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332584 [details]
work in progress for EWS testing.

Attachment 332584 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6253855

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332598 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332584 [details]
work in progress for EWS testing.

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

New failing tests:
fast/canvas/webgl/arraybuffer-transfer-of-control.html

[EWS Watchlist]

Created attachment 332601 [details]
Archive of layout-test-results from ews116 for mac-sierra

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

[Mark Lam]

Created attachment 332630 [details]
work in progress for EWS testing.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 22 files


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

[Mark Lam]

Created attachment 332680 [details]
proposed patch.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 20 files


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

[Mark Lam]

Comment on attachment 332680 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:5
> +        Apply poisoning to TypedArray vector pointers.
> +        https://bugs.webkit.org/show_bug.cgi?id=182155
> +        <rdar://problem/36286266>

I should add a comment about how I poison the vector pointers.  Basically, ...

The TypeArray's vector pointer is now poisoned.  The poison value is chosen based on a TypeArray's jsType.  The JSType must be between FirstTypedArrayType and LastTypedArrayType.  At runtime, we enforce that the index is well-behaved by masking it against TypedArrayPoisonIndexMask.  TypedArrayPoisonIndexMask (16) is the number of TypedArray types (10) rounded up to the next power of 2.  Accordingly, we reserve an array of TypedArrayPoisonIndexMask poisons so that we can use index masking on the index.

[EWS Watchlist]

Comment on attachment 332680 [details]
proposed patch.

Attachment 332680 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/6264300

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-cjit

[Mark Lam]

(In reply to Build Bot from comment #40)
> Comment on attachment 332680 [details]
> proposed patch.
> 
> Attachment 332680 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/6264300
> 
> New failing tests:
> stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-
> cjit

Strange, but I can't seem to reproduce this locally.  Still trying.

[JF Bastien]

Comment on attachment 332680 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6388
> +    GPRReg indexGPR = index.gpr();

Assert that poison/index aren't the same as base/vector?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6408
> +    m_jit.xorPtr(poisonGPR, vectorGPR);

Blow away the poisonGPR content after?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3536
> +        poisonedVector = m_out.bitXor(poisonedVector, poison);

Zero out the poison Lvalue after?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:402
> +        xorp scratch, dest

No need to zero out scratch if GIGACAGE_ENABLED and not C_LOOP because it'll be clobbered. I guess we don't care otherwise?

> Source/JavaScriptCore/runtime/CagedBarrierPtr.h:34
> +template<typename Poison, typename T> struct PoisonedPtrTraits;

Include Forwards.h instead.

> Source/JavaScriptCore/runtime/JSArrayBufferView.h:226
> +    PoisonedCagedBarrierPtr<Poison, Gigacage::Primitive, void> m_poisonedVector;

🤯 a vector of poisons, held in a poisoned caged barrier pointer 🤯

> Source/WTF/wtf/CagedPtr.h:38
> +    explicit CagedPtr(T* ptr = nullptr)

Do you want to add a nullptr_t ctor since this one is now explicit (if someone doesn't want to rely on default)?

> Source/WTF/wtf/CagedPtr.h:98
> +    explicit CagedPtr(void* ptr = nullptr)

Ditto.

[Mark Lam]

Comment on attachment 332680 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6388
>> +    GPRReg indexGPR = index.gpr();
> 
> Assert that poison/index aren't the same as base/vector?

Don't need an assert here.  All 4 values are local GPRTemporarys just instantiated above.  Hence, they are guaranteed to be mutually exclusive with each other and with base.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6408
>> +    m_jit.xorPtr(poisonGPR, vectorGPR);
> 
> Blow away the poisonGPR content after?

Fixed.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:402
>> +        xorp scratch, dest
> 
> No need to zero out scratch if GIGACAGE_ENABLED and not C_LOOP because it'll be clobbered. I guess we don't care otherwise?

Fixed.

>> Source/JavaScriptCore/runtime/CagedBarrierPtr.h:34
>> +template<typename Poison, typename T> struct PoisonedPtrTraits;
> 
> Include Forwards.h instead.

I'll fix this in a subsequent patch.  Currently, no one #include Forward.h for this decl.  We can change that, but it shouldn't be in just this one place.

>> Source/WTF/wtf/CagedPtr.h:38
>> +    explicit CagedPtr(T* ptr = nullptr)
> 
> Do you want to add a nullptr_t ctor since this one is now explicit (if someone doesn't want to rely on default)?

Fixed.

>> Source/WTF/wtf/CagedPtr.h:98
>> +    explicit CagedPtr(void* ptr = nullptr)
> 
> Ditto.

Fixed.

[Mark Lam]

Created attachment 332737 [details]
Patch for landing.

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/CagedPtr.h:38:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:101:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:102:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 20 files


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

[Mark Lam]

(In reply to Build Bot from comment #40)
> New failing tests:
> stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-
> cjit

1. I can't reproduce this failure locally.
2. The failing test doesn't even use TypedArrays.

So, I'm quite sure this is not related to my patch.

[Mark Lam]

Created attachment 332739 [details]
patch for landing.

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/CagedPtr.h:38:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:101:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/CagedPtr.h:102:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/JSCPoison.cpp:40:  g_typedArrayPoisons is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 20 files


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

[Mark Lam]

Landed in r227874: <http://trac.webkit.org/r227874>.

[Mark Lam]

Build fix for CLoop landed in r227929: <http://trac.webkit.org/r227929>.