From 8d50d0fd76adb570176b30177d993128a05cdfa7 Mon Sep 17 00:00:00 2001 From: Joel Dice Date: Fri, 11 Feb 2011 21:57:27 -0700 Subject: [PATCH] fix aliasing bug in util.cpp We use a template function called "cast" to get raw access to fields in in the VM. In particular, we use this function in util.cpp to treat reference fields as intptr_t fields so we can use the least significant bit as the red/black flag in red/black tree nodes. Unfortunately, this runs afoul of the type aliasing rules in C/C++, and the compiler is permitted to optimize in a way that assumes such aliasing cannot occur. Such optimization caused all the nodes in the tree to be black, leading to extremely unbalanced trees and thus slow performance. The fix in this case is to use the __may_alias__ attribute to tell the compiler we're doing something devious. I've also used this technique to avoid other potential aliasing problems. There may be others lurking, so a complete audit of the VM might be a good idea. --- src/common.h | 4 ++++ src/machine.cpp | 8 ++++---- src/machine.h | 12 ++++++------ src/util.cpp | 22 +++++++++++++++------- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/common.h b/src/common.h index 1514369d15..d4af04aa32 100644 --- a/src/common.h +++ b/src/common.h @@ -59,6 +59,8 @@ typedef uint64_t uintptr_t; # error "unsupported architecture" # endif +typedef intptr_t alias_t; + #else // not _MSC_VER # include "stdint.h" @@ -86,6 +88,8 @@ typedef uint64_t uintptr_t; # error "unsupported architecture" # endif +typedef intptr_t __attribute__((__may_alias__)) alias_t; + #endif // not _MSC_VER #undef JNIEXPORT diff --git a/src/machine.cpp b/src/machine.cpp index 92cca91a1b..4933d30f7c 100644 --- a/src/machine.cpp +++ b/src/machine.cpp @@ -2145,8 +2145,8 @@ class HeapClient: public Heap::Client { memcpy(dst, src, n * BytesPerWord); if (hashTaken(t, src)) { - cast(dst, 0) &= PointerMask; - cast(dst, 0) |= ExtendedMark; + cast(dst, 0) &= PointerMask; + cast(dst, 0) |= ExtendedMark; extendedWord(t, dst, base) = takeHash(t, src); } } @@ -2781,7 +2781,7 @@ allocate3(Thread* t, Allocator* allocator, Machine::AllocationType type, memset(o, 0, sizeInBytes); - cast(o, 0) = FixedMark; + cast(o, 0) = FixedMark; t->m->fixedFootprint += total; @@ -2796,7 +2796,7 @@ allocate3(Thread* t, Allocator* allocator, Machine::AllocationType type, memset(o, 0, sizeInBytes); - cast(o, 0) = FixedMark; + cast(o, 0) = FixedMark; return o; } diff --git a/src/machine.h b/src/machine.h index 3c152223ab..78ce6d5448 100644 --- a/src/machine.h +++ b/src/machine.h @@ -1878,8 +1878,8 @@ setObjectClass(Thread*, object o, object value) { cast(o, 0) = reinterpret_cast - (reinterpret_cast(value) - | (reinterpret_cast(cast(o, 0)) & (~PointerMask))); + (reinterpret_cast(value) + | (reinterpret_cast(cast(o, 0)) & (~PointerMask))); } inline const char* @@ -2094,19 +2094,19 @@ setType(Thread* t, Machine::Type type, object value) inline bool objectFixed(Thread*, object o) { - return (cast(o, 0) & (~PointerMask)) == FixedMark; + return (cast(o, 0) & (~PointerMask)) == FixedMark; } inline bool objectExtended(Thread*, object o) { - return (cast(o, 0) & (~PointerMask)) == ExtendedMark; + return (cast(o, 0) & (~PointerMask)) == ExtendedMark; } inline bool hashTaken(Thread*, object o) { - return (cast(o, 0) & (~PointerMask)) == HashTakenMark; + return (cast(o, 0) & (~PointerMask)) == HashTakenMark; } inline unsigned @@ -2237,7 +2237,7 @@ markHashTaken(Thread* t, object o) ACQUIRE_RAW(t, t->m->heapLock); - cast(o, 0) |= HashTakenMark; + cast(o, 0) |= HashTakenMark; t->m->heap->pad(o); } diff --git a/src/util.cpp b/src/util.cpp index 511389ade8..7ae8de1217 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -66,32 +66,32 @@ inline object getTreeNodeValue(Thread*, object n) { return reinterpret_cast - (cast(n, TreeNodeValue) & PointerMask); + (cast(n, TreeNodeValue) & PointerMask); } inline void setTreeNodeValue(Thread* t, object n, object value) { - intptr_t red = cast(n, TreeNodeValue) & (~PointerMask); + alias_t red = cast(n, TreeNodeValue) & (~PointerMask); set(t, n, TreeNodeValue, value); - cast(n, TreeNodeValue) |= red; + cast(n, TreeNodeValue) |= red; } inline bool treeNodeRed(Thread*, object n) { - return (cast(n, TreeNodeValue) & (~PointerMask)) == 1; + return (cast(n, TreeNodeValue) & (~PointerMask)) == 1; } inline void setTreeNodeRed(Thread*, object n, bool red) { if (red) { - cast(n, TreeNodeValue) |= 1; + cast(n, TreeNodeValue) |= 1; } else { - cast(n, TreeNodeValue) &= PointerMask; + cast(n, TreeNodeValue) &= PointerMask; } } @@ -108,7 +108,7 @@ cloneTreeNode(Thread* t, object n) object treeFind(Thread* t, object tree, intptr_t key, object sentinal, - intptr_t (*compare)(Thread* t, intptr_t key, object b)) + intptr_t (*compare)(Thread* t, intptr_t key, object b)) { object node = tree; while (node != sentinal) { @@ -140,6 +140,7 @@ treeFind(Thread* t, TreeContext* c, object old, intptr_t key, object node, object new_ = newRoot; PROTECT(t, new_); + int count = 0; while (old != sentinal) { c->ancestors = path(c, new_, c->ancestors); @@ -162,6 +163,13 @@ treeFind(Thread* t, TreeContext* c, object old, intptr_t key, object node, c->ancestors = c->ancestors->next; return; } + + if (++ count > 100) { + // if we've gone this deep, we probably have an unbalanced tree, + // which should only happen if there's a serious bug somewhere + // in our insertion process + abort(t); + } } setTreeNodeValue(t, new_, getTreeNodeValue(t, node));