[webkit-reviews] review denied: [Bug 19775] Alignment problems on Sparc : [Attachment 22185] patches applied on openbsd
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jul 9 11:38:24 PDT 2008
David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Landry Breuil
<landry at fr.homeunix.org>'s request for review:
Bug 19775: Alignment problems on Sparc
https://bugs.webkit.org/show_bug.cgi?id=19775
Attachment 22185: patches applied on openbsd
https://bugs.webkit.org/attachment.cgi?id=22185&action=edit
------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
Thanks for submitting this patch for review, Landry!
In order for this patch to be integrated upstream into WebKit, you will need to
define a new PLATFORM(OPENBSD) macro in JavaScriptCore/wtf/Platform.h and use
that instead of removing/changing existing code.
I would also suggest filing a separate bug for the OpenBSD fixes that aren't
covered by this bug, although that's up to you.
You may also want to consider setting up an OpenBSD buildbot that could be
added to <http://build.webkit.org/> to make sure the build doesn't break on
OpenBSD. Please chat with "bdash" on the #webkit IRC channel on
irc.freenode.net if you'd like more information about this.
>--- JavaScriptCore/kjs/JSImmediate.h.orig Sat Jun 28 15:28:13 2008
>+++ JavaScriptCore/kjs/JSImmediate.h Sat Jun 28 15:38:46 2008
>@@ -31,6 +31,22 @@
> #include <stdint.h>
> #include <stdlib.h>
>
>+typedef union {
>+ double value;
>+ int64_t bits;
>+} ieee_double_union;
>+
>+static __inline int signbit(double x)
>+{
>+ ieee_double_union mask;
>+ mask.value = -0.0;
>+ int64_t sign_bit = mask.bits;
>+
>+ ieee_double_union number;
>+ number.value = x;
>+ return (number.bits & sign_bit) != 0;
>+}
>+
> namespace KJS {
>
> class ExecState;
Probably needs #if PLATFORM(OPENBSD)/#endif around this addition.
>--- JavaScriptCore/kjs/collector.cpp.orig Mon Jun 16 01:40:06 2008
>+++ JavaScriptCore/kjs/collector.cpp Sun Jul 6 22:54:31 2008
>@@ -33,9 +33,7 @@
> #include <wtf/HashCountedSet.h>
> #include <wtf/UnusedParam.h>
>
>-#if USE(MULTIPLE_THREADS)
> #include <pthread.h>
>-#endif
>
> #if PLATFORM(DARWIN)
>
>@@ -61,11 +59,7 @@
> #include <thread.h>
> #endif
>
>-#if HAVE(PTHREAD_NP_H)
> #include <pthread_np.h>
>-#else
>-#include <pthread.h>
>-#endif
>
> #endif
>
Instead of removing code here, you probably should define USE(MULTIPLE_THREADS)
and HAVE(PTHREAD_NP_H) for PLATFORM(OPENBSD) in Platform.h.
>@@ -332,27 +326,10 @@ static inline void* currentThreadStackBase()
> thr_stksegment(&s);
> return s.ss_sp;
> #elif PLATFORM(UNIX)
>- static void* stackBase = 0;
>- static size_t stackSize = 0;
>- static pthread_t stackThread;
> pthread_t thread = pthread_self();
>- if (stackBase == 0 || thread != stackThread) {
>- pthread_attr_t sattr;
>- pthread_attr_init(&sattr);
>-#if HAVE(PTHREAD_NP_H)
>- // e.g. on FreeBSD 5.4, neundorf at kde.org
>- pthread_attr_get_np(thread, &sattr);
>-#else
>- // FIXME: this function is non-portable; other POSIX systems may have
different np alternatives
>- pthread_getattr_np(thread, &sattr);
>-#endif
>- int rc = pthread_attr_getstack(&sattr, &stackBase, &stackSize);
>- (void)rc; // FIXME: Deal with error code somehow? Seems fatal.
>- ASSERT(stackBase);
>- pthread_attr_destroy(&sattr);
>- stackThread = thread;
>- }
>- return static_cast<char*>(stackBase) + stackSize;
>+ stack_t stack;
>+ pthread_stackseg_np(thread, &stack);
>+ return stack.ss_sp;
> #else
> #error Need a way to get the stack base on this platform
> #endif
The OpenBSD code should be in its own #elif PLATFORM(OPENBSD) section before
#elif PLATFORM(UNIX) rather than removing this code.
>--- JavaScriptCore/kjs/date_object.cpp.orig Mon Jun 16 01:40:06 2008
>+++ JavaScriptCore/kjs/date_object.cpp Sun Jul 6 19:00:35 2008
>@@ -63,6 +63,8 @@
> #include <CoreFoundation/CoreFoundation.h>
> #endif
>
>+#define isfinite(x) finite(x)
>+
> using namespace WTF;
>
> namespace KJS {
Needs #if PLATFORM(OPENBSD)/#endif wrapper. (Or you may want to consider add
this to a header file; perhaps config.h.)
>--- JavaScriptCore/wtf/FastMalloc.cpp.orig Tue Jul 8 23:21:56 2008
>+++ JavaScriptCore/wtf/FastMalloc.cpp Tue Jul 8 23:22:46 2008
>@@ -1824,13 +1824,13 @@ static TCMalloc_Central_FreeListPadded central_cache[k
>
> // Page-level allocator
> static SpinLock pageheap_lock = SPINLOCK_INITIALIZER;
>-static void* pageheap_memory[(sizeof(TCMalloc_PageHeap) + sizeof(void*) - 1)
/ sizeof(void*)];
>+static uint64_t pageheap_memory[(sizeof(TCMalloc_PageHeap) + sizeof(uint64_t)
- 1) / sizeof(uint64_t)];
> static bool phinited = false;
>
> // Avoid extra level of indirection by making "pageheap" be just an alias
> // of pageheap_memory.
> typedef union {
>- void* m_memory;
>+ uint64_t* m_memory;
> TCMalloc_PageHeap* m_pageHeap;
> } PageHeapUnion;
>
>$OpenBSD$
>--- JavaScriptCore/wtf/ListHashSet.h.orig Tue Jul 8 23:23:01 2008
>+++ JavaScriptCore/wtf/ListHashSet.h Tue Jul 8 23:24:03 2008
>@@ -122,7 +122,7 @@ namespace WTF {
> : m_freeList(pool())
> , m_isDoneWithInitialFreeList(false)
> {
>- memset(m_pool.pool, 0, sizeof(m_pool.pool));
>+ memset(m_pool, 0, sizeof(m_pool));
> }
>
> Node* allocate()
>@@ -166,7 +166,7 @@ namespace WTF {
> }
>
> private:
>- Node* pool() { return reinterpret_cast<Node*>(m_pool.pool); }
>+ Node* pool() { return reinterpret_cast<Node*>(m_pool); }
> Node* pastPool() { return pool() + m_poolSize; }
>
> bool inPool(Node* node)
>@@ -177,10 +177,7 @@ namespace WTF {
> Node* m_freeList;
> bool m_isDoneWithInitialFreeList;
> static const size_t m_poolSize = 256;
>- union {
>- char pool[sizeof(Node) * m_poolSize];
>- double forAlignment;
>- } m_pool;
>+ uint32_t m_pool[(sizeof(Node) * m_poolSize + sizeof(uint32_t) - 1) /
sizeof(uint32_t)];
> };
>
> template<typename ValueArg> struct ListHashSetNode {
I think these changes should be reviewed in Mike's patch separately.
>--- JavaScriptCore/wtf/Platform.h.orig Tue Jul 8 23:24:28 2008
>+++ JavaScriptCore/wtf/Platform.h Tue Jul 8 23:25:54 2008
>@@ -172,6 +172,12 @@
> #define WTF_PLATFORM_X86_64 1
> #endif
>
>+/* PLATFORM(SPARC64) */
>+#if defined(__sparc64__)
>+#define WTF_PLATFORM_SPARC64 1
>+#define WTF_PLATFORM_BIG_ENDIAN 1
>+#endif
>+
> /* Compiler */
>
> /* COMPILER(MSVC) */
This looks good!
>--- JavaScriptCore/wtf/Threading.h.orig Sun Jul 6 19:10:20 2008
>+++ JavaScriptCore/wtf/Threading.h Sun Jul 6 19:10:49 2008
>@@ -168,26 +168,6 @@ class ThreadCondition : Noncopyable { (private)
> PlatformCondition m_condition;
> };
>
>-#if PLATFORM(WIN_OS) && !COMPILER(MSVC7)
>-#define WTF_USE_LOCKFREE_THREADSAFESHARED 1
>-
>-inline void atomicIncrement(int volatile* addend) {
InterlockedIncrement(reinterpret_cast<long volatile*>(addend)); }
>-inline int atomicDecrement(int volatile* addend) { return
InterlockedDecrement(reinterpret_cast<long volatile*>(addend)); }
>-
>-#elif PLATFORM(DARWIN)
>-#define WTF_USE_LOCKFREE_THREADSAFESHARED 1
>-
>-inline void atomicIncrement(int volatile* addend) {
OSAtomicIncrement32Barrier(const_cast<int*>(addend)); }
>-inline int atomicDecrement(int volatile* addend) { return
OSAtomicDecrement32Barrier(const_cast<int*>(addend)); }
>-
>-#elif COMPILER(GCC)
>-#define WTF_USE_LOCKFREE_THREADSAFESHARED 1
>-
>-inline void atomicIncrement(int volatile* addend) {
__gnu_cxx::__atomic_add(addend, 1); }
>-inline int atomicDecrement(int volatile* addend) { return
__gnu_cxx::__exchange_and_add(addend, -1) - 1; }
>-
>-#endif
>-
> template<class T> class ThreadSafeShared : Noncopyable {
> public:
> ThreadSafeShared(int initialRefCount = 1)
Should probably change #elif COMPILER(GCC) to:
#elif COMPILER(GCC) && !PLATFORM(OPENBSD)
>@@ -197,25 +177,17 @@ template<class T> class ThreadSafeShared : Noncopyable
>
> void ref()
> {
>-#if USE(LOCKFREE_THREADSAFESHARED)
>- atomicIncrement(&m_refCount);
>-#else
> MutexLocker locker(m_mutex);
> ++m_refCount;
>-#endif
> }
>
> void deref()
> {
>-#if USE(LOCKFREE_THREADSAFESHARED)
>- if (atomicDecrement(&m_refCount) <= 0)
>-#else
> {
> MutexLocker locker(m_mutex);
> --m_refCount;
> }
> if (m_refCount <= 0)
>-#endif
> delete static_cast<T*>(this);
> }
>
>@@ -226,17 +198,13 @@ template<class T> class ThreadSafeShared : Noncopyable
>
> int refCount() const
> {
>-#if !USE(LOCKFREE_THREADSAFESHARED)
> MutexLocker locker(m_mutex);
>-#endif
> return static_cast<int const volatile &>(m_refCount);
> }
>
> private:
> int m_refCount;
>-#if !USE(LOCKFREE_THREADSAFESHARED)
> mutable Mutex m_mutex;
>-#endif
> };
>
> // This function must be called from the main thread. It is safe to call it
repeatedly.
These changes aren't needed if you change the #eilf COMPILER(GCC) above.
>--- JavaScriptCore/wtf/Vector.h.orig Tue Jul 8 23:26:12 2008
>+++ JavaScriptCore/wtf/Vector.h Tue Jul 8 23:26:41 2008
>@@ -386,8 +386,7 @@ namespace WTF {
> static const size_t m_inlineBufferSize = inlineCapacity * sizeof(T);
> T* inlineBuffer() { return reinterpret_cast<T*>(&m_inlineBuffer); }
>
>- // FIXME: Nothing guarantees this buffer is appropriately aligned to
hold objects of type T.
>- char m_inlineBuffer[m_inlineBufferSize];
>+ uint64_t m_inlineBuffer[(m_inlineBufferSize + sizeof(uint64_t) - 1) /
sizeof(uint64_t)];
> };
>
> template<typename T, size_t inlineCapacity = 0>
I think these changes should be reviewed in Mike's patch separately.
>--- WebCore/html/CanvasRenderingContext2D.cpp.orig Sat Jun 28 15:31:53
2008
>+++ WebCore/html/CanvasRenderingContext2D.cpp Sat Jun 28 15:33:07 2008
>@@ -64,6 +64,8 @@
> #include <cairo.h>
> #endif
>
>+#define isfinite(x) finite(x)
>+
> namespace WebCore {
>
> using namespace HTMLNames;
>$OpenBSD$
>--- WebCore/platform/graphics/cairo/PathCairo.cpp.orig Sat Jun 28 15:32:43
2008
>+++ WebCore/platform/graphics/cairo/PathCairo.cpp Sat Jun 28 15:32:55
2008
>@@ -34,6 +34,8 @@
> #include <math.h>
> #include <wtf/MathExtras.h>
>
>+#define isfinite(x) finite(x)
>+
> namespace WebCore {
>
> Path::Path()
>$OpenBSD$
Again, defining this in config.h means you don't have to define it everywhere.
>--- WebCore/platform/text/AtomicString.cpp.orig Tue Jul 8 23:27:07
2008
>+++ WebCore/platform/text/AtomicString.cpp Tue Jul 8 23:27:35 2008
>@@ -94,7 +94,7 @@ static inline bool equal(StringImpl* string, const UCh
> if (string->length() != length)
> return false;
>
>-#if PLATFORM(ARM)
>+#if PLATFORM(ARM) || PLATFORM(SPARC64)
> const UChar* stringCharacters = string->characters();
> for (unsigned i = 0; i != length; ++i) {
> if (*stringCharacters++ != *characters++)
Looks good.
>--- WebCore/platform/text/StringHash.h.orig Tue Jul 8 23:27:50 2008
>+++ WebCore/platform/text/StringHash.h Tue Jul 8 23:29:14 2008
>@@ -46,6 +46,15 @@ namespace WebCore {
> if (aLength != bLength)
> return false;
>
>+#if PLATFORM(ARM) || PLATFORM(SPARC64)
>+ const UChar* aChars = a->characters();
>+ const UChar* bChars = b->characters();
>+ for (unsigned i = 0; i != aLength; ++i)
>+ if (*aChars++ != *bChars++)
>+ return false;
>+
>+ return true;
>+#else
> const uint32_t* aChars = reinterpret_cast<const
uint32_t*>(a->characters());
> const uint32_t* bChars = reinterpret_cast<const
uint32_t*>(b->characters());
>
>@@ -58,6 +67,7 @@ namespace WebCore {
> return false;
>
> return true;
>+#endif
> }
>
> static unsigned hash(const RefPtr<StringImpl>& key) { return
key->hash(); }
It would be nice if this change were relative to Mike's patch. If Mike doesn't
mind, though, maybe these changes could be combined (if you're willing to
resubmit the patch).
>--- WebKitTools/DumpRenderTree/DumpRenderTree.h.orig Sat Jun 28 15:34:07
2008
>+++ WebKitTools/DumpRenderTree/DumpRenderTree.h Sat Jun 28 15:34:15
2008
>@@ -46,8 +46,6 @@ extern CFRunLoopTimerRef waitToDumpWatchdog;
>
> #include <string>
>
>-std::wstring urlSuitableForTestResult(const std::wstring& url);
>-
> class LayoutTestController;
>
> extern volatile bool done;
Needs #if !PLATFORM(OPENBSD)/#endif around the code (unless there is another
macro that's used to ignore wide strings).
>--- autogen.sh.orig Tue Jul 1 18:02:03 2008
>+++ autogen.sh Tue Jul 1 18:02:25 2008
>@@ -42,8 +42,6 @@ if test "$DIE" -eq 1; then
> exit 1
> fi
>
>-rm -rf $top_srcdir/autom4te.cache
>-
> touch README INSTALL
>
> aclocal || exit $?
>@@ -51,7 +49,3 @@ $LIBTOOLIZE --force || exit $?
> autoheader || exit $?
> automake --foreign --add-missing || exit $?
> autoconf || exit $?
>-
>-cd $ORIGDIR || exit 1
>-
>-$srcdir/configure $AUTOGEN_CONFIGURE_ARGS "$@" || exit $?
You should probably confer with the Gtk porters to make sure these changes are
okay (to the point where it may be beneficial to have a separate bug just for
these autogen.sh changes).
Thanks again for the patch, Landry!
More information about the webkit-reviews
mailing list