[csa] Fix semantics of PopAndReturn

This CL prohibits using PopAndReturn from the builtins that
have calling convention with arguments on the stack.

This CL also updates the PopAndReturn tests so that even off-by-one
errors in the number of poped arguments are caught which was not the
case before.

Motivation:

PopAndReturn is supposed to be using ONLY in CSA/Torque builtins for
dropping ALL JS arguments that are currently located on the stack.
Disallowing PopAndReturn in builtins with stack arguments simplifies
semantics of this instruction because in case of presence of declared
stack parameters it's impossible to distinguish the following cases:
1) stack parameter is included in JS arguments (and therefore it will
   be dropped as a part of 'pop' number of arguments),
2) stack parameter is NOT included in JS arguments (and therefore it
   should be dropped in ADDITION to the 'pop' number of arguments).

This issue wasn't noticed before because builtins with stack parameters
relied on adapter frames machinery to ensure that the expected
parameters are present on the stack, but on the same time the adapter
frame tearing down code was effectively recovering the stack pointer
potentially broken by the CSA builtin.

Once we get rid of the arguments adapter frames keeping stack pointer
in a valid state becomes crucial.

Bug: v8:5269, v8:10201
Change-Id: Id3ea9730bb0d41d17999c73136c4dfada374a822
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2460819
Commit-Queue: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70454}

src/codegen/code-stub-assembler.cc

@@ -10434,14 +10434,20 @@ TNode<TIndex> CodeStubAssembler::BuildFastLoop(const VariableList& vars,
 }
 
 // Instantiate BuildFastLoop for IntPtrT and UintPtrT.
