Commit c40b2b7e authored by Clemens Backes's avatar Clemens Backes Committed by Commit Bot

Revert "Reland "[csa] Fix semantics of PopAndReturn""

This reverts commit 3593ee83.

Reason for revert: MSan issues: https://ci.chromium.org/p/v8/builders/ci/V8%20Linux%20-%20arm64%20-%20sim%20-%20MSAN/34798

Original change's description:
> Reland "[csa] Fix semantics of PopAndReturn"
>
> This is a reland of 5e5eaf79
>
> This CL fixes the "function returns address of local variable" issue
> which GCC was complaining about by using inline assembly instead of
> address of a local for getting stack pointer approximation.
>
> Original change's description:
> > [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}
>
> Tbr: tebbi@chromium.org
> Bug: v8:5269
> Bug: v8:10201
> Change-Id: Ic1a05fcc4efd2068538bff28189545cfd2617d9b
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2465839
> Reviewed-by: Igor Sheludko <ishell@chromium.org>
> Reviewed-by: Victor Gomes <victorgomes@chromium.org>
> Commit-Queue: Igor Sheludko <ishell@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#70483}

TBR=tebbi@chromium.org,ishell@chromium.org,victorgomes@chromium.org

Change-Id: Icbd71d744a519a58e49feb917109228631b9d9a3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:5269
Bug: v8:10201
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2467846Reviewed-by: 's avatarClemens Backes <clemensb@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70485}
parent 5f6124f9
...@@ -10434,20 +10434,14 @@ TNode<TIndex> CodeStubAssembler::BuildFastLoop(const VariableList& vars, ...@@ -10434,20 +10434,14 @@ TNode<TIndex> CodeStubAssembler::BuildFastLoop(const VariableList& vars,
} }
// Instantiate BuildFastLoop for IntPtrT and UintPtrT. // Instantiate BuildFastLoop for IntPtrT and UintPtrT.
template V8_EXPORT_PRIVATE TNode<IntPtrT> template TNode<IntPtrT> CodeStubAssembler::BuildFastLoop<IntPtrT>(
CodeStubAssembler::BuildFastLoop<IntPtrT>(const VariableList& vars, const VariableList& vars, TNode<IntPtrT> start_index,
TNode<IntPtrT> start_index, TNode<IntPtrT> end_index, const FastLoopBody<IntPtrT>& body, int increment,
TNode<IntPtrT> end_index, IndexAdvanceMode advance_mode);
const FastLoopBody<IntPtrT>& body, template TNode<UintPtrT> CodeStubAssembler::BuildFastLoop<UintPtrT>(
int increment, const VariableList& vars, TNode<UintPtrT> start_index,
IndexAdvanceMode advance_mode); TNode<UintPtrT> end_index, const FastLoopBody<UintPtrT>& body,
template V8_EXPORT_PRIVATE TNode<UintPtrT> int increment, IndexAdvanceMode advance_mode);
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> template <typename TIndex>
void CodeStubAssembler::BuildFastArrayForEach( void CodeStubAssembler::BuildFastArrayForEach(
......
...@@ -632,21 +632,6 @@ void RawMachineAssembler::Return(int count, Node* vs[]) { ...@@ -632,21 +632,6 @@ void RawMachineAssembler::Return(int count, Node* vs[]) {
} }
void RawMachineAssembler::PopAndReturn(Node* pop, Node* value) { 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* values[] = {pop, value};
Node* ret = MakeNode(common()->Return(1), 2, values); Node* ret = MakeNode(common()->Return(1), 2, values);
schedule()->AddReturn(CurrentBlock(), ret); schedule()->AddReturn(CurrentBlock(), ret);
......
...@@ -13,7 +13,6 @@ ...@@ -13,7 +13,6 @@
#include "src/interpreter/bytecodes.h" #include "src/interpreter/bytecodes.h"
#include "src/objects/arguments-inl.h" #include "src/objects/arguments-inl.h"
#include "src/objects/cell-inl.h" #include "src/objects/cell-inl.h"
#include "src/objects/code-kind.h"
#include "src/objects/data-handler-inl.h" #include "src/objects/data-handler-inl.h"
#include "src/objects/debug-objects-inl.h" #include "src/objects/debug-objects-inl.h"
#include "src/objects/embedder-data-array-inl.h" #include "src/objects/embedder-data-array-inl.h"
...@@ -1296,10 +1295,7 @@ void JSFunction::JSFunctionPrint(std::ostream& os) { // NOLINT ...@@ -1296,10 +1295,7 @@ void JSFunction::JSFunctionPrint(std::ostream& os) { // NOLINT
os << "\n - kind: " << shared().kind(); os << "\n - kind: " << shared().kind();
os << "\n - context: " << Brief(context()); os << "\n - context: " << Brief(context());
os << "\n - code: " << Brief(code()); os << "\n - code: " << Brief(code());
if (code().kind() == CodeKind::DEOPT_ENTRIES_OR_FOR_TESTING) { if (ActiveTierIsIgnition()) {
os << "\n - FunctionTester function";
} else if (ActiveTierIsIgnition()) {
os << "\n - interpreted"; os << "\n - interpreted";
if (shared().HasBytecodeArray()) { if (shared().HasBytecodeArray()) {
os << "\n - bytecode: " << shared().GetBytecodeArray(); os << "\n - bytecode: " << shared().GetBytecodeArray();
......
...@@ -76,7 +76,6 @@ v8_source_set("cctest_sources") { ...@@ -76,7 +76,6 @@ v8_source_set("cctest_sources") {
"../common/wasm/flag-utils.h", "../common/wasm/flag-utils.h",
"../common/wasm/test-signatures.h", "../common/wasm/test-signatures.h",
"../common/wasm/wasm-macro-gen.h", "../common/wasm/wasm-macro-gen.h",
"cctest-utils.h",
"collector.h", "collector.h",
"compiler/c-signature.h", "compiler/c-signature.h",
"compiler/call-tester.h", "compiler/call-tester.h",
......
// Copyright 2020 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include <stdint.h>
#include "src/base/build_config.h"
#include "test/cctest/cctest.h"
namespace v8 {
namespace internal {
#ifdef V8_CC_GNU
DISABLE_ASAN inline uintptr_t GetStackPointer() {
uintptr_t sp_addr;
#if V8_HOST_ARCH_X64
__asm__ __volatile__("mov %%rsp, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_IA32
__asm__ __volatile__("mov %%esp, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_ARM
__asm__ __volatile__("str sp, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_ARM64
__asm__ __volatile__("mov x16, sp; str x16, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_MIPS
__asm__ __volatile__("sw $sp, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_MIPS64
__asm__ __volatile__("sd $sp, %0" : "=g"(sp_addr));
#elif defined(__s390x__) || defined(_ARCH_S390X)
__asm__ __volatile__("stg %%r15, %0" : "=m"(sp_addr));
#elif defined(__s390__) || defined(_ARCH_S390)
__asm__ __volatile__("st 15, %0" : "=m"(sp_addr));
#elif defined(__PPC64__) || defined(_ARCH_PPC64)
__asm__ __volatile__("std 1, %0" : "=g"(sp_addr));
#elif defined(__PPC__) || defined(_ARCH_PPC)
__asm__ __volatile__("stw 1, %0" : "=g"(sp_addr));
#else
#error Host architecture was not detected as supported by v8
#endif
return sp_addr;
}
#endif // V8_CC_GNU
} // namespace internal
} // namespace v8
...@@ -116,10 +116,6 @@ ...@@ -116,10 +116,6 @@
# https://crbug.com/v8/8919 # https://crbug.com/v8/8919
'test-platform/StackAlignment': [PASS, ['not is_clang', SKIP]], '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. # Slow tests.
'test-debug/CallFunctionInDebugger': [PASS, ['mode == debug', SLOW]], 'test-debug/CallFunctionInDebugger': [PASS, ['mode == debug', SLOW]],
......
...@@ -18,18 +18,13 @@ namespace compiler { ...@@ -18,18 +18,13 @@ namespace compiler {
class CodeAssemblerTester { class CodeAssemblerTester {
public: public:
CodeAssemblerTester(Isolate* isolate, // Test generating code for a stub. Assumes VoidDescriptor call interface.
const CallInterfaceDescriptor& descriptor, explicit CodeAssemblerTester(Isolate* isolate, const char* name = "test")
const char* name = "test")
: zone_(isolate->allocator(), ZONE_NAME, kCompressGraphZone), : zone_(isolate->allocator(), ZONE_NAME, kCompressGraphZone),
scope_(isolate), scope_(isolate),
state_(isolate, &zone_, descriptor, state_(isolate, &zone_, VoidDescriptor{},
CodeKind::DEOPT_ENTRIES_OR_FOR_TESTING, name, CodeKind::DEOPT_ENTRIES_OR_FOR_TESTING, name,
PoisoningMitigationLevel::kDontPoison, Builtins::kNoBuiltinId) {} PoisoningMitigationLevel::kDontPoison) {}
// 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). // Test generating code for a JS function (e.g. builtins).
CodeAssemblerTester(Isolate* isolate, int parameter_count, CodeAssemblerTester(Isolate* isolate, int parameter_count,
...@@ -42,7 +37,10 @@ class CodeAssemblerTester { ...@@ -42,7 +37,10 @@ class CodeAssemblerTester {
CodeAssemblerTester(Isolate* isolate, CodeKind kind, CodeAssemblerTester(Isolate* isolate, CodeKind kind,
const char* name = "test") const char* name = "test")
: CodeAssemblerTester(isolate, 0, kind, name) {} : zone_(isolate->allocator(), ZONE_NAME, kCompressGraphZone),
scope_(isolate),
state_(isolate, &zone_, 0, kind, name,
PoisoningMitigationLevel::kDontPoison) {}
CodeAssemblerTester(Isolate* isolate, CallDescriptor* call_descriptor, CodeAssemblerTester(Isolate* isolate, CallDescriptor* call_descriptor,
const char* name = "test") const char* name = "test")
......
This diff is collapsed.
...@@ -3,10 +3,8 @@ ...@@ -3,10 +3,8 @@
// found in the LICENSE file. // found in the LICENSE file.
#include <stdint.h> #include <stdint.h>
#include "src/base/build_config.h" #include "src/base/build_config.h"
#include "src/base/platform/platform.h" #include "src/base/platform/platform.h"
#include "test/cctest/cctest-utils.h"
#include "test/cctest/cctest.h" #include "test/cctest/cctest.h"
using OS = v8::base::OS; using OS = v8::base::OS;
...@@ -15,9 +13,32 @@ namespace v8 { ...@@ -15,9 +13,32 @@ namespace v8 {
namespace internal { namespace internal {
#ifdef V8_CC_GNU #ifdef V8_CC_GNU
static uintptr_t sp_addr = 0;
void GetStackPointerCallback(const v8::FunctionCallbackInfo<v8::Value>& args) { void GetStackPointer(const v8::FunctionCallbackInfo<v8::Value>& args) {
uintptr_t sp_addr = GetStackPointer(); #if V8_HOST_ARCH_X64
__asm__ __volatile__("mov %%rsp, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_IA32
__asm__ __volatile__("mov %%esp, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_ARM
__asm__ __volatile__("str sp, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_ARM64
__asm__ __volatile__("mov x16, sp; str x16, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_MIPS
__asm__ __volatile__("sw $sp, %0" : "=g"(sp_addr));
#elif V8_HOST_ARCH_MIPS64
__asm__ __volatile__("sd $sp, %0" : "=g"(sp_addr));
#elif defined(__s390x__) || defined(_ARCH_S390X)
__asm__ __volatile__("stg %%r15, %0" : "=m"(sp_addr));
#elif defined(__s390__) || defined(_ARCH_S390)
__asm__ __volatile__("st 15, %0" : "=m"(sp_addr));
#elif defined(__PPC64__) || defined(_ARCH_PPC64)
__asm__ __volatile__("std 1, %0" : "=g"(sp_addr));
#elif defined(__PPC__) || defined(_ARCH_PPC)
__asm__ __volatile__("stw 1, %0" : "=g"(sp_addr));
#else
#error Host architecture was not detected as supported by v8
#endif
args.GetReturnValue().Set(v8::Integer::NewFromUnsigned( args.GetReturnValue().Set(v8::Integer::NewFromUnsigned(
args.GetIsolate(), static_cast<uint32_t>(sp_addr))); args.GetIsolate(), static_cast<uint32_t>(sp_addr)));
...@@ -28,9 +49,8 @@ TEST(StackAlignment) { ...@@ -28,9 +49,8 @@ TEST(StackAlignment) {
v8::HandleScope handle_scope(isolate); v8::HandleScope handle_scope(isolate);
v8::Local<v8::ObjectTemplate> global_template = v8::Local<v8::ObjectTemplate> global_template =
v8::ObjectTemplate::New(isolate); v8::ObjectTemplate::New(isolate);
global_template->Set( global_template->Set(isolate, "get_stack_pointer",
isolate, "get_stack_pointer", v8::FunctionTemplate::New(isolate, GetStackPointer));
v8::FunctionTemplate::New(isolate, GetStackPointerCallback));
LocalContext env(nullptr, global_template); LocalContext env(nullptr, global_template);
CompileRun( CompileRun(
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment