From e36bc165c0d6670d4e612367d5b031954f809393 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Tue, 19 Sep 2023 16:51:30 +0000 Subject: [PATCH] [vm/ffi] MSAN instrument StoreIndexedInstr x64 TEST=ffi/function_callbacks_structs_by_value_generated_test TEST=ffi/function_callbacks_structs_by_value_native_callable_generated_test TEST=ffi/regress_52399_test.dart Bug: https://github.com/dart-lang/sdk/issues/52399 Change-Id: Id16ccea5645d9b14a8f2726cb896b99266bba5a2 Cq-Include-Trybots: luci.dart.try:vm-msan-linux-release-x64-try,vm-aot-msan-linux-release-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/303360 Commit-Queue: Daco Harkes Reviewed-by: Ryan Macnak --- .../vm/compiler/assembler/assembler_x64.cc | 9 ++++ runtime/vm/compiler/assembler/assembler_x64.h | 2 + runtime/vm/compiler/backend/il_riscv.cc | 4 ++ runtime/vm/compiler/backend/il_x64.cc | 43 +++++++++++---- tests/ffi/regress_52399_test.dart | 54 +++++++++++++++++++ 5 files changed, 103 insertions(+), 9 deletions(-) create mode 100644 tests/ffi/regress_52399_test.dart diff --git a/runtime/vm/compiler/assembler/assembler_x64.cc b/runtime/vm/compiler/assembler/assembler_x64.cc index 04a35f37239..039e580a0b1 100644 --- a/runtime/vm/compiler/assembler/assembler_x64.cc +++ b/runtime/vm/compiler/assembler/assembler_x64.cc @@ -2007,6 +2007,15 @@ LeafRuntimeScope::~LeafRuntimeScope() { __ LeaveFrame(); } +void Assembler::MsanUnpoison(Register base, intptr_t length_in_bytes) { + if (base != CallingConventions::kArg1Reg) { + movq(CallingConventions::kArg1Reg, base); + } + LoadImmediate(CallingConventions::kArg2Reg, length_in_bytes); + CallCFunction( + compiler::Address(THR, kMsanUnpoisonRuntimeEntry.OffsetFromThread())); +} + #if defined(TARGET_USES_THREAD_SANITIZER) void Assembler::TsanLoadAcquire(Address addr) { LeafRuntimeScope rt(this, /*frame_size=*/0, /*preserve_registers=*/true); diff --git a/runtime/vm/compiler/assembler/assembler_x64.h b/runtime/vm/compiler/assembler/assembler_x64.h index a8e7590f474..c2d8936820f 100644 --- a/runtime/vm/compiler/assembler/assembler_x64.h +++ b/runtime/vm/compiler/assembler/assembler_x64.h @@ -1206,6 +1206,8 @@ class Assembler : public AssemblerBase { } } + void MsanUnpoison(Register base, intptr_t length_in_bytes); + #if defined(TARGET_USES_THREAD_SANITIZER) void TsanLoadAcquire(Address addr); void TsanStoreRelease(Address addr); diff --git a/runtime/vm/compiler/backend/il_riscv.cc b/runtime/vm/compiler/backend/il_riscv.cc index c7ffc269046..7f209478691 100644 --- a/runtime/vm/compiler/backend/il_riscv.cc +++ b/runtime/vm/compiler/backend/il_riscv.cc @@ -2574,6 +2574,10 @@ void StoreIndexedInstr::EmitNativeCode(FlowGraphCompiler* compiler) { default: UNREACHABLE(); } + +#if defined(USING_MEMORY_SANITIZER) + UNIMPLEMENTED(); +#endif } static void LoadValueCid(FlowGraphCompiler* compiler, diff --git a/runtime/vm/compiler/backend/il_x64.cc b/runtime/vm/compiler/backend/il_x64.cc index b81c727f5f1..a9f101c2d09 100644 --- a/runtime/vm/compiler/backend/il_x64.cc +++ b/runtime/vm/compiler/backend/il_x64.cc @@ -8,7 +8,9 @@ #include "vm/compiler/backend/il.h" +#include "platform/assert.h" #include "platform/memory_sanitizer.h" +#include "vm/class_id.h" #include "vm/compiler/assembler/assembler.h" #include "vm/compiler/backend/flow_graph.h" #include "vm/compiler/backend/flow_graph_compiler.h" @@ -17,6 +19,7 @@ #include "vm/compiler/backend/range_analysis.h" #include "vm/compiler/ffi/native_calling_convention.h" #include "vm/compiler/jit/compiler.h" +#include "vm/compiler/runtime_api.h" #include "vm/dart_entry.h" #include "vm/instructions.h" #include "vm/object_store.h" @@ -1294,18 +1297,12 @@ void FfiCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) { __ PushRegisters(kVolatileRegisterSet); // Outgoing arguments passed on the stack to the foreign function. - __ movq(CallingConventions::kArg1Reg, temp); - __ LoadImmediate(CallingConventions::kArg2Reg, stack_space); - __ CallCFunction( - compiler::Address(THR, kMsanUnpoisonRuntimeEntry.OffsetFromThread())); + __ MsanUnpoison(temp, stack_space); // Incoming Dart arguments to this trampoline are potentially used as local // handles. - __ movq(CallingConventions::kArg1Reg, is_leaf_ ? FPREG : saved_fp); - __ LoadImmediate(CallingConventions::kArg2Reg, - (kParamEndSlotFromFp + InputCount()) * kWordSize); - __ CallCFunction( - compiler::Address(THR, kMsanUnpoisonRuntimeEntry.OffsetFromThread())); + __ MsanUnpoison(is_leaf_ ? FPREG : saved_fp, + (kParamEndSlotFromFp + InputCount()) * kWordSize); // Outgoing arguments passed by register to the foreign function. __ LoadImmediate(CallingConventions::kArg1Reg, InputCount()); @@ -2209,6 +2206,34 @@ void StoreIndexedInstr::EmitNativeCode(FlowGraphCompiler* compiler) { default: UNREACHABLE(); } + +#if defined(USING_MEMORY_SANITIZER) + RegisterSet kVolatileRegisterSet(CallingConventions::kVolatileCpuRegisters, + CallingConventions::kVolatileXmmRegisters); + __ PushRegisters(kVolatileRegisterSet); + const Register base = CallingConventions::kArg1Reg; + __ leaq(base, element_address); + intptr_t length_in_bytes; + if (IsTypedDataBaseClassId(class_id_)) { + length_in_bytes = compiler::TypedDataElementSizeInBytes(class_id_); + } else { + switch (class_id_) { + case kArrayCid: + length_in_bytes = compiler::target::kWordSize; + break; + case kOneByteStringCid: + length_in_bytes = 1; + break; + case kTwoByteStringCid: + length_in_bytes = 2; + break; + default: + FATAL("Unknown cid: %" Pd, class_id_); + } + } + __ MsanUnpoison(base, length_in_bytes); + __ PopRegisters(kVolatileRegisterSet); +#endif } LocationSummary* GuardFieldClassInstr::MakeLocationSummary(Zone* zone, diff --git a/tests/ffi/regress_52399_test.dart b/tests/ffi/regress_52399_test.dart new file mode 100644 index 00000000000..832c7d506ab --- /dev/null +++ b/tests/ffi/regress_52399_test.dart @@ -0,0 +1,54 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +// SharedObjects=ffi_test_functions + +import 'dart:ffi'; + +main() { + fine(); + fine2(); + repro52399(); +} + +const length = 8; + +repro52399() { + final memory = malloc(length).cast(); + for (int i = 0; i < length; i++) { + memory[i] = 0; + } + final typedList1 = memory.asTypedList(length); + // MSAN unhappy when unoptimized, due to runtime entry. + final readVal = typedList1[0]; + print(readVal); + free(memory); +} + +fine() { + final memory = calloc(length, 1).cast(); + final typedList1 = memory.asTypedList(length); + final readVal = typedList1[0]; + print(readVal); + free(memory); +} + +fine2() { + final memory = malloc(length).cast(); + for (int i = 0; i < length; i++) { + memory[i] = 0; + } + final readVal = memory[0]; // MSAN doesn't see this one, it's force-optimized. + print(readVal); + free(memory); +} + +@Native Function(IntPtr num, IntPtr size)>(isLeaf: true) +external Pointer calloc(int num, int size); + +@Native Function(IntPtr)>(isLeaf: true) +external Pointer malloc(int size); + +@Native(isLeaf: true) +external void free(Pointer pointer);