[Bug c++/108342] New: std::complex: ignoring packed attribute because of unpacked non-POD field

[Bug c++/108342] New: std::complex: ignoring packed attribute because of unpacked non-POD field

[ruilvo at ua dot pt]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

            Bug ID: 108342
           Summary: std::complex: ignoring packed attribute because of
                    unpacked non-POD field
           Product: gcc
           Version: 12.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ruilvo at ua dot pt
  Target Milestone: ---

Created attachment 54217
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54217&action=edit
Code to reproduce the bug report

Summary:

With the annexed code, reproduced on the following CE link:
https://godbolt.org/z/1q9a5Mq7r

I get the following warning:

```
<source>:29:16: warning: ignoring packed attribute because of unpacked non-POD
field 'fcomplex_t <unnamed struct>::bb_iq_samples [512]'
   29 |     fcomplex_t bb_iq_samples[BB_FRAME_IQ_SAMPLES_COUNT];
      |                ^~~~~~~~~~~~~
```

Clang doesn't complain facing the same code.


Details:

I was trying to write a C++ library, which is going to target an ARM Linux
system, but leaving C-compatible "bindings" and structs for using with CFFI on
other languages.

The library deals with a serial stream that I don't control, and that I wanted
to de-serialize, thus the weird structure. 

I discovered the problem (warning) when I changed from a C source file and
`_Complex float` to C++ and `std::complex<float>`. 

The compiler I first discovered the problem on was arm-linux-gnueabihf-g++
(GCC) 12.2.1 20221203 [releases/gcc-12 revision
c03cb4b762aceeba95da918b042583af0d9f6030] from Linaro. 

The bug is reproducible on x86 and mainline GCC (see Compiler Explorer URL)


Expected behaviour:

For this code to compile without warnings and producing the desired effect
(packing). The C++ standard even carves out a guarantee than `_Complex
[float|double]` is memory-layout-compatible with
`std::complex<[float|double]>`.

[Bug c++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>The C++ standard even carves out a guarantee than `_Complex [float|double]` is memory-layout-compatible with `std::complex<[float|double]>`.

I know about _Atomic and std::atomic but not std::complex and _Complex. Because
std::complex was part of C++98 which was done before C99's _Complex ...

[Bug libstdc++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Hmm: diff.cpp03.numerics

[Bug libstdc++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[ruilvo at ua dot pt]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

--- Comment #3 from Rui Oliveira <ruilvo at ua dot pt> ---
(In reply to Andrew Pinski from comment #1)
> 
> I know about _Atomic and std::atomic but not std::complex and _Complex.
> Because std::complex was part of C++98 which was done before C99's _Complex
> ...

[complex.numbers.general] mentions:

If z is an lvalue of type cv complex<T> then: 
the expression reinterpret_­cast<cv T(&)[2]>(z) is well-formed,

https://eel.is/c++draft/complex.numbers.general

Basically stating that std::complex<T> is layouted exactly the same as T[2]. 

C standard says something similar iirc.

[Bug libstdc++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[ruilvo at ua dot pt]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

--- Comment #4 from Rui Oliveira <ruilvo at ua dot pt> ---
(In reply to Andrew Pinski from comment #2)
> Hmm: diff.cpp03.numerics

I saw you moved the bug to libstdc++ but is the problem libstdc++, or should
g++ just accept packing when it encounters it?

[Bug libstdc++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I don't think there's anything the library can do here. The layout of
std::complex is fixed, as stated above. And the fact it's a non-POD is also
fixed.

If the front-end warns about trying to pack a non-POD, then the library can't
stop it warning about std::complex<float>.

I don't know whether clang allows packing non-PODs, or just doesn't ever warn
for them, or has a special case for std::complex, or does something smarter
like not warn for types that have no padding bytes anyway (so that packing them
would be a no-op).

[Bug c++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|libstdc++                   |c++

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
clang:
typedef struct __attribute__((__packed__))
{
    /** Every frame starts with BB_FRAME_MAGIC. */
    uint32_t magic;
    unsigned char t;

    fcomplex_t bb_iq_samples[BB_FRAME_IQ_SAMPLES_COUNT];

} bb_frame_t;
int t = sizeof(bb_frame_t);

t:
        .long   4101                            # 0x1005
        .size   t, 4


it might be the case clang does not implement the ABI ....

[Bug c++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #5)
> I don't know whether clang allows packing non-PODs, or just doesn't ever
> warn for them, or has a special case for std::complex, or does something
> smarter like not warn for types that have no padding bytes anyway (so that
> packing them would be a no-op).

clang allows packing of non-PODs:
```
class f
{
public:
  f();
private:
  int t[2];
};

typedef struct __attribute__((__packed__))
{
    /** Every frame starts with BB_FRAME_MAGIC. */
    unsigned magic;
    unsigned char t;

    f bb_iq_samples[100];

} bb_frame_t;
```

[Bug c++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=13983,
                   |                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=17519

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
r0-51814-ge0d1297c4320ae added the warning and started to ignore the packed
(back in 2003).

[Bug c++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://gcc.gnu.org/pipermail/gcc-patches/2003-July/110087.html

This was done on purpose. GCC also rejects References too while clang accepts
that (but I am not sure it gives the correct code for references of packed
either which is why GCC started to reject it).

[Bug c++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[ruilvo at ua dot pt]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

--- Comment #10 from Rui Oliveira <ruilvo at ua dot pt> ---
So my options are to create like a placeholder, say 

```c
typedef struct __attribute__((__packed__)) // Packed isn't really necessary
here I think?
{
    float re, im;
} ph_fcomplex_t

```

To silence the warning and get packing to work, and trust
[complex.numbers.general] for a reinterpret_cast into std::complex<float> I
guess.

[Bug c++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Rui Oliveira from comment #10)
> So my options are to create like a placeholder, say 
> 
> ```c
> typedef struct __attribute__((__packed__)) // Packed isn't really necessary
> here I think?
> {
>     float re, im;
> } ph_fcomplex_t
> 
> ```
> 
> To silence the warning and get packing to work, and trust
> [complex.numbers.general] for a reinterpret_cast into std::complex<float> I
> guess.

No, if you have the packed ph_fcomplex_t not aligned at alignof (float), you
need
to copy it to a properly aligned variable before trying to reinterpret_cast it.

[Bug c++/108342] std::complex: ignoring packed attribute because of unpacked non-POD field

[ruilvo at ua dot pt]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108342

--- Comment #12 from Rui Oliveira <ruilvo at ua dot pt> ---
(In reply to Jakub Jelinek from comment #11)

> No, if you have the packed ph_fcomplex_t not aligned at alignof (float), you
> need
> to copy it to a properly aligned variable before trying to reinterpret_cast
> it.

Some `if constexpr` comparing of the remainder between
alignof(std::complex<float>) and (alignof(bb_frame_t) +
offsetof(bb_iq_samples)) could perhaps make one avoid that. But that's just a
side idea to think of.

Main point is, the code is de-serializing a serial stream. I do not expect to
find the "magic" word at the right aligment for `bb_frame_t` anyway, so
generous memcpy'ing to properly aligned variables will be required anyway.