Remove type punning from QV4::Value.

The union in QV4::Value is used to do type punning. In C++, this is
compiler-defined behavior. For example, Clang and GCC will try to detect
it and try to do the proper thing. However, it can play havoc with Alias
Analysis, and it is not guaranteed that some Undefined Behavior (or
Compiler depenedent behavior) might occur.

The really problematic part is the struct inside the union: depending on
the calling convention and the register size, it results in some
exciting code. For example, the AMD64 ABI specifies that a struct of two
values of INTEGER class can be passed in separate registers when doing a
function call. Now, if the AA in the compiler looses track of the fact
that the tag overlaps with the double, you might get:

    ecx := someTag
    ... conditional jumps
double_case:
    rdx := xorredDoubleValue
    callq someWhere

If the someWhere function checks for the tag first, mayhem ensues: the
double value in rdx does not overwrite the tag that is passed in ecx.

Changing the code to do reinterpret_cast<>s might also give problems
on 32bit architectures, because there is a double, whose size is not the
same as the size of the tag, which could confuse AA.

So, to fix this, the following is changed:
- only have a quint64 field in the QV4::Value, which has the added
  benefit that it's very clear for the compiler that it's a POD
- as memcpy is the only approved way to ensure bit-by-bit "conversion"
  between types (esp. FP<->non-FP types), change all conversions to use
  memcpy. Use bitops (shift/and/or) for anything else.
- only use accessor functions for non-quint64 values

As any modern compiler has memcpy as an intrinsic, the call will be
replaced with one or a few move instructions. The accessor functions
also get inlined, the bitops get optimized, so in all cases the compiler
can generate the most compact code possible.

This patch obsoletes f558bc48585c69de36151248c969a484a969ebb4 (which had
the exact aliassing problem of the double and the tag as described
above).

Change-Id: I60a39d8564be5ce6106403a56a8de90943217006
Reviewed-by: Ulf Hermann <ulf.hermann@theqtcompany.com>

1 files changed, 34 insertions, 31 deletions

diff --git a/src/qml/jsruntime/qv4arraydata.cpp b/src/qml/jsruntime/qv4arraydata.cpp
index 627aed0192..da91db6aae 100644
--- a/src/qml/jsruntime/qv4arraydata.cpp
+++ b/src/qml/jsruntime/qv4arraydata.cpp
@@ -91,6 +91,13 @@ const ArrayVTable SparseArrayData::static_vtbl =
 Q_STATIC_ASSERT(sizeof(Heap::ArrayData) == sizeof(Heap::SimpleArrayData));
 Q_STATIC_ASSERT(sizeof(Heap::ArrayData) == sizeof(Heap::SparseArrayData));
 
+static Q_ALWAYS_INLINE void storeValue(ReturnedValue *target, uint value)
+{
+    Value v = Value::fromReturnedValue(*target);
+    v.setValue(value);
+    *target = v.asReturnedValue();
+}
+
 void ArrayData::realloc(Object *o, Type newType, uint requested, bool enforceAttributes)
 {
     Scope scope(o->engine());
@@ -166,7 +173,7 @@ void ArrayData::realloc(Object *o, Type newType, uint requested, bool enforceAtt
 
     Heap::SparseArrayData *sparse = static_cast<Heap::SparseArrayData *>(newData->d());
 
-    uint *lastFree;
+    ReturnedValue *lastFree;
     if (d && d->type() == Heap::ArrayData::Sparse) {
         Heap::SparseArrayData *old = static_cast<Heap::SparseArrayData *>(d->d());
         sparse->sparse = old->sparse;
@@ -181,20 +188,20 @@ void ArrayData::realloc(Object *o, Type newType, uint requested, bool enforceAtt
                 SparseArrayNode *n = sparse->sparse->insert(i);
                 n->value = i;
             } else {
-                *lastFree = i;
-                sparse->arrayData[i].tag = Value::Empty_Type;
-                lastFree = &sparse->arrayData[i].uint_32;
+                storeValue(lastFree, i);
+                sparse->arrayData[i].setTag(Value::Empty_Type);
+                lastFree = &sparse->arrayData[i].rawValueRef();
             }
         }
     }
 
     if (toCopy < sparse->alloc) {
         for (uint i = toCopy; i < sparse->alloc; ++i) {
-            *lastFree = i;
-            sparse->arrayData[i].tag = Value::Empty_Type;
-            lastFree = &sparse->arrayData[i].uint_32;
+            storeValue(lastFree, i);
+            sparse->arrayData[i].setTag(Value::Empty_Type);
+            lastFree = &sparse->arrayData[i].rawValueRef();
         }
-        *lastFree = UINT_MAX;
+        storeValue(lastFree, UINT_MAX);
     }
     // ### Could explicitly free the old data
 }
@@ -338,13 +345,10 @@ void SparseArrayData::free(Heap::ArrayData *d, uint idx)
     Value *v = d->arrayData + idx;
     if (d->attrs && d->attrs[idx].isAccessor()) {
         // double slot, free both. Order is important, so we have a double slot for allocation again afterwards.
-        v[1].tag = Value::Empty_Type;
-        v[1].uint_32 = d->freeList;
-        v[0].tag = Value::Empty_Type;
-        v[0].uint_32 = idx + 1;
+        v[1].setTagValue(Value::Empty_Type, Value::fromReturnedValue(d->freeList).value());
+        v[0].setTagValue(Value::Empty_Type, idx + 1);
     } else {
-        v->tag = Value::Empty_Type;
-        v->uint_32 = d->freeList;
+        v->setTagValue(Value::Empty_Type, Value::fromReturnedValue(d->freeList).value());
     }
     d->freeList = idx;
     if (d->attrs)
@@ -372,33 +376,35 @@ uint SparseArrayData::allocate(Object *o, bool doubleSlot)
     Q_ASSERT(o->d()->arrayData->type == Heap::ArrayData::Sparse);
     Heap::SimpleArrayData *dd = o->d()->arrayData.cast<Heap::SimpleArrayData>();
     if (doubleSlot) {
-        uint *last = &dd->freeList;
+        ReturnedValue *last = &dd->freeList;
         while (1) {
-            if (*last == UINT_MAX) {
+            if (Value::fromReturnedValue(*last).value() == UINT_MAX) {
                 reallocate(o, dd->alloc + 2, true);
                 dd = o->d()->arrayData.cast<Heap::SimpleArrayData>();
                 last = &dd->freeList;
-                Q_ASSERT(*last != UINT_MAX);
+                Q_ASSERT(Value::fromReturnedValue(*last).value() != UINT_MAX);
             }
 
-            Q_ASSERT(dd->arrayData[*last].uint_32 != *last);
-            if (dd->arrayData[*last].uint_32 == (*last + 1)) {
+            Q_ASSERT(dd->arrayData[Value::fromReturnedValue(*last).value()].value() != Value::fromReturnedValue(*last).value());
+            if (dd->arrayData[Value::fromReturnedValue(*last).value()].value() == (Value::fromReturnedValue(*last).value() + 1)) {
                 // found two slots in a row
-                uint idx = *last;
-                *last = dd->arrayData[*last + 1].uint_32;
+                uint idx = Value::fromReturnedValue(*last).uint_32();
+                Value lastV = Value::fromReturnedValue(*last);
+                lastV.setValue(dd->arrayData[lastV.value() + 1].value());
+                *last = lastV.rawValue();
                 dd->attrs[idx] = Attr_Accessor;
                 return idx;
             }
-            last = &dd->arrayData[*last].uint_32;
+            last = &dd->arrayData[Value::fromReturnedValue(*last).value()].rawValueRef();
         }
     } else {
-        if (dd->freeList == UINT_MAX) {
+        if (Value::fromReturnedValue(dd->freeList).value() == UINT_MAX) {
             reallocate(o, dd->alloc + 1, false);
             dd = o->d()->arrayData.cast<Heap::SimpleArrayData>();
         }
-        uint idx = dd->freeList;
+        uint idx = Value::fromReturnedValue(dd->freeList).value();
         Q_ASSERT(idx != UINT_MAX);
-        dd->freeList = dd->arrayData[idx].uint_32;
+        dd->freeList = dd->arrayData[idx].uint_32();
         if (dd->attrs)
             dd->attrs[idx] = Attr_Data;
         return idx;
@@ -453,13 +459,10 @@ bool SparseArrayData::del(Object *o, uint index)
 
     if (isAccessor) {
         // free up both indices
-        dd->arrayData[pidx + 1].tag = Value::Empty_Type;
-        dd->arrayData[pidx + 1].uint_32 = dd->freeList;
-        dd->arrayData[pidx].tag = Value::Undefined_Type;
-        dd->arrayData[pidx].uint_32 = pidx + 1;
+        dd->arrayData[pidx + 1].setTagValue(Value::Empty_Type, Value::fromReturnedValue(dd->freeList).value());
+        dd->arrayData[pidx].setTagValue(Value::Undefined_Type, pidx + 1);
     } else {
-        dd->arrayData[pidx].tag = Value::Empty_Type;
-        dd->arrayData[pidx].uint_32 = dd->freeList;
+        dd->arrayData[pidx].setTagValue(Value::Empty_Type, Value::fromReturnedValue(dd->freeList).value());
     }
 
     dd->freeList = pidx;