Fix a bug that interpreter reuses an argument buffer of caller in tai…

…l call Signed-off-by: HyukWoo Park <[email protected]>
Samsung · May 30, 2023 · 7e68583 · 7e68583
1 parent 0c1a125
commit 7e68583
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 19 deletions.
diff --git a/.github/workflows/es-actions.yml b/.github/workflows/es-actions.yml
@@ -226,14 +226,14 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        tc: ['octane', 'v8 chakracore spidermonkey']
-        build_opt: ['', '-DESCARGOT_THREADING=ON', '-DESCARGOT_SMALL_CONFIG=ON -DESCARGOT_USE_CUSTOM_LOGGING=ON']
+        tc: ['new-es octane', 'v8 chakracore spidermonkey']
+        build_opt: ['', '-DESCARGOT_THREADING=ON -DESCARGOT_TCO=ON', '-DESCARGOT_SMALL_CONFIG=ON -DESCARGOT_USE_CUSTOM_LOGGING=ON']
         exclude:
           # exclude spidermonkey on threading-enabled build (TODO)
           - tc: 'v8 chakracore spidermonkey'
             build_opt: '-DESCARGOT_THREADING=ON'
           # exclue octane due to low performance incurred by SMALL_CONFIG
-          - tc: 'octane'
+          - tc: 'new-es octane'
             build_opt: '-DESCARGOT_SMALL_CONFIG=ON -DESCARGOT_USE_CUSTOM_LOGGING=ON'
     steps:
     - uses: actions/checkout@v3

