Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packed etl::unaligned_type #988

Open
rolandreichweinbmw opened this issue Dec 6, 2024 · 7 comments
Open

Packed etl::unaligned_type #988

rolandreichweinbmw opened this issue Dec 6, 2024 · 7 comments

Comments

@rolandreichweinbmw
Copy link

Hi!

I'm using etl::unaligned_type in a packed struct like this:

struct __attribute__((packed)) B
{
    ::etl::be_uint16_t x;
    ::etl::be_uint32_t y;
};

Unfortunately, with gcc, this results in:

Example.cpp:23:24: error: ignoring packed attribute because of unpacked non-POD field ‘etl::be_uint16_t {anonymous}::B::x’ [-Werror]

I traced this to the fact that etl::unaligned_type is a nested struct/class, including etl::private_unaligned_type::unaligned_type_common within etl::unaligned_type.

Now, when I add the packed attribute just to the outer class etl::unaligned_type, the problem goes away:

class __attribute__((packed)) unaligned_type : public private_unaligned_type::unaligned_type_common<sizeof(T)>
{
...
}

What do you think? Would it be reasonable to add this to unaligned_type? Or would you suggest sth. different?

Using the ARM GCC Toolchain:

$ arm-none-eabi-g++ --version
arm-none-eabi-g++ (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release)
@jwellbelove
Copy link
Contributor

The 'packed' attribute would have to be defined in something like ETL_PACKED for it to be cross compiler compatible.

@rolandreichweinbmw
Copy link
Author

The 'packed' attribute would have to be defined in something like ETL_PACKED for it to be cross compiler compatible.

I.e. like this: #989 ?

@benedekkupper
Copy link
Contributor

Adding packed keyword doesn't give you any benefit, the point of these unaligned types that they are already packed

@rolandreichweinbmw
Copy link
Author

rolandreichweinbmw commented Dec 19, 2024

Consider this code with a simplified etl::unaligned_type:

#include <iostream>
#include <cstdint>

namespace etl {

template<size_t SIZE>
class unaligned_type_common
{
    protected:
        unsigned char _storage[SIZE];
};

template<typename T>
class unaligned_type: public unaligned_type_common<sizeof(T)>
{
};

} // namespace etl

// Typical and common pattern when packing data:
struct __attribute__((packed)) A
{
    etl::unaligned_type<uint8_t>  _a1;
    etl::unaligned_type<uint32_t> _a2;
};

int main()
{
    std::cout << "Size: " << std::to_string(sizeof(A)) << std::endl;
    return 0;
}

With GCC, it correctly prints:

Size: 5

However, it gives this warning:

<source>:22:35: warning: ignoring packed attribute because of unpacked non-POD field 'etl::unaligned_type<unsigned char> A::_a1'
   22 |     etl::unaligned_type<uint8_t>  _a1;
      |   

Turns out to be a known GCC issue: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83732

(Clang doesn't have this issue.)

The change in #989 fixes the issue, although it can rather be considered a GCC workaround and not an actual packing issue.

We can document this fact where ETL_PACKED is applied to unaligned_type.

@jwellbelove
Copy link
Contributor

Yes, this is more of a GCC specific fix, but ETL_PACKED is also a reasonable addition for cross compiler compatibility.
I've modified the syntax in the PR slightly to allow for compilers, such as MSVC, that use pragmas.

Eg.

struct ETL_PACKED Data
{
  uint32_t a = 0x12345678;
  uint8_t  b = 0x9A;
  uint32_t c = 0x87654321;
}; ETL_END_PACKED

@benedekkupper
Copy link
Contributor

I have no problem compiling this:

#include "etl/unaligned_type.h"
struct slim
{
  etl::le_uint8_t a;
  etl::le_uint32_t b;
};
static_assert(sizeof(slim) == 5);

so I still don't see what is the benefit of bringing in the mess with the packed attribute.

@rolandreichweinbmw
Copy link
Author

You are not using the packed attribute in your application code (using ETL).

Imagine a codebase that uses the packed attribute (as I regularly experienced).

Then, with GCC, the warning is generated, as noted above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants