From 39d60e603f783244f5ca6e54ee0bd900989cf13c Mon Sep 17 00:00:00 2001
From: Tzvetan Mikov <tmikov@meta.com>
Date: Fri, 15 Dec 2023 10:23:30 -0800
Subject: [PATCH] Fix bigint side effects for >>>, /, %

Summary:
BigInt differs from numbers - several bigint operations can throw an
exception, specifically:
- >>> is not allowed at all and throws
- / and % can result in division by zero

Improve the side effect model and add a test.

Reported in https://github.com/facebook/hermes/issues/1212

Reviewed By: avp

Differential Revision: D52013220

fbshipit-source-id: 34606cb7d53a3db3ea49dd026661b7fd340583e5
---
 lib/IR/Instrs.cpp                  | 21 ++++++++++++++++++---
 test/shermes/regress-bigint-dce.js | 29 +++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/lib/IR/Instrs.cpp b/lib/IR/Instrs.cpp
index 25adea414f9..42ddf25fb37 100644
--- a/lib/IR/Instrs.cpp
+++ b/lib/IR/Instrs.cpp
@@ -26,6 +26,7 @@ unsigned TerminatorInst::getNumSuccessors() const {
     return I->getNumSuccessorsImpl();
 #define BEGIN_TERMINATOR(NAME, PARENT) TERMINATOR(NAME, PARENT)
 #include "hermes/IR/Instrs.def"
+
   llvm_unreachable("not a terminator?!");
 }
 
@@ -36,6 +37,7 @@ BasicBlock *TerminatorInst::getSuccessor(unsigned idx) const {
     return I->getSuccessorImpl(idx);
 #define BEGIN_TERMINATOR(NAME, PARENT) TERMINATOR(NAME, PARENT)
 #include "hermes/IR/Instrs.def"
+
   llvm_unreachable("not a terminator?!");
 }
 
@@ -46,6 +48,7 @@ void TerminatorInst::setSuccessor(unsigned idx, BasicBlock *B) {
     return I->setSuccessorImpl(idx, B);
 #define BEGIN_TERMINATOR(NAME, PARENT) TERMINATOR(NAME, PARENT)
 #include "hermes/IR/Instrs.def"
+
   llvm_unreachable("not a terminator?!");
 }
 
@@ -134,6 +137,21 @@ SideEffect BinaryOperatorInst::getBinarySideEffect(
         return SideEffect{}.setIdempotent();
       break;
 
+    case ValueKind::BinaryUnsignedRightShiftInstKind:
+    case ValueKind::BinaryDivideInstKind:
+    case ValueKind::BinaryModuloInstKind:
+      // We can only reason about primitive types.
+      if (!leftTy.isPrimitive() || !rightTy.isPrimitive())
+        break;
+      // if either of the operands can be a BigInt, this instruction may throw
+      // for one of the following reasons:
+      // - BigInt doesn't have unsigned right shift.
+      // - BigInt division by zero.
+      if (leftTy.canBeBigInt() || rightTy.canBeBigInt())
+        return SideEffect{}.setThrow();
+      // We have primitive operands that are not BigInt.
+      return SideEffect{}.setIdempotent();
+
     case ValueKind::BinaryAddInstKind:
       // We can only reason about primitive types.
       if (!leftTy.isPrimitive() || !rightTy.isPrimitive())
@@ -145,11 +163,8 @@ SideEffect BinaryOperatorInst::getBinarySideEffect(
 
     case ValueKind::BinaryLeftShiftInstKind:
     case ValueKind::BinaryRightShiftInstKind:
-    case ValueKind::BinaryUnsignedRightShiftInstKind:
     case ValueKind::BinarySubtractInstKind:
     case ValueKind::BinaryMultiplyInstKind:
-    case ValueKind::BinaryDivideInstKind:
-    case ValueKind::BinaryModuloInstKind:
     case ValueKind::BinaryOrInstKind:
     case ValueKind::BinaryXorInstKind:
     case ValueKind::BinaryAndInstKind:
diff --git a/test/shermes/regress-bigint-dce.js b/test/shermes/regress-bigint-dce.js
index 39689bd42b3..ed7e30b1073 100644
--- a/test/shermes/regress-bigint-dce.js
+++ b/test/shermes/regress-bigint-dce.js
@@ -8,11 +8,12 @@
 // RUN: %shermes -exec -O0 %s | %FileCheck --match-full-lines %s
 // RUN: %shermes -exec -O %s | %FileCheck --match-full-lines %s
 
+(function() {
+
 // Verify that arithmetic operations with a BigInt operand and non-BigInt
 // operand (except adds) are not DCE-d, because that would throw a runtime
 // exception.
 
-(function() {
 try {
     1n + 1;
 } catch (e) {
@@ -25,5 +26,29 @@ try {
 } catch (e) {
     print(e);
 }
-})();
 //CHECK-NEXT: TypeError: Cannot convert BigInt to number
+
+// Verify that BigInt operations that may throw are not eliminated:
+// unsigned right shift, division, modulo.
+try {
+    1n >>> 1n;
+} catch (e) {
+    print(e);
+}
+//CHECK-NEXT: TypeError: BigInts have no unsigned shift
+
+try {
+    1n / 0n;
+} catch (e) {
+    print(e);
+}
+//CHECK-NEXT: RangeError: Division by zero
+
+try {
+    1n % 0n;
+} catch (e) {
+    print(e);
+}
+//CHECK-NEXT: RangeError: Division by zero
+
+})();