MB-30044: Make TaggedPtr<T> use explicit constructors

Add bool operator==(const TaggedPtr<T>& other) const to TaggedPtr<T>, so
that we can compare two TaggedPtr templates. As currently when comparing
two, they get cast to a bool() and then compared which is incorrect.

Also refactor SingleThreadedRCPtr<T,P,D> to separate the single arg
constructor and default constructor which inits value to Pointer's
default value. Plus add reset() method that resets SingleThreadedRCPtr<>
without taking an argument and sets the value to pointers default value.

Refactor BasicLinkedList::purgeListElem() to only take ownership of the
item that is to be erased after its been removed from the LinkedList.

Change-Id: I8dfd27120bd73b774244fd5e3f6741408e23832b
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/132648
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: rdemellow

engines/ep/src/atomic.h

@@ -128,7 +128,9 @@ class RCValue {
 template <class T, class Pointer = T*, class Deleter = std::default_delete<T>>
 class SingleThreadedRCPtr {
 public:
-    explicit SingleThreadedRCPtr(Pointer init = nullptr) : value(init) {
+    SingleThreadedRCPtr() : value(Pointer()){};
+
+    explicit SingleThreadedRCPtr(Pointer init) : value(init) {
         if (init != nullptr) {
             ++value->_rc_refcount;
         }
@@ -165,7 +167,11 @@ class SingleThreadedRCPtr {
         }
     }
 
-    void reset(Pointer newValue = nullptr) {
+    void reset() {
+        swap(Pointer());
+    }
+
+    void reset(Pointer newValue) {
         if (newValue != nullptr) {
             ++newValue->_rc_refcount;
         }

engines/ep/src/dcp/consumer.cc

@@ -603,7 +603,8 @@ ENGINE_ERROR_CODE DcpConsumer::deletion(uint32_t opaque,
                 }
             } else {
                 // MB-31141: Deletes cannot have a value
-                item->replaceValue(Blob::New(0));
+                item->replaceValue(TaggedPtr<Blob>(Blob::New(0),
+                                                   TaggedPtrBase::NoTagValue));
                 item->setDataType(PROTOCOL_BINARY_RAW_BYTES);
             }
         }

engines/ep/src/item.h

@@ -559,7 +559,7 @@ class Item : public RCValue {
         } else {
             data = Blob::New(dta, nb);
         }
-        replaceValue(data);
+        replaceValue(TaggedPtr<Blob>(data, TaggedPtrBase::NoTagValue));
     }
 
     ItemMetaData metaData;

engines/ep/src/kv_bucket.cc

@@ -577,12 +577,14 @@ void KVBucket::runPreExpiryHook(VBucket& vb, Item& it) {
     auto result = engine.getServerApi()->document->pre_expiry(info);
     if (!result.empty()) {
         // A modified value was returned, use it
-        it.replaceValue(Blob::New(result.data(), result.size()));
+        it.replaceValue(TaggedPtr<Blob>(Blob::New(result.data(), result.size()),
+                                        TaggedPtrBase::NoTagValue));
         // The API states only uncompressed xattr values are returned
         it.setDataType(PROTOCOL_BINARY_DATATYPE_XATTR);
     } else {
         // Make the document empty and raw
-        it.replaceValue(Blob::New(0));
+        it.replaceValue(
+                TaggedPtr<Blob>(Blob::New(0), TaggedPtrBase::NoTagValue));
         it.setDataType(PROTOCOL_BINARY_RAW_BYTES);
     }
 }

engines/ep/src/linked_list.cc

@@ -500,10 +500,12 @@ std::ostream& operator <<(std::ostream& os, const BasicLinkedList& ll) {
 
 OrderedLL::iterator BasicLinkedList::purgeListElem(OrderedLL::iterator it,
                                                    bool isStale) {
-    StoredValue::UniquePtr purged(&*it);
+    std::unique_ptr<OrderedStoredValue> purged;
+    auto next = it;
     {
         std::lock_guard<std::mutex> lckGd(getListWriteLock());
-        it = seqList.erase(it);
+        next = seqList.erase(it);
+        purged.reset(&*it);
     }
 
     if (isStale) {
@@ -523,7 +525,7 @@ OrderedLL::iterator BasicLinkedList::purgeListElem(OrderedLL::iterator it,
         purged->getBySeqno() > highestPurgedDeletedSeqno.load()) {
         highestPurgedDeletedSeqno = purged->getBySeqno();
     }
-    return it;
+    return next;
 }
 
 std::unique_ptr<BasicLinkedList::RangeIteratorLL>

engines/ep/src/stored-value.h

@@ -575,8 +575,7 @@ class StoredValue {
     void replaceValue(std::unique_ptr<Blob> data) {
         // Maintain the tag
         auto tag = getValueTag();
-        value.reset(data.release());
-        setValueTag(tag);
+        value.reset({data.release(), tag.raw});
     }
 
     /**
@@ -1078,7 +1077,8 @@ class OrderedStoredValue : public StoredValue {
         // point to the updated version of this StoredValue. _BUT_ we do not
         // own the new SV. At destruction, we must release this ptr if
         // we are stale.
-        chain_next_or_replacement.reset(newSv);
+        chain_next_or_replacement.reset(
+                TaggedPtr<StoredValue>(newSv, TaggedPtrBase::NoTagValue));
         setStale(true);
     }
 

engines/ep/src/stored_value_factories.cc

@@ -23,38 +23,42 @@ StoredValue::UniquePtr StoredValueFactory::operator()(
         const Item& itm, StoredValue::UniquePtr next) {
     // Allocate a buffer to store the StoredValue and any trailing bytes
     // that maybe required.
-    return StoredValue::UniquePtr(
+    return StoredValue::UniquePtr(TaggedPtr<StoredValue>(
             new (::operator new(StoredValue::getRequiredStorage(itm.getKey())))
                     StoredValue(itm,
                                 std::move(next),
                                 *stats,
-                                /*isOrdered*/ false));
+                                /*isOrdered*/ false),
+            TaggedPtrBase::NoTagValue));
 }
 
 StoredValue::UniquePtr StoredValueFactory::copyStoredValue(
         const StoredValue& other, StoredValue::UniquePtr next) {
     // Allocate a buffer to store the copy of StoredValue and any
     // trailing bytes required for the key.
-    return StoredValue::UniquePtr(
+    return StoredValue::UniquePtr(TaggedPtr<StoredValue>(
             new (::operator new(other.getObjectSize()))
-                    StoredValue(other, std::move(next), *stats));
+                    StoredValue(other, std::move(next), *stats),
+            TaggedPtrBase::NoTagValue));
 }
 
 StoredValue::UniquePtr OrderedStoredValueFactory::operator()(
         const Item& itm, StoredValue::UniquePtr next) {
     // Allocate a buffer to store the OrderStoredValue and any trailing
     // bytes required for the key.
-    return StoredValue::UniquePtr(
+    return StoredValue::UniquePtr(TaggedPtr<StoredValue>(
             new (::operator new(
                     OrderedStoredValue::getRequiredStorage(itm.getKey())))
-                    OrderedStoredValue(itm, std::move(next), *stats));
+                    OrderedStoredValue(itm, std::move(next), *stats),
+            TaggedPtrBase::NoTagValue));
 }
 
 StoredValue::UniquePtr OrderedStoredValueFactory::copyStoredValue(
         const StoredValue& other, StoredValue::UniquePtr next) {
     // Allocate a buffer to store the copy ofOrderStoredValue and any
     // trailing bytes required for the key.
-    return StoredValue::UniquePtr(
+    return StoredValue::UniquePtr(TaggedPtr<StoredValue>(
             new (::operator new(other.getObjectSize()))
-                    OrderedStoredValue(other, std::move(next), *stats));
+                    OrderedStoredValue(other, std::move(next), *stats),
+            TaggedPtrBase::NoTagValue));
 }

engines/ep/src/tagged_ptr.h

@@ -21,6 +21,11 @@
 
 #if (defined(__x86_64__) || defined(_M_X64) || defined(__s390x__))
 
+class TaggedPtrBase {
+public:
+    static const uint16_t NoTagValue = 0;
+};
+
 /*
  * This class provides a tagged pointer, which means it is a pointer however
  * the top 16 bits (the tag) can be used to hold user data.  This works
@@ -36,16 +41,12 @@
  */
 
 template <typename T>
-class TaggedPtr {
+class TaggedPtr : public TaggedPtrBase {
 public:
     using element_type = T;
 
     // Need to define all methods which unique_ptr expects from a pointer type
-    TaggedPtr() : raw(0) {
-    }
-
-    TaggedPtr(T* obj) : TaggedPtr() {
-        set(obj);
+    TaggedPtr() : raw(NoTagValue) {
     }
 
     TaggedPtr(T* obj, uint16_t tag) : TaggedPtr() {
@@ -61,7 +62,15 @@ class TaggedPtr {
         return !operator!=(other);
     }
 
-    operator bool() const {
+    bool operator==(const TaggedPtr& other) const {
+        return get() == other.get();
+    }
+
+    bool operator!=(const TaggedPtr& other) const {
+        return get() != other.get();
+    }
+
+    explicit operator bool() const {
         return extractPointer(raw) != 0;
     }
 

engines/ep/src/vbucket.cc

@@ -3881,7 +3881,7 @@ std::unique_ptr<Item> VBucket::pruneXattrDocument(
         rv->setFlags(itemMeta.flags);
         rv->setExpTime(itemMeta.exptime);
         rv->setRevSeqno(itemMeta.revSeqno);
-        rv->replaceValue(newValue);
+        rv->replaceValue(TaggedPtr<Blob>(newValue, TaggedPtrBase::NoTagValue));
         rv->setDataType(PROTOCOL_BINARY_DATATYPE_XATTR);
         return rv;
     } else {

engines/ep/src/vbucket_state.cc

@@ -28,7 +28,9 @@ bool vbucket_transition_state::needsToBePersisted(
 void vbucket_transition_state::toItem(Item& item) const {
     nlohmann::json json = *this;
     std::string jsonState = json.dump();
-    item.replaceValue(Blob::New(jsonState.data(), jsonState.size()));
+    item.replaceValue(
+            TaggedPtr<Blob>(Blob::New(jsonState.data(), jsonState.size()),
+                            TaggedPtrBase::NoTagValue));
 }
 
 void vbucket_transition_state::fromItem(const Item& item) {