diff --git a/src/interpreter/ByteCode.h b/src/interpreter/ByteCode.h
@@ -1966,7 +1966,11 @@ class Call : public ByteCode {
 #ifndef NDEBUG
     void dump()
     {
-        printf("call r%u <- r%u(r%u-r%u)", m_resultIndex, m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        if (m_argumentCount) {
+            printf("call r%u <- r%u(r%u-r%u)", m_resultIndex, m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        } else {
+            printf("call r%u <- r%u()", m_resultIndex, m_calleeIndex);
+        }
     }
 #endif
 };
@@ -1992,13 +1996,16 @@ class CallWithReceiver : public ByteCode {
 #ifndef NDEBUG
     void dump()
     {
-        printf("call r%u <- r%u,r%u(r%u-r%u)", m_resultIndex, m_receiverIndex, m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        if (m_argumentCount) {
+            printf("call r%u <- r%u.r%u(r%u-r%u)", m_resultIndex, m_receiverIndex, m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        } else {
+            printf("call r%u <- r%u.r%u()", m_resultIndex, m_receiverIndex, m_calleeIndex);
+        }
     }
 #endif
 };
 
 #if defined(ENABLE_TCO)
-// TCO
 class CallReturn : public ByteCode {
 public:
     CallReturn(const ByteCodeLOC& loc, const size_t calleeIndex, const size_t argumentsStartIndex, const size_t argumentCount)
@@ -2015,7 +2022,11 @@ class CallReturn : public ByteCode {
 #ifndef NDEBUG
     void dump()
     {
-        printf("tail call r%u(r%u-r%u)", m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        if (m_argumentCount) {
+            printf("tail call r%u(r%u-r%u)", m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        } else {
+            printf("tail call r%u()", m_calleeIndex);
+        }
     }
 #endif
 };
@@ -2036,7 +2047,11 @@ class TailRecursion : public ByteCode {
 #ifndef NDEBUG
     void dump()
     {
-        printf("tail recursion call r%u(r%u-r%u)", m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        if (m_argumentCount) {
+            printf("tail recursion call r%u(r%u-r%u)", m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        } else {
+            printf("tail recursion call r%u()", m_calleeIndex);
+        }
     }
 #endif
 };
@@ -2060,7 +2075,11 @@ class CallReturnWithReceiver : public ByteCode {
 #ifndef NDEBUG
     void dump()
     {
-        printf("tail call with receiver <- r%u,r%u(r%u-r%u)", m_receiverIndex, m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        if (m_argumentCount) {
+            printf("tail call r%u.r%u(r%u-r%u)", m_receiverIndex, m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        } else {
+            printf("tail call r%u.r%u()", m_receiverIndex, m_calleeIndex);
+        }
     }
 #endif
 };
@@ -2084,7 +2103,11 @@ class TailRecursionWithReceiver : public ByteCode {
 #ifndef NDEBUG
     void dump()
     {
-        printf("tail recursion call with receiver r%u,r%u(r%u-r%u)", m_receiverIndex, m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        if (m_argumentCount) {
+            printf("tail recursion call r%u.r%u(r%u-r%u)", m_receiverIndex, m_calleeIndex, m_argumentsStartIndex, m_argumentsStartIndex + m_argumentCount);
+        } else {
+            printf("tail recursion call r%u.r%u()", m_receiverIndex, m_calleeIndex);
+        }
     }
 #endif
 };

diff --git a/src/interpreter/ByteCodeInterpreter.cpp b/src/interpreter/ByteCodeInterpreter.cpp
@@ -1546,11 +1546,20 @@ Value Interpreter::interpret(ExecutionState* state, ByteCodeBlock* byteCodeBlock
             // fast tail recursion
             ASSERT(callee.isPointerValue() && callee.asPointerValue()->isScriptFunctionObject());
             ASSERT(callee.asPointerValue()->asScriptFunctionObject()->codeBlock() == byteCodeBlock->codeBlock());
-
-            // its safe to overwrite arguments because old arguments are no longer necessary
             ASSERT(state->m_argc == code->m_argumentCount);
-            for (size_t i = 0; i < state->m_argc; i++) {
-                state->m_argv[i] = registerFile[code->m_argumentsStartIndex + i];
+
+            if (code->m_argumentCount) {
+                // At the start of tail call, we need to allocate a buffer for arguments
+                // because recursive tail call reuses this buffer
+                if (UNLIKELY(!state->initTCO())) {
+                    Value* newArgs = ALLOCA(sizeof(Value) * code->m_argumentCount, Value, state);
+                    state->setTCOArguments(newArgs);
+                }
+
+                // its safe to overwrite arguments because old arguments are no longer necessary
+                for (size_t i = 0; i < state->m_argc; i++) {
+                    state->m_argv[i] = registerFile[code->m_argumentsStartIndex + i];
+                }
             }
 
             // set this value
@@ -1579,11 +1588,20 @@ Value Interpreter::interpret(ExecutionState* state, ByteCodeBlock* byteCodeBlock
             // fast tail recursion with receiver
             ASSERT(callee.isPointerValue() && callee.asPointerValue()->isScriptFunctionObject());
             ASSERT(callee.asPointerValue()->asScriptFunctionObject()->codeBlock() == byteCodeBlock->codeBlock());
-
-            // its safe to overwrite arguments because old arguments are no longer necessary
             ASSERT(state->m_argc == code->m_argumentCount);
-            for (size_t i = 0; i < state->m_argc; i++) {
-                state->m_argv[i] = registerFile[code->m_argumentsStartIndex + i];
+
+            if (code->m_argumentCount) {
+                // At the start of tail call, we need to allocate a buffer for arguments
+                // because recursive tail call reuses this buffer
+                if (UNLIKELY(!state->initTCO())) {
+                    Value* newArgs = ALLOCA(sizeof(Value) * code->m_argumentCount, Value, state);
+                    state->setTCOArguments(newArgs);
+                }
+
+                // its safe to overwrite arguments because old arguments are no longer necessary
+                for (size_t i = 0; i < state->m_argc; i++) {
+                    state->m_argv[i] = registerFile[code->m_argumentsStartIndex + i];
+                }
             }
 
             // set this value (receiver) // FIXME

diff --git a/src/runtime/ExecutionState.h b/src/runtime/ExecutionState.h
@@ -81,6 +81,9 @@ class ExecutionState : public gc {
         , m_onTry(false)
         , m_onCatch(false)
         , m_onFinally(false)
+#if defined(ENABLE_TCO)
+        , m_initTCO(false)
+#endif
         , m_argc(parent->argc())
         , m_argv(parent->argv())
     {
@@ -99,6 +102,9 @@ class ExecutionState : public gc {
         , m_onTry(false)
         , m_onCatch(false)
         , m_onFinally(false)
+#if defined(ENABLE_TCO)
+        , m_initTCO(false)
+#endif
         , m_argc(0)
         , m_argv(nullptr)
     {
@@ -117,6 +123,9 @@ class ExecutionState : public gc {
         , m_onTry(false)
         , m_onCatch(false)
         , m_onFinally(false)
+#if defined(ENABLE_TCO)
+        , m_initTCO(false)
+#endif
         , m_argc(argc)
         , m_argv(argv)
     {
@@ -135,6 +144,9 @@ class ExecutionState : public gc {
         , m_onTry(false)
         , m_onCatch(false)
         , m_onFinally(false)
+#if defined(ENABLE_TCO)
+        , m_initTCO(false)
+#endif
         , m_argc(argc)
         , m_argv(argv)
     {
@@ -156,6 +168,9 @@ class ExecutionState : public gc {
         , m_onTry(false)
         , m_onCatch(false)
         , m_onFinally(false)
+#if defined(ENABLE_TCO)
+        , m_initTCO(false)
+#endif
         , m_argc(argc)
         , m_argv(argv)
     {
@@ -253,6 +268,22 @@ class ExecutionState : public gc {
         return m_onFinally;
     }
 
+#if defined(ENABLE_TCO)
+    bool initTCO() const
+    {
+        return m_initTCO;
+    }
+
+    void setTCOArguments(Value* argv)
+    {
+        // allocate a new argument buffer
+        // because tail call reuses this buffer which can modify caller's register file
+        ASSERT(!m_initTCO && !!argv);
+        m_initTCO = true;
+        m_argv = argv;
+    }
+#endif
+
     bool isNativeFunctionObjectExecutionContext() const
     {
         return m_isNativeFunctionObjectExecutionContext;
@@ -330,6 +361,10 @@ class ExecutionState : public gc {
     bool m_onTry : 1;
     bool m_onCatch : 1;
     bool m_onFinally : 1;
+#if defined(ENABLE_TCO)
+    bool m_initTCO : 1;
+#endif
+
 #ifdef ESCARGOT_32
     size_t m_argc : 24;
 #else

diff --git a/test/vendortest b/test/vendortest
+35 −0		Escargot/new-es/number-toString.js
+106 −0		Escargot/new-es/tail-call.js