[MLton] Card/cross map alignment broken?

[Wesley W. Terpstra]

On Wed, Oct 14, 2009 at 2:10 AM, Wesley W. Terpstra <wesley at terpstra.ca>wrote:

> Ok, but it is definitely updateCrossMap that is doing the unaligned
> accesses.
>
> gdb says the address of the trap is...
> (gdb) break *(void*)0x0000000120d843ac
> 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. :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mlton.org/pipermail/mlton/attachments/20091014/c02a42e9/attachment.html