Partial fix for retaining Inline scalar in mutable container [#223]

This is a bandaid for a nasty architectural bug.

Fleece/Core/Value.cc

@@ -309,6 +309,11 @@ namespace fleece { namespace impl {
     }
 
 
+    bool Value::isMutable() const {
+        return HeapValue::isHeapValue(this);
+    }
+
+
 #pragma mark - VALIDATION:
 
 

Fleece/Core/Value.hh

@@ -142,7 +142,7 @@ namespace fleece { namespace impl {
         alloc_slice toString() const;
 
         /** Returns true if this value is a mutable array or dict. */
-        bool isMutable() const FLPURE              {return ((size_t)this & 1) != 0;}
+        bool isMutable() const FLPURE;
 
         /** Looks up the SharedKeys from the enclosing Doc (if any.) */
         SharedKeys* sharedKeys() const noexcept FLPURE;

Fleece/Mutable/HeapValue.cc

@@ -37,7 +37,6 @@ namespace fleece { namespace impl { namespace internal {
 
 
     HeapValue::HeapValue(tags tag, int tiny) {
-        _pad = 0xFF;
         _header = uint8_t((tag << 4) | tiny);
     }
 
@@ -140,7 +139,7 @@ namespace fleece { namespace impl { namespace internal {
         if (!isHeapValue(v))
             return nullptr;
         auto ov = (offsetValue*)(size_t(v) & ~1);
-        assert_postcondition(ov->_pad == 0xFF);
+        assert_postcondition(ov->_pad == kHeapValuePad);
         return (HeapValue*)ov;
     }
 
@@ -159,10 +158,23 @@ namespace fleece { namespace impl { namespace internal {
             RetainedConst<Doc> doc = Doc::containing(v);
             if (_usuallyTrue(doc != nullptr))
                 (void)fleece::retain(std::move(doc));
-            else if (!isHardwiredValue(v))
+            else if (!isHardwiredValue(v)) {
+#ifdef __LITTLE_ENDIAN__
+                if (ValueSlot::isInlineValue(v)) {
+                    // This is an annoying limitation, currently. This Value is contained in a
+                    // ValueSlot in a mutable Array or Dict. We could effectively retain it by
+                    // retaining the container ... but there's not enough information in the slot
+                    // to locate the container. (See issue #223)
+                    // In big-endian we can't even detect this situation because inline values in
+                    // a ValueSlot are even-aligned, indistinguishably from an immutable Value.
+                    FleeceException::_throw(InvalidData,
+                                            "Can't retain scalar Value %p that's inline in a "
+                                            "mutable container [Fleece #223]", v);
+                }
+#endif
                 FleeceException::_throw(InvalidData,
-                                        "Can't retain immutable Value %p that's not part of a Doc",
-                                        v);
+                            "Can't retain immutable Value %p that's not part of a Doc", v);
+            }
         }
         return v;
     }

Fleece/Mutable/HeapValue.hh

@@ -20,8 +20,10 @@ namespace fleece { namespace impl {
     namespace internal {
         using namespace fleece::impl;
 
+        constexpr uint8_t kHeapValuePad = 0xFF;
+
         struct offsetValue {
-            uint8_t _pad = 0xFF;                // Unused byte, to ensure _header is at an odd address
+            uint8_t _pad = kHeapValuePad;       // Unused byte, to ensure _header is at an odd address
             uint8_t _header;                    // Value header byte (tag | tiny)
 //          uint8_t _data[0];                   // Extra Value data (object is dynamically sized)
 
@@ -48,10 +50,13 @@ namespace fleece { namespace impl {
             static HeapValue* createData(slice s)       {return createStr(internal::kBinaryTag, s);}
             static HeapValue* create(const Value*);
 
-            static const Value* asValue(HeapValue *v) FLPURE   {return v ? v->asValue() : nullptr;}
+            static const Value* asValue(HeapValue *v) FLPURE   {return (v ? v->asValue() : nullptr);}
             const Value* asValue() const FLPURE                {return (const Value*)&_header;}
 
-            static bool isHeapValue(const Value *v) FLPURE     {return ((size_t)v & 1) != 0;}
+            static bool isHeapValue(const Value *v) FLPURE {
+                return ((size_t)v & 1) != 0 && ((uint8_t*)v)[-1] == kHeapValuePad;
+            }
+
             static HeapValue* asHeapValue(const Value*);
 
             static const Value* retain(const Value *v);

Fleece/Mutable/ValueSlot.hh

@@ -64,6 +64,12 @@ namespace fleece { namespace impl {
         /** Replaces an external value with a copy of itself. */
         void copyValue(CopyFlags);
 
+#ifdef __LITTLE_ENDIAN__
+        static bool isInlineValue(const Value* v) {
+            return ((size_t)v & 1) != 0 && ((uint8_t*)v)[-1] == kInlineTag;
+        }
+#endif
+
     protected:
         friend class internal::HeapArray;
         friend class internal::HeapDict;
@@ -120,7 +126,8 @@ namespace fleece { namespace impl {
             };
         };
 
-        static constexpr uint8_t kInlineTag = 0xFF;
+        // Value stored in `_tag`. Must have 2 LSB set. Must not be equal to kHeapValuePad.
+        static constexpr uint8_t kInlineTag = 0x8F;
     };
 
 } }

Tests/MutableTests.cc

@@ -121,9 +121,12 @@ namespace fleece {
             MutableArray::iterator i(ma);
             for (int n = 0; n < kSize; ++n) {
                 std::cerr << "Item " << n << ": " << (void*)i.value() << "\n";
+                INFO("Item " << n << ": " << (void*)i.value());
                 CHECK(i);
                 CHECK(i.value() != nullptr);
                 CHECK(i.value()->type() == kExpectedTypes[n]);
+                bool expectMutable = (n == 8 || n == 10 || n == 12 || n >= 15);
+                CHECK(i.value()->isMutable() == expectMutable);
                 ++i;
             }
             CHECK(!i);