From c73cb64585f301c8bb3b03a0684f6baead99b7ac Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Wed, 22 Feb 2017 10:11:30 -0800 Subject: [PATCH] ART: Remove ObjPtr kPoison template parameter Move to a global constexpr, so that object pointer poisoning can be explicitly turned off for lower debug build overhead. Bug: 35644797 Test: m Test: m test-art-host Change-Id: I2412b67cbec144f2aee206fb48591abe581fd00a --- runtime/gc_root-inl.h | 2 +- runtime/gc_root.h | 4 +- runtime/handle_scope-inl.h | 9 ++- runtime/handle_scope.h | 10 ++-- runtime/mirror/object_test.cc | 99 ++++++++++++++++---------------- runtime/obj_ptr-inl.h | 22 +++---- runtime/obj_ptr.h | 46 ++++++++------- runtime/scoped_thread_state_change-inl.h | 6 +- runtime/scoped_thread_state_change.h | 6 +- runtime/thread-inl.h | 3 +- 10 files changed, 106 insertions(+), 101 deletions(-) diff --git a/runtime/gc_root-inl.h b/runtime/gc_root-inl.h index 390ed3c66..7795c661b 100644 --- a/runtime/gc_root-inl.h +++ b/runtime/gc_root-inl.h @@ -38,7 +38,7 @@ inline GcRoot::GcRoot(MirrorType* ref) : root_(mirror::CompressedReference::FromMirrorPtr(ref)) { } template -inline GcRoot::GcRoot(ObjPtr ref) +inline GcRoot::GcRoot(ObjPtr ref) : GcRoot(ref.Ptr()) { } inline std::string RootInfo::ToString() const { diff --git a/runtime/gc_root.h b/runtime/gc_root.h index 79e80f148..0894e9bee 100644 --- a/runtime/gc_root.h +++ b/runtime/gc_root.h @@ -24,7 +24,7 @@ namespace art { class ArtField; class ArtMethod; -template class ObjPtr; +template class ObjPtr; namespace mirror { class Object; @@ -215,7 +215,7 @@ class GcRoot { ALWAYS_INLINE GcRoot() {} explicit ALWAYS_INLINE GcRoot(MirrorType* ref) REQUIRES_SHARED(Locks::mutator_lock_); - explicit ALWAYS_INLINE GcRoot(ObjPtr ref) + explicit ALWAYS_INLINE GcRoot(ObjPtr ref) REQUIRES_SHARED(Locks::mutator_lock_); private: diff --git a/runtime/handle_scope-inl.h b/runtime/handle_scope-inl.h index 077f45e8f..492d4b4bd 100644 --- a/runtime/handle_scope-inl.h +++ b/runtime/handle_scope-inl.h @@ -114,9 +114,9 @@ inline MutableHandle FixedSizeHandleScope::NewHandle(T* objec return h; } -template template +template template inline MutableHandle FixedSizeHandleScope::NewHandle( - ObjPtr object) { + ObjPtr object) { return NewHandle(object.Ptr()); } @@ -191,9 +191,8 @@ MutableHandle VariableSizedHandleScope::NewHandle(T* object) { return current_scope_->NewHandle(object); } -template -inline MutableHandle VariableSizedHandleScope::NewHandle( - ObjPtr ptr) { +template +inline MutableHandle VariableSizedHandleScope::NewHandle(ObjPtr ptr) { return NewHandle(ptr.Ptr()); } diff --git a/runtime/handle_scope.h b/runtime/handle_scope.h index f6720bd3b..c43a482fa 100644 --- a/runtime/handle_scope.h +++ b/runtime/handle_scope.h @@ -30,7 +30,7 @@ namespace art { class HandleScope; -template class ObjPtr; +template class ObjPtr; class Thread; class VariableSizedHandleScope; @@ -224,8 +224,8 @@ class PACKED(4) FixedSizeHandleScope : public HandleScope { ALWAYS_INLINE HandleWrapperObjPtr NewHandleWrapper(ObjPtr* object) REQUIRES_SHARED(Locks::mutator_lock_); - template - ALWAYS_INLINE MutableHandle NewHandle(ObjPtr object) + template + ALWAYS_INLINE MutableHandle NewHandle(ObjPtr object) REQUIRES_SHARED(Locks::mutator_lock_); ALWAYS_INLINE void SetReference(size_t i, mirror::Object* object) @@ -286,8 +286,8 @@ class VariableSizedHandleScope : public BaseHandleScope { template MutableHandle NewHandle(T* object) REQUIRES_SHARED(Locks::mutator_lock_); - template - MutableHandle NewHandle(ObjPtr ptr) + template + MutableHandle NewHandle(ObjPtr ptr) REQUIRES_SHARED(Locks::mutator_lock_); // Number of references contained within this handle scope. diff --git a/runtime/mirror/object_test.cc b/runtime/mirror/object_test.cc index e761e4db7..d306f9c39 100644 --- a/runtime/mirror/object_test.cc +++ b/runtime/mirror/object_test.cc @@ -726,57 +726,60 @@ TEST_F(ObjectTest, ObjectPointer) { ScopedObjectAccess soa(Thread::Current()); jobject jclass_loader = LoadDex("XandY"); StackHandleScope<2> hs(soa.Self()); - ObjPtr null_ptr; - EXPECT_TRUE(null_ptr.IsNull()); - EXPECT_TRUE(null_ptr.IsValid()); - EXPECT_TRUE(null_ptr.Ptr() == nullptr); - EXPECT_TRUE(null_ptr == nullptr); - EXPECT_TRUE(null_ptr == null_ptr); - EXPECT_FALSE(null_ptr != null_ptr); - EXPECT_FALSE(null_ptr != nullptr); - null_ptr.AssertValid(); Handle class_loader(hs.NewHandle(soa.Decode(jclass_loader))); Handle h_X( hs.NewHandle(class_linker_->FindClass(soa.Self(), "LX;", class_loader))); - ObjPtr X(h_X.Get()); - EXPECT_TRUE(!X.IsNull()); - EXPECT_TRUE(X.IsValid()); - EXPECT_TRUE(X.Ptr() != nullptr); - EXPECT_OBJ_PTR_EQ(h_X.Get(), X); - // FindClass may cause thread suspension, it should invalidate X. - ObjPtr Y(class_linker_->FindClass(soa.Self(), "LY;", class_loader)); - EXPECT_TRUE(!Y.IsNull()); - EXPECT_TRUE(Y.IsValid()); - EXPECT_TRUE(Y.Ptr() != nullptr); - - // Should IsNull be safe to call on null ObjPtr? I'll allow it for now. - EXPECT_TRUE(!X.IsNull()); - EXPECT_TRUE(!X.IsValid()); - // Make X valid again by copying out of handle. - X.Assign(h_X.Get()); - EXPECT_TRUE(!X.IsNull()); - EXPECT_TRUE(X.IsValid()); - EXPECT_OBJ_PTR_EQ(h_X.Get(), X); - - // Allow thread suspension to invalidate Y. - soa.Self()->AllowThreadSuspension(); - EXPECT_TRUE(!Y.IsNull()); - EXPECT_TRUE(!Y.IsValid()); - - // Test unpoisoned. - ObjPtr unpoisoned; - EXPECT_TRUE(unpoisoned.IsNull()); - EXPECT_TRUE(unpoisoned.IsValid()); - EXPECT_TRUE(unpoisoned.Ptr() == nullptr); - EXPECT_TRUE(unpoisoned == nullptr); - EXPECT_TRUE(unpoisoned == unpoisoned); - EXPECT_FALSE(unpoisoned != unpoisoned); - EXPECT_FALSE(unpoisoned != nullptr); - - unpoisoned = h_X.Get(); - EXPECT_FALSE(unpoisoned.IsNull()); - EXPECT_TRUE(unpoisoned == h_X.Get()); - EXPECT_OBJ_PTR_EQ(unpoisoned, h_X.Get()); + + if (kObjPtrPoisoning) { + ObjPtr null_ptr; + EXPECT_TRUE(null_ptr.IsNull()); + EXPECT_TRUE(null_ptr.IsValid()); + EXPECT_TRUE(null_ptr.Ptr() == nullptr); + EXPECT_TRUE(null_ptr == nullptr); + EXPECT_TRUE(null_ptr == null_ptr); + EXPECT_FALSE(null_ptr != null_ptr); + EXPECT_FALSE(null_ptr != nullptr); + null_ptr.AssertValid(); + ObjPtr X(h_X.Get()); + EXPECT_TRUE(!X.IsNull()); + EXPECT_TRUE(X.IsValid()); + EXPECT_TRUE(X.Ptr() != nullptr); + EXPECT_OBJ_PTR_EQ(h_X.Get(), X); + // FindClass may cause thread suspension, it should invalidate X. + ObjPtr Y(class_linker_->FindClass(soa.Self(), "LY;", class_loader)); + EXPECT_TRUE(!Y.IsNull()); + EXPECT_TRUE(Y.IsValid()); + EXPECT_TRUE(Y.Ptr() != nullptr); + + // Should IsNull be safe to call on null ObjPtr? I'll allow it for now. + EXPECT_TRUE(!X.IsNull()); + EXPECT_TRUE(!X.IsValid()); + // Make X valid again by copying out of handle. + X.Assign(h_X.Get()); + EXPECT_TRUE(!X.IsNull()); + EXPECT_TRUE(X.IsValid()); + EXPECT_OBJ_PTR_EQ(h_X.Get(), X); + + // Allow thread suspension to invalidate Y. + soa.Self()->AllowThreadSuspension(); + EXPECT_TRUE(!Y.IsNull()); + EXPECT_TRUE(!Y.IsValid()); + } else { + // Test unpoisoned. + ObjPtr unpoisoned; + EXPECT_TRUE(unpoisoned.IsNull()); + EXPECT_TRUE(unpoisoned.IsValid()); + EXPECT_TRUE(unpoisoned.Ptr() == nullptr); + EXPECT_TRUE(unpoisoned == nullptr); + EXPECT_TRUE(unpoisoned == unpoisoned); + EXPECT_FALSE(unpoisoned != unpoisoned); + EXPECT_FALSE(unpoisoned != nullptr); + + unpoisoned = h_X.Get(); + EXPECT_FALSE(unpoisoned.IsNull()); + EXPECT_TRUE(unpoisoned == h_X.Get()); + EXPECT_OBJ_PTR_EQ(unpoisoned, h_X.Get()); + } } } // namespace mirror diff --git a/runtime/obj_ptr-inl.h b/runtime/obj_ptr-inl.h index d0be6dc98..f2921daea 100644 --- a/runtime/obj_ptr-inl.h +++ b/runtime/obj_ptr-inl.h @@ -22,27 +22,27 @@ namespace art { -template -inline bool ObjPtr::IsValid() const { - if (!kPoison || IsNull()) { +template +inline bool ObjPtr::IsValid() const { + if (!kObjPtrPoisoning || IsNull()) { return true; } return GetCookie() == TrimCookie(Thread::Current()->GetPoisonObjectCookie()); } -template -inline void ObjPtr::AssertValid() const { - if (kPoison) { +template +inline void ObjPtr::AssertValid() const { + if (kObjPtrPoisoning) { CHECK(IsValid()) << "Stale object pointer " << PtrUnchecked() << " , expected cookie " << TrimCookie(Thread::Current()->GetPoisonObjectCookie()) << " but got " << GetCookie(); } } -template -inline uintptr_t ObjPtr::Encode(MirrorType* ptr) { +template +inline uintptr_t ObjPtr::Encode(MirrorType* ptr) { uintptr_t ref = reinterpret_cast(ptr); DCHECK_ALIGNED(ref, kObjectAlignment); - if (kPoison && ref != 0) { + if (kObjPtrPoisoning && ref != 0) { DCHECK_LE(ref, 0xFFFFFFFFU); ref >>= kObjectAlignmentShift; // Put cookie in high bits. @@ -53,8 +53,8 @@ inline uintptr_t ObjPtr::Encode(MirrorType* ptr) { return ref; } -template -inline std::ostream& operator<<(std::ostream& os, ObjPtr ptr) { +template +inline std::ostream& operator<<(std::ostream& os, ObjPtr ptr) { // May be used for dumping bad pointers, do not use the checked version. return os << ptr.PtrUnchecked(); } diff --git a/runtime/obj_ptr.h b/runtime/obj_ptr.h index 2da2ae565..92cf4ebe2 100644 --- a/runtime/obj_ptr.h +++ b/runtime/obj_ptr.h @@ -26,10 +26,12 @@ namespace art { +constexpr bool kObjPtrPoisoning = kIsDebugBuild; + // Value type representing a pointer to a mirror::Object of type MirrorType // Pass kPoison as a template boolean for testing in non-debug builds. // Since the cookie is thread based, it is not safe to share an ObjPtr between threads. -template +template class ObjPtr { static constexpr size_t kCookieShift = sizeof(kHeapReferenceSize) * kBitsPerByte - kObjectAlignmentShift; @@ -60,14 +62,14 @@ class ObjPtr { template ::value>::type> - ALWAYS_INLINE ObjPtr(const ObjPtr& other) // NOLINT + ALWAYS_INLINE ObjPtr(const ObjPtr& other) // NOLINT REQUIRES_SHARED(Locks::mutator_lock_) : reference_(Encode(static_cast(other.Ptr()))) { } template ::value>::type> - ALWAYS_INLINE ObjPtr& operator=(const ObjPtr& other) + ALWAYS_INLINE ObjPtr& operator=(const ObjPtr& other) REQUIRES_SHARED(Locks::mutator_lock_) { reference_ = Encode(static_cast(other.Ptr())); return *this; @@ -130,7 +132,7 @@ class ObjPtr { // Ptr unchecked does not check that object pointer is valid. Do not use if you can avoid it. ALWAYS_INLINE MirrorType* PtrUnchecked() const { - if (kPoison) { + if (kObjPtrPoisoning) { return reinterpret_cast( static_cast(static_cast(reference_ << kObjectAlignmentShift))); } else { @@ -167,46 +169,46 @@ static_assert(std::is_trivially_copyable>::value, // Hash function for stl data structures. class HashObjPtr { public: - template - size_t operator()(const ObjPtr& ptr) const NO_THREAD_SAFETY_ANALYSIS { + template + size_t operator()(const ObjPtr& ptr) const NO_THREAD_SAFETY_ANALYSIS { return std::hash()(ptr.Ptr()); } }; -template -ALWAYS_INLINE bool operator==(const PointerType* a, const ObjPtr& b) +template +ALWAYS_INLINE bool operator==(const PointerType* a, const ObjPtr& b) REQUIRES_SHARED(Locks::mutator_lock_) { return b == a; } -template -ALWAYS_INLINE bool operator==(std::nullptr_t, const ObjPtr& b) { +template +ALWAYS_INLINE bool operator==(std::nullptr_t, const ObjPtr& b) { return b == nullptr; } -template -ALWAYS_INLINE bool operator!=(const PointerType* a, const ObjPtr& b) +template +ALWAYS_INLINE bool operator!=(const PointerType* a, const ObjPtr& b) REQUIRES_SHARED(Locks::mutator_lock_) { return b != a; } -template -ALWAYS_INLINE bool operator!=(std::nullptr_t, const ObjPtr& b) { +template +ALWAYS_INLINE bool operator!=(std::nullptr_t, const ObjPtr& b) { return b != nullptr; } -template -static inline ObjPtr MakeObjPtr(MirrorType* ptr) { - return ObjPtr(ptr); +template +static inline ObjPtr MakeObjPtr(MirrorType* ptr) { + return ObjPtr(ptr); } -template -static inline ObjPtr MakeObjPtr(ObjPtr ptr) { - return ObjPtr(ptr); +template +static inline ObjPtr MakeObjPtr(ObjPtr ptr) { + return ObjPtr(ptr); } -template -ALWAYS_INLINE std::ostream& operator<<(std::ostream& os, ObjPtr ptr); +template +ALWAYS_INLINE std::ostream& operator<<(std::ostream& os, ObjPtr ptr); } // namespace art diff --git a/runtime/scoped_thread_state_change-inl.h b/runtime/scoped_thread_state_change-inl.h index 000da59bd..c817a9ee8 100644 --- a/runtime/scoped_thread_state_change-inl.h +++ b/runtime/scoped_thread_state_change-inl.h @@ -79,11 +79,11 @@ inline T ScopedObjectAccessAlreadyRunnable::AddLocalReference(ObjPtrAddLocalReference(obj); } -template -inline ObjPtr ScopedObjectAccessAlreadyRunnable::Decode(jobject obj) const { +template +inline ObjPtr ScopedObjectAccessAlreadyRunnable::Decode(jobject obj) const { Locks::mutator_lock_->AssertSharedHeld(Self()); DCHECK(IsRunnable()); // Don't work with raw objects in non-runnable states. - return ObjPtr::DownCast(Self()->DecodeJObject(obj)); + return ObjPtr::DownCast(Self()->DecodeJObject(obj)); } inline bool ScopedObjectAccessAlreadyRunnable::IsRunnable() const { diff --git a/runtime/scoped_thread_state_change.h b/runtime/scoped_thread_state_change.h index 24199f76b..a3286ac3d 100644 --- a/runtime/scoped_thread_state_change.h +++ b/runtime/scoped_thread_state_change.h @@ -27,7 +27,7 @@ namespace art { struct JNIEnvExt; -template class ObjPtr; +template class ObjPtr; // Scoped change into and out of a particular state. Handles Runnable transitions that require // more complicated suspension checking. The subclasses ScopedObjectAccessUnchecked and @@ -91,8 +91,8 @@ class ScopedObjectAccessAlreadyRunnable : public ValueObject { T AddLocalReference(ObjPtr obj) const REQUIRES_SHARED(Locks::mutator_lock_); - template - ObjPtr Decode(jobject obj) const REQUIRES_SHARED(Locks::mutator_lock_); + template + ObjPtr Decode(jobject obj) const REQUIRES_SHARED(Locks::mutator_lock_); ALWAYS_INLINE bool IsRunnable() const; diff --git a/runtime/thread-inl.h b/runtime/thread-inl.h index 8d946262e..482e0e39a 100644 --- a/runtime/thread-inl.h +++ b/runtime/thread-inl.h @@ -29,6 +29,7 @@ #include "base/mutex-inl.h" #include "gc/heap.h" #include "jni_env_ext.h" +#include "obj_ptr.h" #include "runtime.h" #include "thread_pool.h" @@ -355,7 +356,7 @@ inline void Thread::RevokeThreadLocalAllocationStack() { } inline void Thread::PoisonObjectPointersIfDebug() { - if (kIsDebugBuild) { + if (kObjPtrPoisoning) { Thread::Current()->PoisonObjectPointers(); } } -- 2.11.0