-template TNode<IntPtrT> CodeStubAssembler::BuildFastLoop<IntPtrT>(
-    const VariableList& vars, TNode<IntPtrT> start_index,
-    TNode<IntPtrT> end_index, const FastLoopBody<IntPtrT>& body, int increment,
+template V8_EXPORT_PRIVATE TNode<IntPtrT>
+CodeStubAssembler::BuildFastLoop<IntPtrT>(const VariableList& vars,
+                                          TNode<IntPtrT> start_index,
+                                          TNode<IntPtrT> end_index,
+                                          const FastLoopBody<IntPtrT>& body,
+                                          int increment,
+                                          IndexAdvanceMode advance_mode);
+template V8_EXPORT_PRIVATE TNode<UintPtrT>
+CodeStubAssembler::BuildFastLoop<UintPtrT>(const VariableList& vars,
+                                           TNode<UintPtrT> start_index,
+                                           TNode<UintPtrT> end_index,
+                                           const FastLoopBody<UintPtrT>& body,
+                                           int increment,
                                            IndexAdvanceMode advance_mode);
-template TNode<UintPtrT> CodeStubAssembler::BuildFastLoop<UintPtrT>(
-    const VariableList& vars, TNode<UintPtrT> start_index,
-    TNode<UintPtrT> end_index, const FastLoopBody<UintPtrT>& body,
-    int increment, IndexAdvanceMode advance_mode);
 
 template <typename TIndex>
 void CodeStubAssembler::BuildFastArrayForEach(

src/compiler/raw-machine-assembler.cc

@@ -632,6 +632,21 @@ void RawMachineAssembler::Return(int count, Node* vs[]) {
 }
 
 void RawMachineAssembler::PopAndReturn(Node* pop, Node* value) {
+  // PopAndReturn is supposed to be using ONLY in CSA/Torque builtins for
+  // dropping ALL JS arguments that are currently located on the stack.
+  // The check below ensures that there are no directly accessible stack
+  // parameters from current builtin, which implies that the builtin with
+  // JS calling convention (TFJ) was created with kDontAdaptArgumentsSentinel.
+  // This simplifies semantics of this instruction because in case of presence
+  // of directly accessible stack parameters it's impossible to distinguish
+  // the following cases:
+  // 1) stack parameter is included in JS arguments (and therefore it will be
+  //    dropped as a part of 'pop' number of arguments),
+  // 2) stack parameter is NOT included in JS arguments (and therefore it should
+  //    be dropped in ADDITION to the 'pop' number of arguments).
+  // Additionally, in order to simplify assembly code, PopAndReturn is also
+  // not allowed in builtins with stub linkage and parameters on stack.
+  CHECK_EQ(call_descriptor()->StackParameterCount(), 0);
   Node* values[] = {pop, value};
   Node* ret = MakeNode(common()->Return(1), 2, values);
   schedule()->AddReturn(CurrentBlock(), ret);

src/diagnostics/objects-printer.cc

@@ -13,6 +13,7 @@
 #include "src/interpreter/bytecodes.h"
 #include "src/objects/arguments-inl.h"
 #include "src/objects/cell-inl.h"
+#include "src/objects/code-kind.h"
 #include "src/objects/data-handler-inl.h"
 #include "src/objects/debug-objects-inl.h"
 #include "src/objects/embedder-data-array-inl.h"
@@ -1289,7 +1290,10 @@ void JSFunction::JSFunctionPrint(std::ostream& os) {  // NOLINT
   os << "\n - kind: " << shared().kind();
   os << "\n - context: " << Brief(context());
   os << "\n - code: " << Brief(code());
-  if (ActiveTierIsIgnition()) {
+  if (code().kind() == CodeKind::DEOPT_ENTRIES_OR_FOR_TESTING) {
+    os << "\n - FunctionTester function";
+
+  } else if (ActiveTierIsIgnition()) {
     os << "\n - interpreted";
     if (shared().HasBytecodeArray()) {
       os << "\n - bytecode: " << shared().GetBytecodeArray();

test/cctest/cctest.status

@@ -116,6 +116,10 @@
   # https://crbug.com/v8/8919
   'test-platform/StackAlignment': [PASS, ['not is_clang', SKIP]],
 
+  # Test that misuse of PopAndReturn does not compile.
+  'test-code-stub-assembler/PopAndReturnFromJSBuiltinWithStackParameters' : [FAIL],
+  'test-code-stub-assembler/PopAndReturnFromTFCBuiltinWithStackParameters' : [FAIL],
+
   ############################################################################
   # Slow tests.
   'test-debug/CallFunctionInDebugger': [PASS, ['mode == debug', SLOW]],

test/cctest/compiler/code-assembler-tester.h

@@ -18,13 +18,18 @@ namespace compiler {
 
 class CodeAssemblerTester {
  public:
-  // Test generating code for a stub. Assumes VoidDescriptor call interface.
-  explicit CodeAssemblerTester(Isolate* isolate, const char* name = "test")
+  CodeAssemblerTester(Isolate* isolate,
+                      const CallInterfaceDescriptor& descriptor,
+                      const char* name = "test")
       : zone_(isolate->allocator(), ZONE_NAME, kCompressGraphZone),
         scope_(isolate),
-        state_(isolate, &zone_, VoidDescriptor{},
+        state_(isolate, &zone_, descriptor,
                CodeKind::DEOPT_ENTRIES_OR_FOR_TESTING, name,
-               PoisoningMitigationLevel::kDontPoison) {}
+               PoisoningMitigationLevel::kDontPoison, Builtins::kNoBuiltinId) {}
+
+  // Test generating code for a stub. Assumes VoidDescriptor call interface.
+  explicit CodeAssemblerTester(Isolate* isolate, const char* name = "test")
+      : CodeAssemblerTester(isolate, VoidDescriptor{}, name) {}
 
   // Test generating code for a JS function (e.g. builtins).
   CodeAssemblerTester(Isolate* isolate, int parameter_count,
@@ -37,10 +42,7 @@ class CodeAssemblerTester {
 
   CodeAssemblerTester(Isolate* isolate, CodeKind kind,
                       const char* name = "test")
-      : zone_(isolate->allocator(), ZONE_NAME, kCompressGraphZone),
-        scope_(isolate),
-        state_(isolate, &zone_, 0, kind, name,
-               PoisoningMitigationLevel::kDontPoison) {}
+      : CodeAssemblerTester(isolate, 0, kind, name) {}
 
   CodeAssemblerTester(Isolate* isolate, CallDescriptor* call_descriptor,
                       const char* name = "test")