From fb326cffc679cab8eb873b9e44795706f023cb3c Mon Sep 17 00:00:00 2001 From: Igor Murashkin Date: Thu, 23 Jul 2015 16:53:53 -0700 Subject: [PATCH] base: replace raw pointers for out-parameters with safer out Add a zero-cost type-safe abstraction for representing "out" parameters (i.e. when the calling function has to return multiple results out by-reference into the argument slots instead of using the return slot). Change-Id: I33a941e4863b6bed71d2bfa43d7f48e9b111f83f --- build/Android.gtest.mk | 1 + runtime/base/out.h | 274 +++++++++++++++++++++++++++++++ runtime/base/out_test.cc | 99 +++++++++++ runtime/interpreter/interpreter_common.h | 2 +- runtime/lambda/box_table.cc | 3 +- runtime/lambda/box_table.h | 3 +- runtime/stack.cc | 17 +- 7 files changed, 387 insertions(+), 12 deletions(-) create mode 100644 runtime/base/out.h create mode 100644 runtime/base/out_test.cc diff --git a/build/Android.gtest.mk b/build/Android.gtest.mk index 63ad9cf3f..4850e6c44 100644 --- a/build/Android.gtest.mk +++ b/build/Android.gtest.mk @@ -165,6 +165,7 @@ RUNTIME_GTEST_COMMON_SRC_FILES := \ runtime/base/hex_dump_test.cc \ runtime/base/histogram_test.cc \ runtime/base/mutex_test.cc \ + runtime/base/out_test.cc \ runtime/base/scoped_flock_test.cc \ runtime/base/stringprintf_test.cc \ runtime/base/time_utils_test.cc \ diff --git a/runtime/base/out.h b/runtime/base/out.h new file mode 100644 index 000000000..7199b631e --- /dev/null +++ b/runtime/base/out.h @@ -0,0 +1,274 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef ART_RUNTIME_BASE_OUT_H_ +#define ART_RUNTIME_BASE_OUT_H_ + +#include +#include + +#include +// A zero-overhead abstraction marker that means this value is meant to be used as an out +// parameter for functions. It mimics semantics of a pointer that the function will +// dereference and output its value into. +// +// Inspired by the 'out' language keyword in C#. +// +// Declaration example: +// int do_work(size_t args, out result); +// // returns 0 on success, sets result, otherwise error code +// +// Use-site example: +// // (1) -- out of a local variable or field +// int res; +// if (do_work(1, outof(res)) { +// cout << "success: " << res; +// } +// // (2) -- out of an iterator +// std::vector list = {1}; +// std::vector::iterator it = list.begin(); +// if (do_work(2, outof_iterator(*it)) { +// cout << "success: " << list[0]; +// } +// // (3) -- out of a pointer +// int* array = &some_other_value; +// if (do_work(3, outof_ptr(array))) { +// cout << "success: " << *array; +// } +// +// The type will also automatically decay into a C-style pointer for compatibility +// with calling legacy code that expect pointers. +// +// Declaration example: +// void write_data(int* res) { *res = 5; } +// +// Use-site example: +// int data; +// write_data(outof(res)); +// // data is now '5' +// (The other outof_* functions can be used analogously when the target is a C-style pointer). +// +// --------------- +// +// Other typical pointer operations such as addition, subtraction, etc are banned +// since there is exactly one value being output. +// +namespace art { + +// Forward declarations. See below for specific functions. +template +struct out_convertible; // Implicitly converts to out or T*. + +// Helper function that automatically infers 'T' +// +// Returns a type that is implicitly convertible to either out or T* depending +// on the call site. +// +// Example: +// int do_work(size_t args, out result); +// // returns 0 on success, sets result, otherwise error code +// +// Usage: +// int res; +// if (do_work(1, outof(res)) { +// cout << "success: " << res; +// } +template +out_convertible outof(T& param) ALWAYS_INLINE; + +// Helper function that automatically infers 'T' from a container::iterator. +// To use when the argument is already inside an iterator. +// +// Returns a type that is implicitly convertible to either out or T* depending +// on the call site. +// +// Example: +// int do_work(size_t args, out result); +// // returns 0 on success, sets result, otherwise error code +// +// Usage: +// std::vector list = {1}; +// std::vector::iterator it = list.begin(); +// if (do_work(2, outof_iterator(*it)) { +// cout << "success: " << list[0]; +// } +template +auto ALWAYS_INLINE outof_iterator(It iter) + -> out_convertible::type>; + +// Helper function that automatically infers 'T'. +// To use when the argument is already a pointer. +// +// ptr must be not-null, else a DCHECK failure will occur. +// +// Returns a type that is implicitly convertible to either out or T* depending +// on the call site. +// +// Example: +// int do_work(size_t args, out result); +// // returns 0 on success, sets result, otherwise error code +// +// Usage: +// int* array = &some_other_value; +// if (do_work(3, outof_ptr(array))) { +// cout << "success: " << *array; +// } +template +out_convertible outof_ptr(T* ptr) ALWAYS_INLINE; + +// Zero-overhead wrapper around a non-null non-const pointer meant to be used to output +// the result of parameters. There are no other extra guarantees. +// +// The most common use case is to treat this like a typical pointer argument, for example: +// +// void write_out_5(out x) { +// *x = 5; +// } +// +// The following operations are supported: +// operator* -> use like a pointer (guaranteed to be non-null) +// == and != -> compare against other pointers for (in)equality +// begin/end -> use in standard C++ algorithms as if it was an iterator +template +struct out { + // Has to be mutable lref. Otherwise how would you write something as output into it? + explicit inline out(T& param) + : param_(param) {} + + // Model a single-element iterator (or pointer) to the parameter. + inline T& operator *() { + return param_; + } + + // + // Comparison against this or other pointers. + // + template + inline bool operator==(const T2* other) const { + return std::addressof(param_) == other; + } + + template + inline bool operator==(const out& other) const { + return std::addressof(param_) == std::addressof(other.param_); + } + + // An out-parameter is never null. + inline bool operator==(std::nullptr_t) const { + return false; + } + + template + inline bool operator!=(const T2* other) const { + return std::addressof(param_) != other; + } + + template + inline bool operator!=(const out& other) const { + return std::addressof(param_) != std::addressof(other.param_); + } + + // An out-parameter is never null. + inline bool operator!=(std::nullptr_t) const { + return true; + } + + // + // Iterator interface implementation. Use with standard algorithms. + // TODO: (add items in iterator_traits if this is truly useful). + // + + inline T* begin() { + return std::addressof(param_); + } + + inline const T* begin() const { + return std::addressof(param_); + } + + inline T* end() { + return std::addressof(param_) + 1; + } + + inline const T* end() const { + return std::addressof(param_) + 1; + } + + private: + T& param_; +}; + +// +// IMPLEMENTATION DETAILS +// + +// +// This intermediate type should not be used directly by user code. +// +// It enables 'outof(x)' to be passed into functions that expect either +// an out **or** a regular C-style pointer (T*). +// +template +struct out_convertible { + explicit inline out_convertible(T& param) + : param_(param) { + } + + // Implicitly convert into an out for standard usage. + inline operator out() { + return out(param_); + } + + // Implicitly convert into a '*' for legacy usage. + inline operator T*() { + return std::addressof(param_); + } + private: + T& param_; +}; + +// Helper function that automatically infers 'T' +template +inline out_convertible outof(T& param) { + return out_convertible(param); +} + +// Helper function that automatically infers 'T'. +// To use when the argument is already inside an iterator. +template +inline auto outof_iterator(It iter) + -> out_convertible::type> { + return outof(*iter); +} + +// Helper function that automatically infers 'T'. +// To use when the argument is already a pointer. +template +inline out_convertible outof_ptr(T* ptr) { + DCHECK(ptr != nullptr); + return outof(*ptr); +} + +// Helper function that automatically infers 'T'. +// Forwards an out parameter from one function into another. +template +inline out_convertible outof_forward(out& out_param) { + T& param = std::addressof(*out_param); + return out_convertible(param); +} + +} // namespace art +#endif // ART_RUNTIME_BASE_OUT_H_ diff --git a/runtime/base/out_test.cc b/runtime/base/out_test.cc new file mode 100644 index 000000000..427420035 --- /dev/null +++ b/runtime/base/out_test.cc @@ -0,0 +1,99 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "out.h" + +#include +#include + +namespace art { + +struct OutTest : public testing::Test { + // Multiplies values less than 10 by two, stores the result and returns 0. + // Returns -1 if the original value was not multiplied by two. + static int multiply_small_values_by_two(size_t args, out result) { + if (args < 10) { + *result = args * 2; + return 0; + } else { + return -1; + } + } +}; + +extern "C" int multiply_small_values_by_two_legacy(size_t args, int* result) { + if (args < 10) { + *result = args * 2; + return 0; + } else { + return -1; + } +} + +TEST_F(OutTest, TraditionalCall) { + // For calling traditional C++ functions. + int res; + EXPECT_EQ(multiply_small_values_by_two(1, outof(res)), 0); + EXPECT_EQ(2, res); +} + +TEST_F(OutTest, LegacyCall) { + // For calling legacy, e.g. C-style functions. + int res2; + EXPECT_EQ(0, multiply_small_values_by_two_legacy(1, outof(res2))); + EXPECT_EQ(2, res2); +} + +TEST_F(OutTest, CallFromIterator) { + // For calling a function with a parameter originating as an iterator. + std::vector list = {1, 2, 3}; // NOLINT [whitespace/labels] [4] + std::vector::iterator it = list.begin(); + + EXPECT_EQ(0, multiply_small_values_by_two(2, outof_iterator(it))); + EXPECT_EQ(4, list[0]); +} + +TEST_F(OutTest, CallFromPointer) { + // For calling a function with a parameter originating as a C-pointer. + std::vector list = {1, 2, 3}; // NOLINT [whitespace/labels] [4] + + int* list_ptr = &list[2]; // 3 + + EXPECT_EQ(0, multiply_small_values_by_two(2, outof_ptr(list_ptr))); + EXPECT_EQ(4, list[2]); +} + +TEST_F(OutTest, OutAsIterator) { + // For using the out parameter as an iterator inside of the callee. + std::vector list; + int x = 100; + out out_from_x = outof(x); + + for (const int& val : out_from_x) { + list.push_back(val); + } + + ASSERT_EQ(1u, list.size()); + EXPECT_EQ(100, list[0]); + + // A more typical use-case would be to use std algorithms + EXPECT_NE(out_from_x.end(), + std::find(out_from_x.begin(), + out_from_x.end(), + 100)); // Search for '100' in out. +} + +} // namespace art diff --git a/runtime/interpreter/interpreter_common.h b/runtime/interpreter/interpreter_common.h index 2486a983f..a6cccef61 100644 --- a/runtime/interpreter/interpreter_common.h +++ b/runtime/interpreter/interpreter_common.h @@ -553,7 +553,7 @@ static inline bool DoUnboxLambda(Thread* self, ArtMethod* unboxed_closure = nullptr; // Raise an exception if unboxing fails. if (!Runtime::Current()->GetLambdaBoxTable()->UnboxLambda(boxed_closure_object, - &unboxed_closure)) { + outof(unboxed_closure))) { CHECK(self->IsExceptionPending()); return false; } diff --git a/runtime/lambda/box_table.cc b/runtime/lambda/box_table.cc index 64a6076ae..22cc820b7 100644 --- a/runtime/lambda/box_table.cc +++ b/runtime/lambda/box_table.cc @@ -94,8 +94,7 @@ mirror::Object* BoxTable::BoxLambda(const ClosureType& closure) { return method_as_object; } -bool BoxTable::UnboxLambda(mirror::Object* object, ClosureType* out_closure) { - DCHECK(object != nullptr); +bool BoxTable::UnboxLambda(mirror::Object* object, out out_closure) { *out_closure = nullptr; // Note that we do not need to access lambda_table_lock_ here diff --git a/runtime/lambda/box_table.h b/runtime/lambda/box_table.h index 312d811b9..c6d3d0c0f 100644 --- a/runtime/lambda/box_table.h +++ b/runtime/lambda/box_table.h @@ -18,6 +18,7 @@ #include "base/allocator.h" #include "base/hash_map.h" +#include "base/out.h" #include "gc_root.h" #include "base/macros.h" #include "base/mutex.h" @@ -51,7 +52,7 @@ class BoxTable FINAL { SHARED_REQUIRES(Locks::mutator_lock_) REQUIRES(!Locks::lambda_table_lock_); // Unboxes an object back into the lambda. Returns false and throws an exception on failure. - bool UnboxLambda(mirror::Object* object, ClosureType* out_closure) + bool UnboxLambda(mirror::Object* object, out out_closure) SHARED_REQUIRES(Locks::mutator_lock_); // Sweep weak references to lambda boxes. Update the addresses if the objects have been diff --git a/runtime/stack.cc b/runtime/stack.cc index b07b24428..2916eaaf5 100644 --- a/runtime/stack.cc +++ b/runtime/stack.cc @@ -19,6 +19,7 @@ #include "arch/context.h" #include "art_method-inl.h" #include "base/hex_dump.h" +#include "base/out.h" #include "entrypoints/entrypoint_utils-inl.h" #include "entrypoints/runtime_asm_entrypoints.h" #include "gc_map.h" @@ -180,7 +181,7 @@ mirror::Object* StackVisitor::GetThisObject() const { } else { uint16_t reg = code_item->registers_size_ - code_item->ins_size_; uint32_t value = 0; - bool success = GetVReg(m, reg, kReferenceVReg, &value); + bool success = GetVReg(m, reg, kReferenceVReg, outof(value)); // We currently always guarantee the `this` object is live throughout the method. CHECK(success) << "Failed to read the this object in " << PrettyMethod(m); return reinterpret_cast(value); @@ -375,8 +376,8 @@ bool StackVisitor::GetVRegPairFromQuickCode(ArtMethod* m, uint16_t vreg, VRegKin QuickMethodFrameInfo frame_info = m->GetQuickFrameInfo(code_pointer); uint32_t vmap_offset_lo, vmap_offset_hi; // TODO: IsInContext stops before spotting floating point registers. - if (vmap_table.IsInContext(vreg, kind_lo, &vmap_offset_lo) && - vmap_table.IsInContext(vreg + 1, kind_hi, &vmap_offset_hi)) { + if (vmap_table.IsInContext(vreg, kind_lo, outof(vmap_offset_lo)) && + vmap_table.IsInContext(vreg + 1, kind_hi, outof(vmap_offset_hi))) { bool is_float = (kind_lo == kDoubleLoVReg); uint32_t spill_mask = is_float ? frame_info.FpSpillMask() : frame_info.CoreSpillMask(); uint32_t reg_lo = vmap_table.ComputeRegister(spill_mask, vmap_offset_lo, kind_lo); @@ -399,8 +400,8 @@ bool StackVisitor::GetVRegPairFromOptimizedCode(ArtMethod* m, uint16_t vreg, uint64_t* val) const { uint32_t low_32bits; uint32_t high_32bits; - bool success = GetVRegFromOptimizedCode(m, vreg, kind_lo, &low_32bits); - success &= GetVRegFromOptimizedCode(m, vreg + 1, kind_hi, &high_32bits); + bool success = GetVRegFromOptimizedCode(m, vreg, kind_lo, outof(low_32bits)); + success &= GetVRegFromOptimizedCode(m, vreg + 1, kind_hi, outof(high_32bits)); if (success) { *val = (static_cast(high_32bits) << 32) | static_cast(low_32bits); } @@ -452,7 +453,7 @@ bool StackVisitor::SetVRegFromQuickCode(ArtMethod* m, uint16_t vreg, uint32_t ne QuickMethodFrameInfo frame_info = m->GetQuickFrameInfo(code_pointer); uint32_t vmap_offset; // TODO: IsInContext stops before spotting floating point registers. - if (vmap_table.IsInContext(vreg, kind, &vmap_offset)) { + if (vmap_table.IsInContext(vreg, kind, outof(vmap_offset))) { bool is_float = (kind == kFloatVReg) || (kind == kDoubleLoVReg) || (kind == kDoubleHiVReg); uint32_t spill_mask = is_float ? frame_info.FpSpillMask() : frame_info.CoreSpillMask(); uint32_t reg = vmap_table.ComputeRegister(spill_mask, vmap_offset, kind); @@ -532,8 +533,8 @@ bool StackVisitor::SetVRegPairFromQuickCode( QuickMethodFrameInfo frame_info = m->GetQuickFrameInfo(code_pointer); uint32_t vmap_offset_lo, vmap_offset_hi; // TODO: IsInContext stops before spotting floating point registers. - if (vmap_table.IsInContext(vreg, kind_lo, &vmap_offset_lo) && - vmap_table.IsInContext(vreg + 1, kind_hi, &vmap_offset_hi)) { + if (vmap_table.IsInContext(vreg, kind_lo, outof(vmap_offset_lo)) && + vmap_table.IsInContext(vreg + 1, kind_hi, outof(vmap_offset_hi))) { bool is_float = (kind_lo == kDoubleLoVReg); uint32_t spill_mask = is_float ? frame_info.FpSpillMask() : frame_info.CoreSpillMask(); uint32_t reg_lo = vmap_table.ComputeRegister(spill_mask, vmap_offset_lo, kind_lo); -- 2.11.0