Commit d813f56c authored by Zhi An Ng's avatar Zhi An Ng Committed by Commit Bot

Revert "[csa] Fix semantics of PopAndReturn"

This reverts commit 5e5eaf79.

Reason for revert: Failure on V8 Linux gcc https://ci.chromium.org/p/v8/builders/ci/V8%20Linux%20gcc/8929?

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,ishell@chromium.org

Change-Id: I2673982a8f51cbecf421af11b0ce5ad5031fb406
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/+/2465656Reviewed-by: 's avatarZhi An Ng <zhin@chromium.org>
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70458}
parent 3d48ae2d
......@@ -10434,20 +10434,14 @@ TNode<TIndex> CodeStubAssembler::BuildFastLoop(const VariableList& vars,
}
// Instantiate BuildFastLoop for IntPtrT and UintPtrT.
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,
template 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 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(
......
......@@ -632,21 +632,6 @@ 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);
......
......@@ -13,7 +13,6 @@
#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"
......@@ -1290,10 +1289,7 @@ void JSFunction::JSFunctionPrint(std::ostream& os) { // NOLINT
os << "\n - kind: " << shared().kind();
os << "\n - context: " << Brief(context());
os << "\n - code: " << Brief(code());
if (code().kind() == CodeKind::DEOPT_ENTRIES_OR_FOR_TESTING) {
os << "\n - FunctionTester function";
} else if (ActiveTierIsIgnition()) {
if (ActiveTierIsIgnition()) {
os << "\n - interpreted";
if (shared().HasBytecodeArray()) {
os << "\n - bytecode: " << shared().GetBytecodeArray();
......
......@@ -116,10 +116,6 @@
# 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]],
......
......@@ -18,18 +18,13 @@ namespace compiler {
class CodeAssemblerTester {
public:
CodeAssemblerTester(Isolate* isolate,
const CallInterfaceDescriptor& descriptor,
const char* name = "test")
// Test generating code for a stub. Assumes VoidDescriptor call interface.
explicit CodeAssemblerTester(Isolate* isolate, const char* name = "test")
: zone_(isolate->allocator(), ZONE_NAME, kCompressGraphZone),
scope_(isolate),
state_(isolate, &zone_, descriptor,
state_(isolate, &zone_, VoidDescriptor{},
CodeKind::DEOPT_ENTRIES_OR_FOR_TESTING, name,
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) {}
PoisoningMitigationLevel::kDontPoison) {}
// Test generating code for a JS function (e.g. builtins).
CodeAssemblerTester(Isolate* isolate, int parameter_count,
......@@ -42,7 +37,10 @@ class CodeAssemblerTester {
CodeAssemblerTester(Isolate* isolate, CodeKind kind,
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,
const char* name = "test")
......
......@@ -1930,192 +1930,63 @@ TEST(AllocateNameDictionary) {
}
}
TEST(PopAndReturnFromJSBuiltinWithStackParameters) {
TEST(PopAndReturnConstant) {
Isolate* isolate(CcTest::InitIsolateOnce());
const int kNumStackParams = 1;
CodeAssemblerTester asm_tester(isolate, kNumStackParams);
{
const int kNumParams = 4;
const int kNumProgrammaticParams = 2;
CodeAssemblerTester asm_tester(
isolate,
kNumParams - kNumProgrammaticParams + 1); // Include receiver.
CodeStubAssembler m(asm_tester.state());
m.PopAndReturn(m.SmiUntag(m.Parameter<Smi>(0)),
m.SmiConstant(Smi::FromInt(1234)));
}
// Attempt to generate code must trigger CHECK failure in RawMachineAssebler.
// PopAndReturn is not allowed in builtins with JS linkage and declared stack
// parameters.
asm_tester.GenerateCode();
}
TEST(PopAndReturnFromTFCBuiltinWithStackParameters) {
Isolate* isolate(CcTest::InitIsolateOnce());
// Setup CSA for creating TFC-style builtin with stack arguments.
// For the testing purposes we need any interface descriptor that has at
// least one argument passed on stack.
using Descriptor = FlatMapIntoArrayDescriptor;
Descriptor descriptor;
CHECK_LT(0, descriptor.GetStackParameterCount());
CodeAssemblerTester asm_tester(isolate, Descriptor());
{
CodeStubAssembler m(asm_tester.state());
m.PopAndReturn(m.SmiUntag(m.Parameter<Smi>(0)),
// Call a function that return |kNumProgramaticParams| parameters in addition
// to those specified by the static descriptor. |kNumProgramaticParams| is
// specified as a constant.
CSA_CHECK(&m, m.SmiEqual(m.Parameter<Smi>(2), m.SmiConstant(5678)));
m.PopAndReturn(m.Int32Constant(kNumProgrammaticParams),
m.SmiConstant(Smi::FromInt(1234)));
}
// Attempt to generate code must trigger CHECK failure in RawMachineAssebler.
// PopAndReturn is not allowed in builtins with JS linkage and declared stack
// parameters.
asm_tester.GenerateCode();
}
namespace {
DISABLE_ASAN void* GetStackPointer() {
// Disable ASAN, because it mallocs stack memory and therefore the result
// will not approximate stack pointer value.
intptr_t local = 0;
// Return value via p in order to avoid triggering clang warning:
// Address of stack memory associated with local variable 'local' returned
// clang(-Wreturn-stack-address).
void* p = &local;
return p;
}
TNode<Object> MakeConstantNode(CodeStubAssembler& m, Handle<Object> value) {
if (value->IsSmi()) {
return m.SmiConstant(Smi::ToInt(*value));
}
return m.HeapConstant(Handle<HeapObject>::cast(value));
}
// Buids a CSA function that calls |target| function with given arguments
// |number_of_iterations| times and checks that the stack pointer values before
// the calls and after the calls are the same.
// Then this new function is called multiple times.
template <typename... Args>
void CallFunctionWithStackPointerChecks(Isolate* isolate,
Handle<Object> expected_result,
Handle<Object> target,
Handle<Object> receiver, Args... args) {
// Setup CSA for creating TFJ-style builtin.
using Descriptor = JSTrampolineDescriptor;
CodeAssemblerTester asm_tester(isolate, Descriptor());
{
CodeStubAssembler m(asm_tester.state());
const TNode<ExternalReference> get_stack_ptr = m.ExternalConstant(
ExternalReference::Create(reinterpret_cast<Address>(GetStackPointer)));
TNode<Context> context = m.Parameter<Context>(Descriptor::kContext);
// CSA doesn't have instructions for reading current stack pointer value,
// so we use a C function that returns address of its local variable.
// This is a good-enough approximation for the stack pointer.
MachineType type_intptr = MachineType::IntPtr();
TNode<WordT> stack_pointer0 =
m.UncheckedCast<WordT>(m.CallCFunction(get_stack_ptr, type_intptr));
// CSA::CallCFunction() aligns stack pointer before the call, so off-by one
// errors will not be detected. In order to handle this we do the calls in a
// loop in order to exaggerate the effect of potentially broken stack
// pointer so that the GetStackPointer function will be able to notice it.
m.BuildFastLoop<IntPtrT>(
m.IntPtrConstant(0), m.IntPtrConstant(153),
[&](TNode<IntPtrT> index) {
TNode<Object> result = m.Call(context, MakeConstantNode(m, target),
MakeConstantNode(m, receiver),
MakeConstantNode(m, args)...);
CSA_CHECK(
&m, m.TaggedEqual(result, MakeConstantNode(m, expected_result)));
},
1, CodeStubAssembler::IndexAdvanceMode::kPost);
TNode<WordT> stack_pointer1 =
m.UncheckedCast<WordT>(m.CallCFunction(get_stack_ptr, type_intptr));
CSA_CHECK(&m, m.WordEqual(stack_pointer0, stack_pointer1));
m.Return(m.SmiConstant(42));
}
FunctionTester ft(asm_tester.GenerateCode(), 1); // Include receiver.
FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
Handle<Object> result;
for (int test_count = 0; test_count < 100; ++test_count) {
result = ft.Call().ToHandleChecked();
CHECK_EQ(Smi::FromInt(42), *result);
result = ft.Call(isolate->factory()->undefined_value(),
Handle<Smi>(Smi::FromInt(5678), isolate),
isolate->factory()->undefined_value(),
isolate->factory()->undefined_value())
.ToHandleChecked();
CHECK_EQ(1234, Handle<Smi>::cast(result)->value());
}
}
} // namespace
TEST(PopAndReturnConstant) {
Isolate* isolate(CcTest::InitIsolateOnce());
// Setup CSA for creating TFJ-style builtin.
using Descriptor = JSTrampolineDescriptor;
CodeAssemblerTester asm_tester(isolate, Descriptor());
const int kNumParams = 4; // Not including receiver
{
CodeStubAssembler m(asm_tester.state());
TNode<Int32T> argc =
m.UncheckedParameter<Int32T>(Descriptor::kActualArgumentsCount);
CSA_CHECK(&m, m.Word32Equal(argc, m.Int32Constant(kNumParams)));
m.PopAndReturn(m.IntPtrConstant(kNumParams + 1), // Include receiver.
m.SmiConstant(1234));
}
FunctionTester ft(asm_tester.GenerateCode(), 0);
ft.function->shared().DontAdaptArguments();
// Now call this function multiple time also checking that the stack pointer
// didn't change after the calls.
Handle<Object> receiver = isolate->factory()->undefined_value();
Handle<Smi> expected_result(Smi::FromInt(1234), isolate);
CallFunctionWithStackPointerChecks(isolate, expected_result, ft.function,
receiver,
// Pass kNumParams arguments.
Handle<Smi>(Smi::FromInt(1), isolate),
Handle<Smi>(Smi::FromInt(2), isolate),
Handle<Smi>(Smi::FromInt(3), isolate),
Handle<Smi>(Smi::FromInt(4), isolate));
}
TEST(PopAndReturnVariable) {
Isolate* isolate(CcTest::InitIsolateOnce());
// Setup CSA for creating TFJ-style builtin.
using Descriptor = JSTrampolineDescriptor;
CodeAssemblerTester asm_tester(isolate, Descriptor());
const int kNumParams = 4;
const int kNumProgrammaticParams = 2;
CodeAssemblerTester asm_tester(
isolate,
kNumParams - kNumProgrammaticParams + 1); // Include receiver.
CodeStubAssembler m(asm_tester.state());
// Call a function that return |kNumProgramaticParams| parameters in addition
// to those specified by the static descriptor. |kNumProgramaticParams| is
// passed in as a parameter to the function so that it can't be recognized as
// a constant.
CSA_CHECK(&m, m.SmiEqual(m.Parameter<Smi>(2), m.SmiConstant(5678)));
m.PopAndReturn(m.SmiUntag(m.Parameter<Smi>(1)),
m.SmiConstant(Smi::FromInt(1234)));
const int kNumParams = 4; // Not including receiver
{
CodeStubAssembler m(asm_tester.state());
TNode<Int32T> argc =
m.UncheckedParameter<Int32T>(Descriptor::kActualArgumentsCount);
CSA_CHECK(&m, m.Word32Equal(argc, m.Int32Constant(kNumParams)));
TNode<Int32T> argc_with_receiver = m.Int32Add(argc, m.Int32Constant(1));
m.PopAndReturn(m.ChangeInt32ToIntPtr(argc_with_receiver),
m.SmiConstant(1234));
}
FunctionTester ft(asm_tester.GenerateCode(), 0);
ft.function->shared().DontAdaptArguments();
// Now call this function multiple time also checking that the stack pointer
// didn't change after the calls.
Handle<Object> receiver = isolate->factory()->undefined_value();
Handle<Smi> expected_result(Smi::FromInt(1234), isolate);
CallFunctionWithStackPointerChecks(isolate, expected_result, ft.function,
receiver,
// Pass kNumParams arguments.
Handle<Smi>(Smi::FromInt(1), isolate),
Handle<Smi>(Smi::FromInt(2), isolate),
Handle<Smi>(Smi::FromInt(3), isolate),
Handle<Smi>(Smi::FromInt(4), isolate));
FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
Handle<Object> result;
for (int test_count = 0; test_count < 100; ++test_count) {
result = ft.Call(Handle<Smi>(Smi::FromInt(kNumProgrammaticParams), isolate),
Handle<Smi>(Smi::FromInt(5678), isolate),
isolate->factory()->undefined_value(),
isolate->factory()->undefined_value())
.ToHandleChecked();
CHECK_EQ(1234, Handle<Smi>::cast(result)->value());
}
}
TEST(OneToTwoByteStringCopy) {
......@@ -2253,67 +2124,36 @@ TEST(TwoToTwoByteStringCopy) {
TEST(Arguments) {
Isolate* isolate(CcTest::InitIsolateOnce());
// Setup CSA for creating TFJ-style builtin.
using Descriptor = JSTrampolineDescriptor;
CodeAssemblerTester asm_tester(isolate, Descriptor());
{
const int kNumParams = 3;
CodeAssemblerTester asm_tester(isolate, kNumParams + 1); // Include receiver.
CodeStubAssembler m(asm_tester.state());
TNode<Int32T> argc =
m.UncheckedParameter<Int32T>(Descriptor::kActualArgumentsCount);
CodeStubArguments arguments(&m, argc);
CSA_CHECK(&m, m.TaggedEqual(arguments.AtIndex(0), m.SmiConstant(12)));
CSA_CHECK(&m, m.TaggedEqual(arguments.AtIndex(1), m.SmiConstant(13)));
CSA_CHECK(&m, m.TaggedEqual(arguments.AtIndex(2), m.SmiConstant(14)));
CodeStubArguments arguments(&m, m.IntPtrConstant(3));
arguments.PopAndReturn(arguments.GetReceiver());
}
CSA_ASSERT(&m, m.TaggedEqual(arguments.AtIndex(0), m.SmiConstant(12)));
CSA_ASSERT(&m, m.TaggedEqual(arguments.AtIndex(1), m.SmiConstant(13)));
CSA_ASSERT(&m, m.TaggedEqual(arguments.AtIndex(2), m.SmiConstant(14)));
FunctionTester ft(asm_tester.GenerateCode(), 0);
ft.function->shared().DontAdaptArguments();
arguments.PopAndReturn(arguments.GetReceiver());
Handle<Object> result;
result = ft.Call(Handle<Smi>(Smi::FromInt(12), isolate),
FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
Handle<Object> result = ft.Call(Handle<Smi>(Smi::FromInt(12), isolate),
Handle<Smi>(Smi::FromInt(13), isolate),
Handle<Smi>(Smi::FromInt(14), isolate))
.ToHandleChecked();
// When calling with undefined object as the receiver, the CallFunction
// builtin swaps it to the global proxy object.
CHECK_EQ(*isolate->global_proxy(), *result);
result = ft.Call(Handle<Smi>(Smi::FromInt(12), isolate),
Handle<Smi>(Smi::FromInt(13), isolate),
Handle<Smi>(Smi::FromInt(14), isolate),
Handle<Smi>(Smi::FromInt(15), isolate))
.ToHandleChecked();
CHECK_EQ(*isolate->global_proxy(), *result);
result = ft.Call(Handle<Smi>(Smi::FromInt(12), isolate),
Handle<Smi>(Smi::FromInt(13), isolate),
Handle<Smi>(Smi::FromInt(14), isolate),
Handle<Smi>(Smi::FromInt(15), isolate),
Handle<Smi>(Smi::FromInt(16), isolate),
Handle<Smi>(Smi::FromInt(17), isolate),
Handle<Smi>(Smi::FromInt(18), isolate),
Handle<Smi>(Smi::FromInt(19), isolate))
.ToHandleChecked();
CHECK_EQ(*isolate->global_proxy(), *result);
}
TEST(ArgumentsForEach) {
Isolate* isolate(CcTest::InitIsolateOnce());
// Setup CSA for creating TFJ-style builtin.
using Descriptor = JSTrampolineDescriptor;
CodeAssemblerTester asm_tester(isolate, Descriptor());
{
const int kNumParams = 3;
CodeAssemblerTester asm_tester(isolate, kNumParams + 1); // Include receiver.
CodeStubAssembler m(asm_tester.state());
TNode<Int32T> argc =
m.UncheckedParameter<Int32T>(Descriptor::kActualArgumentsCount);
CodeStubArguments arguments(&m, argc);
CodeStubArguments arguments(&m, m.IntPtrConstant(3));
TVariable<Smi> sum(&m);
CodeAssemblerVariableList list({&sum}, m.zone());
......@@ -2325,35 +2165,13 @@ TEST(ArgumentsForEach) {
});
arguments.PopAndReturn(sum.value());
}
FunctionTester ft(asm_tester.GenerateCode(), 0);
ft.function->shared().DontAdaptArguments();
Handle<Object> result;
result = ft.Call(Handle<Smi>(Smi::FromInt(12), isolate),
FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
Handle<Object> result = ft.Call(Handle<Smi>(Smi::FromInt(12), isolate),
Handle<Smi>(Smi::FromInt(13), isolate),
Handle<Smi>(Smi::FromInt(14), isolate))
.ToHandleChecked();
CHECK_EQ(Smi::FromInt(12 + 13 + 14), *result);
result = ft.Call(Handle<Smi>(Smi::FromInt(12), isolate),
Handle<Smi>(Smi::FromInt(13), isolate),
Handle<Smi>(Smi::FromInt(14), isolate),
Handle<Smi>(Smi::FromInt(15), isolate))
.ToHandleChecked();
CHECK_EQ(Smi::FromInt(12 + 13 + 14 + 15), *result);
result = ft.Call(Handle<Smi>(Smi::FromInt(12), isolate),
Handle<Smi>(Smi::FromInt(13), isolate),
Handle<Smi>(Smi::FromInt(14), isolate),
Handle<Smi>(Smi::FromInt(15), isolate),
Handle<Smi>(Smi::FromInt(16), isolate),
Handle<Smi>(Smi::FromInt(17), isolate),
Handle<Smi>(Smi::FromInt(18), isolate),
Handle<Smi>(Smi::FromInt(19), isolate))
.ToHandleChecked();
CHECK_EQ(Smi::FromInt(12 + 13 + 14 + 15 + 16 + 17 + 18 + 19), *result);
}
TEST(IsDebugActive) {
......
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