Changeset 280659 in webkit

Timestamp:

Aug 4, 2021, 2:17:57 PM (5 years ago)

Author:

Yijia Huang

Message:

[ARM64] Fix Zoom black screen during video meeting on Safari
​https://bugs.webkit.org/show_bug.cgi?id=228776

Reviewed by Saam Barati.

The problem (rdar://81434487) reports that Zoom turns to a black screen during the video
meeting on Safari. The reproduction of this problem is verified and bisected to the previous patch
(​https://bugs.webkit.org/show_bug.cgi?id=228057). Previously, we introduce a pattern
matching for instruction EON-with-shift on ARM64, where the pattern is d = n ^{((m ShiftType amount)} -1).

x = m ShiftType amount
y = x ^{-1
z = n} y

We check canBeInternal() on x but not on y based on the computing cost analysis in that patch,
which is totally wrong. If the pattern matching is triggered, then the compiler would not emit
the corresponding Air of x after lowering, leading to data corruption or system crash since y
depends on x.

In the real world example (Zoom video meeting), we find the B3 IR:

...
Int32 b@528 = SShr(b@526, $31(b@527), Wasm: {opcode: I32ShrS, location: 0x26b})
Int32 b@529 = BitXor(b@528, $-1(b@144), Wasm: {opcode: I32Xor, location: 0x26e})
...
Int32 b@551 = BitXor(b@446, b@529, Wasm: {opcode: I32Xor, location: 0x28e})
...

After Lowering to Air:

...
Not32 %fp, %x2, b@529
...
XorNotRightShift32 %tmp199, %tmp211, $31, %tmp209, b@551
...

Since the implementation of the previous patch does commitInternal() on b@528, the operand of
b@529 turns to a frame pointer. To resolve this problem, we should either check canBeInternal()
on both b@528 and b@529 or not at all.

• b3/B3LowerToAir.cpp:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• b3/B3LowerToAir.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-08-04  Yijia Huang  <yijia_huang@apple.com>
+
+        [ARM64] Fix Zoom black screen during video meeting on Safari
+        https://bugs.webkit.org/show_bug.cgi?id=228776
+
+        Reviewed by Saam Barati.
+
+        The problem (rdar://81434487) reports that Zoom turns to a black screen during the video 
+        meeting on Safari. The reproduction of this problem is verified and bisected to the previous patch
+        (https://bugs.webkit.org/show_bug.cgi?id=228057). Previously, we introduce a pattern 
+        matching for instruction EON-with-shift on ARM64, where the pattern is d = n ^ ((m ShiftType amount) ^ -1).
+
+            x = m ShiftType amount
+            y = x ^ -1
+            z = n ^ y
+
+        We check canBeInternal() on x but not on y based on the computing cost analysis in that patch, 
+        which is totally wrong. If the pattern matching is triggered, then the compiler would not emit 
+        the corresponding Air of x after lowering, leading to data corruption or system crash since y 
+        depends on x.
+
+        In the real world example (Zoom video meeting), we find the B3 IR:
+
+            ...
+            Int32 b@528 = SShr(b@526, $31(b@527), Wasm: {opcode: I32ShrS, location: 0x26b})
+            Int32 b@529 = BitXor(b@528, $-1(b@144), Wasm: {opcode: I32Xor, location: 0x26e})
+            ...
+            Int32 b@551 = BitXor(b@446, b@529, Wasm: {opcode: I32Xor, location: 0x28e})
+            ...
+
+
+        After Lowering to Air:
+
+            ...
+            Not32 %fp, %x2, b@529
+            ...
+            XorNotRightShift32 %tmp199, %tmp211, $31, %tmp209, b@551
+            ...
+
+        Since the implementation of the previous patch does commitInternal() on b@528, the operand of 
+        b@529 turns to a frame pointer. To resolve this problem, we should either check canBeInternal() 
+        on both b@528 and b@529 or not at all.
+
+        * b3/B3LowerToAir.cpp:
+
 2021-08-04  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp

@@ -3208 +3208 @@
                         XorNotRightShift32, XorNotRightShift64, 
                         XorNotUnsignedRightShift32, XorNotUnsignedRightShift64);
-                    if (!isValidForm(opcode, Arg::Tmp, Arg::Tmp, Arg::Imm, Arg::Tmp))
+                    if (!isValidForm(opcode, Arg::Tmp, Arg::Tmp, Arg::Imm, Arg::Tmp) || !canBeInternal(right))
                         return false;
                     Value* mValue = shiftValue->child(0);
@@ -3220 +3220 @@
 
                     append(opcode, tmp(nValue), tmp(mValue), imm(amountValue), tmp(m_value));
+                    commitInternal(right);
                     commitInternal(shiftValue);
                     return true;