[MLton] Card/cross map alignment broken?

[Matthew Fluet]

Thanks for the explanation.  The use of aligning the "pointer" type
was meant for heap objects that are (at least) 4 byte aligned.

On Tue, Oct 13, 2009 at 9:43 PM, Wesley W. Terpstra <wesley at terpstra.ca> wrote:
> 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. :)
>
>
> _______________________________________________
> MLton mailing list
> MLton at mlton.org
> http://mlton.org/mailman/listinfo/mlton
>