2

In my code, I have something like this:

#include <stdint.h>

typedef struct __attribute__((packed)) {
    uint8_t test1;
    uint16_t test2;
} test_struct_t;
test_struct_t test_struct;

int main(void)
{
    uint32_t *ptr = (uint32_t*) &test_struct;
    return 0;
}

When I compile this using arm-none-eabi-gcc, I get the warning

.\test.c:11:2: warning: converting a packed 'test_struct_t' pointer (alignment 1) to a 'uint32_t' {aka 'long unsigned int'} pointer (alignment 4) may result in an unaligned pointer value [-Waddress-of-packed-member]

Can anyone tell me why this is happening? Taking the address of a packed struct member is of course dangerous. But the whole struct itself should always be aligned, shouldn't it?

Adrian Mole
  • 49,934
  • 160
  • 51
  • 83
energetic
  • 897
  • 1
  • 5
  • 7
  • 2
    No. Struct does not have to be aligned: https://godbolt.org/z/jhMza8cEe – 0___________ Aug 03 '21 at 18:02
  • 1
    I am packing the struct because it represents some registers, i.e. a contiguous piece of memory. Hence I need to make sure that all the members are packed together without any padding. This struct is later on passed to a function which takes a uint32_t* pointer as argument, that is where the pointer casting occurs. – energetic Aug 03 '21 at 18:05
  • 1
    @0___________ I am not sure whether your example is comparable as x86_64 is a completely different architecture. Not sure how alignment is there, but at least on ARM based platforms, all structs and variables are always 4-byte aligned. – energetic Aug 03 '21 at 18:08
  • [Raymond has some informative comments on why pack should be avoided](https://devblogs.microsoft.com/oldnewthing/20200103-00/?p=103290). You might avoid some of the pitfalls if you make sure the struct is properly aligned. – jwdonahue Aug 03 '21 at 18:09
  • 1
    @jwdonahue Thanks for the article :) I am well aware that packed structs are not unproblematic, but as I explained before, I am not doing this because it such a nice feature, I am doing this because there is simply no way around it. – energetic Aug 03 '21 at 18:13
  • 1
    @jwdonahue this is not something which can be suggested generally. In particular, in embedded word using packed struct is often the best thing to do. – SergeyA Aug 03 '21 at 18:15
  • Then you will simply have to take the perf hit. – jwdonahue Aug 03 '21 at 18:15
  • @jwdonahue depending on ARM version, it would be a crash rather than a perf hit. – SergeyA Aug 03 '21 at 18:16
  • Perhaps we should ask if the OP needs the struct aligned to one byte, or whether they simply need it to be packed? – jwdonahue Aug 03 '21 at 18:18
  • 1
    Casting the address of that struct to a `uint32_t *` is a violation of the strict aliasing rule, which means it's undefined behavior. What you *can* do is cast the address of the struct to a `uint8_t *`, and then read the bytes individually, and pack them into a `uint32_t` to pass to the function. – user3386109 Aug 03 '21 at 18:18
  • 1
    @user3386109 there is no undefined behavior in OP's snippet. – SergeyA Aug 03 '21 at 18:25
  • @user3386109, I used to have a collection of macros for that kind of shuffling. It's just bit shifting in registers really. But I think OP mentioned needing to pass a pointer to these three bytes to an API, so those probably wouldn't work here. – jwdonahue Aug 03 '21 at 18:25
  • 1
    You can resolve it I think with `__attribute__((packed, aligned(4)))` – Clifford Aug 03 '21 at 18:27
  • @Clifford why not post this as an answer? – SergeyA Aug 03 '21 at 18:28
  • 1
    @energetic, What is the address of these memory mapped registers? Would it be possible to copy the bits to a properly aligned address, hand them to the API, then write them back to the registers when it's done with them? – jwdonahue Aug 03 '21 at 18:28
  • 1
    Okay, I get the whole H/W register thing [because I've written device drivers for similar devices]. Does your H/W device allow byte access to the 16 bit register? And/or does your [arm] processor allow 16 bit fetch/store to unaligned addresses? Does the subfunction just use the values or would it also try to modify the values (i.e. the subfunction will try to write to the H/W device register directly)? – Craig Estey Aug 03 '21 at 18:31
  • This seems like a very poorly designed register bank and API, from what we know of the problem at this point. – jwdonahue Aug 03 '21 at 18:36
  • 1
    The address is 0x60000000, so it is actually 4-byte aligned. This was also why I am wondering why this warning occurs. But the longer I think about it, it actually makes at least some sense. The struct is packed and thus one byte aligned. Casting the pointer to uint32 would require something divisible by 4, which cannot be guaranteed by a packed struct (and is indeed not the case in my example above). And yes, I have to pass the pointer to an API which takes a uint32 pointer. And in our case the struct is actually divisible by 4, but I guess the warning is a general one. – energetic Aug 03 '21 at 18:47
  • 1
    @SergeyA : Because the question is "_Can anyone tell me why this is happening?_"; not "_How do I resolve the warning?_", so it would not be an answer to the question. – Clifford Aug 03 '21 at 18:50
  • You've presented us with an [X/Y problem](https://en.wikipedia.org/wiki/XY_problem). You got your answer for why you get the warning on this struct. You should ask another question that specifies all of your requirements, so we can assist you in solving them. – jwdonahue Aug 03 '21 at 18:51
  • I should point out that if the 1 byte register is aligned at that address, then the uint16_t that follows it, is not properly aligned, unless you have a very strange architecture/configuration. – jwdonahue Aug 03 '21 at 18:55
  • 1
    No, this is just fine. My program is already working the way it is now. I was just curious about the warning because - as I said - I know that variables are 4 byte aligned by default and only the _members_ of a packed structs aren't. So all I wanted is to understand why this happens. – energetic Aug 03 '21 at 18:56
  • 2
    It happens because the struct you declared can be aligned on any one byte boundary, which could cause serious issues on some targets. There's also better ways to solve the problem. I am betting that if we knew the spec's on this register bank and the API in question, we could provide you with a much more elegant and portable solution. – jwdonahue Aug 03 '21 at 18:59

3 Answers3

2

There is an answer in the comments, but since it's author didn't post it, I take the liberty to post it myself. All the credit is due to @Clifford.

By default, when the struct is packed, compilers also change alignment of the struct to 1 byte. However, for your case you need the struct to be both packed and aligned as 32-bit unsigned integer. This can be done by changing the packing attribute as following:

#include <stdint.h>

struct __attribute__((packed, aligned(sizeof(uint32_t)))) TestStruct {
    uint8_t test1;
    uint16_t test2;
};

struct TestStruct test_struct;

int32_t* p = (int32_t*)(&test_struct);

This compiles for ARM platform without any warnings.

SergeyA
  • 61,605
  • 5
  • 78
  • 137
  • 2
    I don't think we've actually established that the OP needs these three bytes to be aligned on a 32 bit address. In fact, I think the the OP's original questions were answered by Adrian. This is a classic [X/Y problem](https://en.wikipedia.org/wiki/XY_problem). The OP has specified hardware requirements and design goals and encountered a road block with this attempted solution. We need to know more about the hardware and that's really a different problem. Adrian already answered the questions that the OP posted. – jwdonahue Aug 03 '21 at 18:48
  • 1
    @jwdonahue Also, it's not clear, really, why you would want a pointer to a 4-byte integer pointing to a 3-byte structure. – Adrian Mole Aug 03 '21 at 18:56
  • @jwdonahue : Maybe but the "OP has asked "_the whole struct itself should always be aligned, shouldn't it?", so has clearly assumed that it would be. – Clifford Aug 03 '21 at 18:56
  • 1
    I actually like this solution, as it fixes the warning by working on its root cause and the behaviour of my stuff is still unchanged. – energetic Aug 03 '21 at 18:57
  • 1
    I appreciate the credit, but the OP did not actually ask how to force the alignment, he just asked why it might not be aligned already. That is why I did not post - even though this answer is probably more useful than an arcane explanation. @energetic You realise you did not actually ask for a _solution_, only an _explanation_ ;-) – Clifford Aug 03 '21 at 19:00
  • I agree with @Clifford. The OP seems to have caught a case of the WMF virus and wanted someone to explain why the compiler doesn't agree with them. – jwdonahue Aug 03 '21 at 19:11
  • The thing about using both `packed` and `align` attributes is that the latter sort of *defies* the former. Without the `align`, `sizeof(TestStruct)` will be (as expected) 3 bytes; with it, `sizeof(TestStruct)` will be 4 bytes - so there is 1 byte of 'padding' (presumably at the end of the structure). That's the only way an array of such structures could possibly work. – Adrian Mole Aug 05 '21 at 18:45
  • @AdrianMole the whole point of OPs question is to remove padding between the members (since they are aliased to registers). The fact that the structure itself becomes 4 bytes is not problematic to OP. – SergeyA Aug 05 '21 at 18:50
1

In my experience, "packed" structs are almost always a bad idea. They don't always do what people think they do, and they might do other things as well. Depending on compilers, target processors, options, etc., you might find the compiler generating code that uses multiple byte accesses to things that you expect to be 16-bit or 32-bit accesses.

Hardware registers are always going to be properly aligned on a microcontroller. (Bitfields may be a different matter, but you are not using bitfields here.) But there might be gaps or padding.

And the whole idea of trying to access this using a pointer to a uint32_t is wrong. Don't access data via pointer casts like this - in fact, if you see a pointer cast at all, be highly suspicious.

So how do you get a structure that matches a hardware structure exactly? You write it out explicitly, and you use compile-time checks to be sure:

#pragma GCC diagnostic error "-Wpadded"

struct TestStruct {
    uint8_t test1;
    uint8_t padding;
    uint16_t test2;
};

_Static_assert(sizeof(struct TestStruct) == 4, "Size check");

The padding is explicit. Any mistakes will be caught by the compiler.

What if you really, really want an unaligned 16-bit field in the middle here, and you haven't made a mistake in reading the datasheets? Use bitfields:

#pragma GCC diagnostic error "-Wpadded"

struct TestStruct2 {
    uint32_t test1 : 8;
    uint32_t test2 : 16;
    uint32_t padding : 8;
};

_Static_assert(sizeof(struct TestStruct2) == 4, "Size check");

Put the padding in explicitly. Tell the compiler to complain about missing padding, and also check the size. The compiler doesn't charge for the extra microsecond of work.

And what if you really, really, really need to access this as a uint32_t ? You use a union for type-punning (though not with C++) :

union TestUnion {
    uint32_t raw;
    struct TestStruct2 s;
};
David
  • 132
  • 3
-1

Your packed structure has a size of 3 bytes, and there can be no padding in it. Thus, if we were to create an array of such structures, with the first element having a 4-byte aligned address then, by the definition of arrays (contiguous memory), the second element would be three bytes (sizeof(struct test_struct_t)) from that. Thus, the second element would have only single byte alignment – so, the alignment requirement of your structure is, by deduction, one byte.

On your ARM platform, a unit32_t requires 4 byte alignment, hence the warning.

Adrian Mole
  • 49,934
  • 160
  • 51
  • 83
  • Changing the order of the fields should change the alignment on the struct yes? But it won't change the fact that the struct is only 3 bytes. – jwdonahue Aug 03 '21 at 18:11
  • No - changing the order of elements in a *packed* structure won't change its overall size. – Adrian Mole Aug 03 '21 at 18:11
  • Not its size, but the structs starting alignment. – jwdonahue Aug 03 '21 at 18:12
  • 1
    But how would that change the alignment requirement, as deduced from the array example? – Adrian Mole Aug 03 '21 at 18:13
  • I seem to recall there's a rule that the pointer to struct is always the same as pointer to the first field. – jwdonahue Aug 03 '21 at 18:13
  • There is such a rule, which would make your packed structure non-viable if the first element had an alignment requirement greater than 1 byte. – Adrian Mole Aug 03 '21 at 18:15
  • Not 100% convinced that this is the solution. The alignment requirement of packed structs is _always_ one byte. Even if you have a packed struct with only a single uint32_t in it, the same warning occurs. However, making an array out of packed structs could indeed lead to errors, you are correct there. – energetic Aug 03 '21 at 18:17
  • So you use whatever alignment attribute GCC provides, but it's all probably moot anyway. – jwdonahue Aug 03 '21 at 18:20
  • 1
    I would not say this. Using packed structs to represent registers and memories is common practice in embedded systems. – energetic Aug 03 '21 at 18:31
  • I've been designing, building and programming embedded systems since the mid 80's. I understand all the ways we have to get around the "normal" modes of programming. Specifying alignment is one of them. – jwdonahue Aug 03 '21 at 18:32
  • Ok, so what are you doing when you have some registers containing multiple fields of different size? You just define a pointer or macro for each field? IMO, this is a lot less readable and more work to handle all of it. – energetic Aug 03 '21 at 18:36
  • @energetic the answer to that question is dependent on the exact spec's of the register banks, their fields and uses, as well as what might be connected to any internal devices or external pins that map to those registers/fields. But a field struct comes to mind. – jwdonahue Aug 03 '21 at 19:06