Cortex-M3+ GCC unaligned access quirks

[abyrvalg]

Perhaps this could save someone from the same pitfall:

As we know, Cortex-M3 and above can handle unaligned memory accesses. Let's try the following code:

Code: [Select]

//compile with -mcpu=cortex-m3 -O2/-O3/-Os
extern void MyFn(int a, int b);

//Disable for now
//#define WITH_PACK

#ifdef WITH_PACK
    #define PACKED __attribute__((packed))
#else
    #define PACKED
#endif

typedef struct
{
    int a, b;
} PACKED MyStruct;

void DoTest(unsigned char *src)
{
    MyStruct *ptr = (MyStruct *)src;
    MyFn(ptr->a, 1); //OK
    MyFn(4, ptr->b); //OK
    MyFn(ptr->a, ptr->b); //Surprise!
}
- everything looks fine, the core should handle int field accesses even if the src pointer is unaligned, but in reality it would UsageFault on the 3rd MyFn() call
The reason is GCC combining sequential memory accesses into a single LDRD or LDM instruction and those, unlike single LDR, can't handle unaligned accesses.
The solution is to make MyStruct packed, even if it looks perfectly packed already. This doesn't split all accesses into individual bytes, GCC perfectly understands M3's capabilities and just issues two separate LDR instructions.

https://godbolt.org/z/TTKzq5dc1

Upd: -Wcast-align option detects such cases.
But the solution with packing the struct type feels wrong to me (1. it can cause unwanted packing if there are not so nice field types, 2. it breaks LDR->LDM optimizations everywhere, including places where the alignment is correct by design). The problem is not in a struct type itself, but in a more local bad pointer type. Does anyone know a better solution?

[brucehoult]

Here's the correct approved answer, in which a modern compiler will automatically use unaligned loads IFF the CPU supports them, and otherwise (e.g. on Cortex M0) will actually call memcpy or use aligned loads and shifts and stuff.

https://godbolt.org/z/v8dzv7qfj

Code: [Select]

void DoTest(unsigned char *src)
{
    MyStruct s;
    memcpy(&s, src, sizeof(s));
    MyFn(s.a, s.b); // No surprise!
}


Code: [Select]

DoTest:
        mov     r2, r0
        sub     sp, sp, #8
        mov     r3, sp
        ldr     r0, [r0]  @ unaligned
        ldr     r1, [r2, #4]      @ unaligned
        stmia   r3!, {r0, r1}
        add     sp, sp, #8
        b       MyFn

Note how the compiler explicitly marks that those ldr are unaligned.

I'm not sure why gcc is failing to optimise away the stack frame and stmia. Clang does much better, though the stupid thing should just reorder the loads and avoid the extra mov.

https://godbolt.org/z/ar34s9j8a

Code: [Select]

DoTest:
        ldr     r2, [r0]
        ldr     r1, [r0, #4]
        mov     r0, r2
        b       MyFn

Has godbolt.org been REALLY sluggish for anyone else recently?  Do we need to donate to Matt for more servers?

[nctnico]

Does anyone know a better solution?

Yes. Never, ever casts structs onto pointers which are of a different type (especially void or byte sized pointers). That is bad coding practise and begging for a world of pain on your bare knees. You have a compiler that knows how to align variables types while adhering to memory access abilities of the platform. Don't throw that out of the window.

[gf]

Never, ever casts structs onto pointers which are of a different type (especially void or byte sized pointers). That is bad coding practise and begging for a world of pain on your bare knees.

How else would you do "type erasure", for instance when you call qsort() in order to sort an array of structs?
Or how can you memcpy() a struct w/o casting the struct pointer to void* ?

[SiliconWizard]

Has godbolt.org been REALLY sluggish for anyone else recently?

Yeah. Some days it's very slow. Right now for me, it is relatively snappy.
As a quick note, I think the fact it triggers a recompile at each text change in the editor is a very bad idea. There should be a "compile" button instead, to avoid unnecessarily using sever resources.

[brucehoult]

Yes, I think the live recompile is past its use-by date and must increase server load by a factor of 10 or more.

[nctnico]

Never, ever casts structs onto pointers which are of a different type (especially void or byte sized pointers). That is bad coding practise and begging for a world of pain on your bare knees.

How else would you do "type erasure", for instance when you call qsort() in order to sort an array of structs?
Or how can you memcpy() a struct w/o casting the struct pointer to void* ?

When you memcpy, you'd copy one struct to another struct of the same type so the compiler deals with alignment. Same for qsort. Neither deals with the actual mapping of struct members to memory so alignment isn't an issue. But it is better to just tell the compiler struct A = struct B and let the compiler decide how to copy the contents. Where it comes to qsort: doing a bubble sort is quite easy so in plain C so I'd write the loop myself and keep using the right types. C is inviting people into bad code practises at many points. The mere fact something is possible, doesn't mean it is a good idea.

But the case the OP shows, is a typical example where things can go horribly wrong by taking a random pointer and mapping it onto a struct. Needing the 'packed' attribute is a red herring.

[brucehoult]

Never, ever casts structs onto pointers which are of a different type (especially void or byte sized pointers). That is bad coding practise and begging for a world of pain on your bare knees.

How else would you do "type erasure", for instance when you call qsort() in order to sort an array of structs?
Or how can you memcpy() a struct w/o casting the struct pointer to void* ?

In the qsort compare function you know where the pointer came from so you know it is properly aligned if the pointer you passed in to qsort in the first place was, even though the type information on the pointer isn't sufficient to indicate that. Usually the call to qsort and the compare function it uses are located very close to each other in the source code, so you can see what is going on. In a slightly better C you'd be able to declare the compare function inline in the actual call to qsort, but oh well.

Code: [Select]

void sortByBs(MyStruct *p, int n){
    qsort(p, sizeof(*p), n,
        [](const void *x, const void *y) -> int {
            return (*(MyStruct*)x).b < (*(MyStruct*)y).b;
        }
    );
}

And, yes, once you go this far it would be better to just take p as a capture in the lambda and have qsort's arguments be only n and the lambda (and start index for generality) but that's a whole different library, not just a more convenient syntax.

[Perkele]

Now try to compile it with -O0 and look at the generated output.
With "-Os" compiler is doing exactly what you told it to do, its trying reduce the size of the compiled program.

Latest Keil MDK (Clang) generates almost exactly the same code with "-Oz".

Any decent static analysis tool should at least give you a warning for doing this kind of cast.

[abyrvalg]

I'm not blaming the initial (w/o PACKED) behavior, it matches the standard (wide type pointers are assumed to be aligned by default), sequential access optimizations are great, Cortex-M has LDM/STM for even longer sequences and I'm glad to see them being used.
The question is how to tell the compiler "this pointer is unaligned, do the best you can on this arch" with a minimal overhead. Although the memcpy approach proposed above solves the alignment problem, it is a trick relying on implicit things, possibly leading to serious overhead (imagine a half-KB sized struct from which we are pulling 3 last ints instead of my shorter example).
 
For people who gets triggered by struct casts here is an array version (imagine it came via UART as a part of some packet) producing an unaligned LDRD: https://godbolt.org/z/P6oETcMc7

A side note: it is quite, ehm, interesting to see such devotion to portability of code most probably (taking into account the forum section where we are) receiving that data in question from some completely non-portable peripheral. UARTs and DMA controllers are are non-portable but we continue to use them somehow, writing nice (or not so nice) drivers when we want to abstract them. Why don't just treat core's "data aligner" as yet another hw feature requiring some "driver"?
Personally I'd be totally ok with some #ifdef COMPILER_A #define UNALIGNED this_way #elif ... #else #error "Sorry, I haven't thought of your CellBE, QDSP6 or whatever" #endif (otherwise I'd be programming in Java or MicroPython )

[nctnico]

Think about testing code on a PC and how much certainty that testing gives you your code will work as intended. The less code is platform dependant, the more certainty you have it will work on your platform because you have a greater test coverage. A typical embedded platform is much harder to debug software on compared to a PC. So having ifdefs for platform dependant stuff is only a last effort. If you need some fields from a 3kB struct, then use accessor functions that take a byte pointer + offset and shuffles byte-per-byte into the type you want to retreive from the struct. That will never fail and thus gives you certainty that code will always work as intended regardless the platform. Very handy if you work on protocol stacks where a myriad of other things can go wrong.

[SiliconWizard]

All this would of course depend on the project, complexity and performance requirements.

Personally, in most cases if I use packed structs (for a good reason), I'll arrange to make all fields naturally aligned. In particular for embedded dev, I almost never allow unaligned accesses, whether they are supported by the target or not.
If they can't be aligned, then I'll implement accessors / or a transformation of the data of some kind rather than direct access.

As always when it comes to non-portable code, if you absolutely have to write some, separate it as much as you can from the portable code, keep it at a minimum and document what is not portable and would need porting effort.

[gf]

The question is how to tell the compiler "this pointer is unaligned, do the best you can on this arch" with a minimal overhead.

You cannot tell the compiler. A valid pointer value must always have the same alignment as the the type to which the pointer points, otherwise dereferencing the pointer is UB. Only a pointer variable pointing to a type with alignment=1 (i.e. pointing to void, char, signed char, unsigned char, or to a packed struct) can contain an unaligned value. Consequently, the only thing you can do is choosing a packed struct as pointer target. Note however, that a regular struct and a packed struct with the same members are considered two different types, which do not necessarily have the same size, and which are not assignment compatible with each other.

[abyrvalg]

More fun, GCC's __attribute__((packed)) seems to be inapplicable to a non-struct pointer type at all: https://godbolt.org/z/MzY6o7vbT
all combinations resulted in ldm     r0, {r0, r1, r2}. I should just switch back to Keil, which allows things like uint32_t __packed *ptr

[nctnico]

More fun, GCC's __attribute__((packed)) seems to be inapplicable to a non-struct pointer type at all: https://godbolt.org/z/MzY6o7vbT
all combinations resulted in ldm     r0, {r0, r1, r2}. I should just switch back to Keil, which allows things like uint32_t __packed *ptr

Check the output window. It's says the packed attribute is ignored. IIRC the packed attribute can only be assigned to variables, not to types.

[SiliconWizard]

More fun, GCC's __attribute__((packed)) seems to be inapplicable to a non-struct pointer type at all: https://godbolt.org/z/MzY6o7vbT
all combinations resulted in ldm     r0, {r0, r1, r2}. I should just switch back to Keil, which allows things like uint32_t __packed *ptr

IIRC the packed attribute can only be assigned to variables, not to types.

No, it can be. But to struct types.
A "packed" uint32_t has no meaning that I know of.
If the intent is to force the compiler to do what it takes to handle a potentially unaligned access, I don't think you can outside the context of a struct.

[abyrvalg]

If the intent is to force the compiler to do what it takes to handle a potentially unaligned access, I don't think you can outside the context of a struct.

Yes, looks like that’s the only possible packed use with GCC. Found an example of wrapping a single uint32_t into a struct type just to be able to add that attribute. In contrast, both IAR and ARMCC allows adding a __packed prefix to almost any data pointer type, marking the pointer as "possibly unaligned".

[brucehoult]

Although the memcpy approach proposed above solves the alignment problem, it is a trick relying on implicit things, possibly leading to serious overhead (imagine a half-KB sized struct from which we are pulling 3 last ints instead of my shorter example).

You didn't try it, did you?

https://godbolt.org/z/sxKbMa1TE

Code: [Select]

typedef struct
{
    int junk[1000];
    int a, b;
} MyStruct;

void DoTest(unsigned char *src)
{
    MyStruct s;
    memcpy(&s, src, sizeof(s));
    MyFn(s.a, s.b); //Surprise!
}

Gives ...

Code: [Select]

DoTest:
        ldr.w   r2, [r0, #4000]
        ldr.w   r1, [r0, #4004]
        mov     r0, r2
        b       MyFn

And, in this case, if you switch to Cortex M0 it doesn't do the whole memcpy, but does individual byte loads and shifts and adds to load the 8 bytes of interest into register variables. It needs 23 instructions (46 bytes) but that's only going to be half a µs at typical M0 clock speeds.

For sure a lot faster than actually doing a memcpy on a 4K object then throwing most of it away.

If you don't want to trust the compiler *that* much, you can still simply do:

Code: [Select]

void DoTest(unsigned char *src)
{
    int a, b;
    MyStruct *s = (MyStruct*)src;
    memcpy(&a, &s->a, sizeof(a));
    memcpy(&b, &s->b, sizeof(b));
    MyFn(a, b);
}

... which gives ...

Code: [Select]

DoTest:
        ldr     r1, [r0, #4004]   @ unaligned
        ldr     r0, [r0, #4000]   @ unaligned
        b       MyFn

I'm not seeing the problem here, at least if you're using a standard, sensible, free, easily available compiler such as gcc or clang.

[westfw]

the memcpy approach

Having to use memcpy and rely on compiler optimizations to not actually do the memcpy, in order to do "type punning", really annoys me.
(frankly, having the compiler replace ANY function call as part of optimization because it "knows" what that function does is annoying.  C used to be so "pure" in that sense - a whole language definition with no pre-written functions!  Sigh.  As someone who spent a long time programming in assembly language, I tend to think in terms of generic pointers - a memory address that can point to anything, and be treated appropriately.  And I don't understand why the language people seem so resistant to providing that service.  Clearly the memcpy-that-isn't shows that the compiler can defeat optimization issues when it wants to - why can't I have a type-punning mechanism that does that it a more intuitively efficient manner?)

A valid pointer value must always have the same alignment as the the type to which the pointer points

Surely not?  A pointer to a 96byte struct doesn't have to have 96 (or 128) byte alignment...

[Siwastaja]

Yeah, casting struct over a char* buffer is problematic. Choose one of the following strategies:

(1) just don't do it. Use memcpy as nctnico suggests,
(2) only use packed structs when overlaying a struct over buffer. Compilers make no assumptions about alignment and create code which always works, but with performance penalty
(3) know all the features and assumptions built in the CPU and compiler. Code won't be very portable and requires care to write, but if the programmer knows what they are doing, performance and code size could be better than in (2).

Good opening post because it demonstrates, in one go, how (3) fails (because even though CPU supposedly "supports" unaligned access, clearly this support has non-obvious limitations) and (2) succeeds (as it always does for everyone except nctnico). (1) was not tried, but it would obviously work.

[gf]

A valid pointer value must always have the same alignment as the the type to which the pointer points

Surely not?  A pointer to a 96byte struct doesn't have to have 96 (or 128) byte alignment...

Alignment of a struct is not a matter of its size, but is the maximum of the alignments of its components. The size of a struct is padded to an integral multiple of its alignment in order that each element is still properly aligned in an array of structs. The declaration

Code: [Select]

typedef struct { int i; double d; } S1;
typedef struct { ...whatsoever... } __attribute__((packed)) S2;
S1* ptr1;

has the following implications

Code: [Select]

_Alignof(S2) == 1    // packed


Code: [Select]

_Alignof(S1) == max(_Alignof(int),_Alignof(double))


Code: [Select]

sizeof(S1) % _Alignof(S1) == 0

and the following condition must be met in order that dereferencing ptr1 is not UB

Code: [Select]

ptr1 != NULL && (uintptr_t)ptr1 % _Alignof(S1) == 0


[hans]

I'm not saying you should use the following (because its still encouraging pointer casts), but it does mitigate the underlying issue on GCC.

GCC assumes that a pointer of MyStruct refers to a storage that is aligned as the object is statically allocated. If its stored in a byte buffer that assumption is invalid. Copying to local variable/struct fixes this allocation by relying on memcpy() dealing with this misalignment.

I've found GCC has the "__builtin_assume_aligned", which can be used as a minimum alignment (which doesn't help, as we need to annotate a maximum alignment of up to 4 bytes as thats the maximum width that can be loaded with unaligned pointers). However, there is also a variant where you can say which byte alignment the struct has on a certain modulo of bytes.
So:

Code: [Select]

void DoTest(unsigned char *src)
{
    MyStruct* pS = (MyStruct*) (src);
    pS = __builtin_assume_aligned(pS, 8, 1);
    MyFn(pS->a, pS->b); //No Surprise!
}

"""Fixes""" this issue by telling GCC that for sure the pointer is aligned to 1 byte offset out of 8.

However, I'm not sure why Clang ignores this annotation and will still go for the ldrd or ldm instruction even though this code is explicitly saying that will cause problems. So it does not seem to be a very stable 'fix', and honestly, I personally wouldn't want to go around my code and telling the compiler which pointers might be crooked.

So I agree with nctnico and bruce. Just use memcpy to make sure the object allocation is sound.
Unfortunately, GCC seem to blindly copy the whole object with memcpy.

Interestingly, when I use in GCC 13.x:

Code: [Select]

typedef struct
{
    int junk[1000];
    int a, b;
} /* __attribute__((packed)) */ MyStruct;

void DoTest(unsigned char *src)
{
    MyStruct s = *(MyStruct*)src;
    MyFn(s.a, s.b); //Surprise!
}
I get:

Code: [Select]

DoTest:
        ldr     r1, [r0, #4004]
        ldr     r0, [r0, #4000]
        b       MyFnOr have atleast 1 other 'int' before a,b.
But with any preceding field removed, it uses LDM again.
Same behaviour on clang.

[gf]

I've found GCC has the "__builtin_assume_aligned", which can be used as a minimum alignment (which doesn't help, as we need to annotate a maximum alignment of up to 4 bytes as thats the maximum width that can be loaded with unaligned pointers). However, there is also a variant where you can say which byte alignment the struct has on a certain modulo of bytes.

My understanding of the doc is, that __builtin_assume_aligned(ptr,a,b) lets GCC assume ((uintptr_t)(ptr)-b)%a==0.
So I wonder I wonder why 4,1 and 8,1 works, and 2,1 does not.
Shouldn't the latter assume (address-1)%2==0, i.e. odd addresses, which are not suitable for ldrd or ldrm either?

Seems that GCC's __builtin_assume_aligned() attaches the given alignment to a value (not to a type or variable), and the property is then propagated through the SSA representation of the function to derived values -- as far as GCC can trace the data flow statically. Since data flow traceability is limited, you can only rely on correct propagation to a limited extent. Check for instance the difference between lines 20 and 21. Both are the same expression, but GCC cannot determine whether p2[idx] still has the same value in line 21, as it was in line 20. Therefore the alignment tag is lost in line 21. Or when MyFn2() is called, the tag ist (silently) discarded, too.

https://godbolt.org/z/ErqdYnPdx

[peter-h]

You can also configure whether a F4 faults an unaligned access or not, and the compiler won't know what you have selected.

This thread is interesting but confirms that by totally avoiding pointers (except when using the &variable when passing parms to functions, etc) I am saving myself a load of hassle

Can you imagine somebody picking up your code a few years later? I sometimes include EEVBLOG thread URLs in my comments, and much more often in project documentation, but still...

[DiTBho]

Alignment of a struct is not a matter of its size, but is the maximum of the alignments of its components. The size of a struct is padded to an integral multiple of its alignment in order that each element is still properly aligned in an array of structs. The declaration

Code: [Select]

typedef struct { int i; double d; } S1;
typedef struct { ...whatsoever... } __attribute__((packed)) S2;
S1* ptr1;

has the following implications

Code: [Select]

_Alignof(S2) == 1    // packed


Code: [Select]

_Alignof(S1) == max(_Alignof(int),_Alignof(double))


Code: [Select]

sizeof(S1) % _Alignof(S1) == 0

and the following condition must be met in order that dereferencing ptr1 is not UB

Code: [Select]

ptr1 != NULL && (uintptr_t)ptr1 % _Alignof(S1) == 0


this is also what my-c does, and that condition must be met, otherwise (for UB avoidance) it refuses to compile.