[MLton-commit] r7259

[Wesley Terpstra]

    gdb says the address of the trap is...
    Breakpoint 1 at 0x120d843ac: file gc/generational.c, line 373.
    => s->generationalMaps.crossMap[cardIndex] = (GC_crossMapElem)offset;


... shockingly, gdb was correct. It took me some time to wrap my mind around 
this, but there is indeed a bug in that line. Or rather, that line became 
buggy when r4683 tried to quell a warning. The warning was harmless, but the 
"fix" wasn't.

Here's what is going on:
  typedef uint8_t GC_crossMapStart __attribute__ ((aligned(4));
defines a type that is a uint8_t, which happens to be aligned on a 4-byte 
boundary. That's all fine and dandy. Make a pointer type to the object, well 
that pointer is 4-byte aligned. Swell.

Now deference that pointer at [1].

Uhm.

Well, the object type pointed to is aligned(4), so the (pointer+1) must be 
4-byte aligned. I'm an alpha and loading a byte is expensive, but no problem,
it's word aligned! Let's just load it using a word32 memory access. 
BAM! unaligned access.

Morale of the story? Don't mess around with __attribute__ ((aligned))! Two of 
the three places it is used in MLton are wrong and I'm not 100% sure the 
remaining use (pointer) is safe either. I assume that "pointer" is used only 
to refer to objects on the heap. That means that on an alpha it is actually
8-byte aligned. Telling the alpha it's less aligned than it actually is will 
either degrade performance or have no effect at all.

When you cast pointers you might get a warning, but gcc is also doing the 
'right thing' for MLton's purposes. If I cast a char* to an int* gcc assumes 
that I know what I'm doing and that the char* pointer was really aligned to 
an int after all. That's why it issues a warning, to let you know it just 
increased its alignment assumptions. Telling gcc that the char* was 4-byte 
aligned quells the warning (on a 32-bit system) but doesn't improve the 
assembler. All it does is introduce potential bugs if you ever derefence 
that char* pointer at say [1] [2] or [3]. So there is absolutely nothing 
to gain (performance-wise) by declaring these types as aligned and everything 
to lose (correctness-wise).

As for the warnings, they aren't even warnings that are enabled by default in 
C! We turned them on with -Wcast-align. They are off by default because lots 
of correct C code (including ours) do casts like this. The align(4) hack also 
doesn't help on 64-bit ports...  I get flooded with these warnings on ia64 and 
alpha.

I am going to unilaterally remove all __attribute__ ((align))s and quell this 
class of warning by removing -Wcast-align. After my rant I assume no one dares 
to object. :)


----------------------------------------------------------------------

U   mlton/trunk/runtime/Makefile
U   mlton/trunk/runtime/gc/generational.h
U   mlton/trunk/runtime/gen/gen-types.c
U   mlton/trunk/runtime/util/pointer.h

----------------------------------------------------------------------

Modified: mlton/trunk/runtime/Makefile
===================================================================
--- mlton/trunk/runtime/Makefile	2009-10-13 18:13:51 UTC (rev 7258)
+++ mlton/trunk/runtime/Makefile	2009-10-14 01:43:05 UTC (rev 7259)
@@ -56,7 +56,7 @@
 endif
 
 ifeq ($(TARGET_ARCH), alpha)
-FLAGS += -mieee
+FLAGS += -mieee 
 endif
 
 ifeq ($(TARGET_ARCH), amd64)
@@ -157,7 +157,7 @@
 WARNCFLAGS += -Wundef
 WARNCFLAGS += -Wshadow
 WARNCFLAGS += -Wpointer-arith
-WARNCFLAGS += -Wbad-function-cast -Wcast-qual -Wcast-align
+WARNCFLAGS += -Wbad-function-cast -Wcast-qual
 WARNCFLAGS += -Wwrite-strings
 # WARNCFLAGS += -Wconversion
 WARNCFLAGS += -Waggregate-return

Modified: mlton/trunk/runtime/gc/generational.h
===================================================================
--- mlton/trunk/runtime/gc/generational.h	2009-10-13 18:13:51 UTC (rev 7258)
+++ mlton/trunk/runtime/gc/generational.h	2009-10-14 01:43:05 UTC (rev 7259)
@@ -16,10 +16,8 @@
 typedef uint8_t GC_cardMapElem;
 typedef uint8_t GC_crossMapElem;
 
-typedef GC_cardMapElem GC_cardMapStart __attribute__ ((aligned (4)));
-typedef GC_crossMapElem GC_crossMapStart __attribute__ ((aligned (4)));
-typedef GC_cardMapStart *GC_cardMap;
-typedef GC_crossMapStart *GC_crossMap;
+typedef GC_cardMapElem *GC_cardMap;
+typedef GC_crossMapElem *GC_crossMap;
 
 typedef size_t GC_cardMapIndex;
 typedef size_t GC_crossMapIndex;

Modified: mlton/trunk/runtime/gen/gen-types.c
===================================================================
--- mlton/trunk/runtime/gen/gen-types.c	2009-10-13 18:13:51 UTC (rev 7258)
+++ mlton/trunk/runtime/gen/gen-types.c	2009-10-14 01:43:05 UTC (rev 7259)
@@ -66,8 +66,7 @@
   // "typedef void* Pointer;",
   // "typedef uintptr_t Pointer;",
   // "typedef unsigned char* Pointer;",
-  // "struct PointerAux { unsigned char z[4]; } __attribute__ ((aligned (4), may_alias));",
-  "typedef unsigned char PointerAux __attribute__ ((aligned (4), may_alias));",
+  "typedef unsigned char PointerAux __attribute__ ((may_alias));",
   "typedef PointerAux* Pointer;",
   "#define Array(t) Pointer",
   "#define Ref(t) Pointer",

Modified: mlton/trunk/runtime/util/pointer.h
===================================================================
--- mlton/trunk/runtime/util/pointer.h	2009-10-13 18:13:51 UTC (rev 7258)
+++ mlton/trunk/runtime/util/pointer.h	2009-10-14 01:43:05 UTC (rev 7259)
@@ -8,7 +8,7 @@
 
 // typedef void* pointer;
 // typedef unsigned char* pointer;
-typedef unsigned char pointerAux __attribute__ ((aligned (4), may_alias));
+typedef unsigned char pointerAux __attribute__ ((may_alias));
 typedef pointerAux* pointer;
 
 #if POINTER_BITS == 32