Bad structure padding in FreeBSD's nvme.h

[p-kraszewski]

Input C/C++ Header

Used on FreeBSD MaxBSD-2 12.0-RELEASE-p4 FreeBSD 12.0-RELEASE-p4 GENERIC amd64

File mycam.h:

#include <fcntl.h>
#include <stdio.h>
#include <sys/stat.h>
#include <cam/cam.h>
#include <cam/cam_ccb.h>
#include <camlib.h>

Structure causing trouble here is nvme_health_information_page from /usr/include/dev/nvme/nvme.h.

struct nvme_health_information_page {
 
        uint8_t                 critical_warning;
        uint16_t                temperature;
        uint8_t                 available_spare;
        uint8_t                 available_spare_threshold;
        uint8_t                 percentage_used;
 
        uint8_t                 reserved[26];
 
        /*
         * Note that the following are 128-bit values, but are
         *  defined as an array of 2 64-bit values.
         */
        /* Data Units Read is always in 512-byte units. */
        uint64_t                data_units_read[2];
        /* Data Units Written is always in 512-byte units. */
        uint64_t                data_units_written[2];
        /* For NVM command set, this includes Compare commands. */
        uint64_t                host_read_commands[2];
        uint64_t                host_write_commands[2];
        /* Controller Busy Time is reported in minutes. */
        uint64_t                controller_busy_time[2];
        uint64_t                power_cycles[2];
        uint64_t                power_on_hours[2];
        uint64_t                unsafe_shutdowns[2];
        uint64_t                media_errors[2];
        uint64_t                num_error_info_log_entries[2];
        uint32_t                warning_temp_time;
        uint32_t                error_temp_time;
        uint16_t                temp_sensor[8];

        uint8_t                 reserved2[296];
} __packed __aligned(4);

_Static_assert(sizeof(struct nvme_health_information_page) == 512, "bad size for nvme_health_information_page");

Yes, I know a sequence of uint8_t-uint16_t-uint8_t is bad karma, but this is NVMe structure beyond negotiation :)

Bindgen Invocation

bindgen  --verbose --no-rustfmt-bindings \
 -o camlib_raw.rs \
 --opaque-type nvme_registers \
 --opaque-type cap_lo_register \
 --opaque-type cap_hi_register \
 --opaque-type csts_register \
 --opaque-type aqa_register \
 --opaque-type cc_register \
 --opaque-type __max_align_t \
  mycam.h

Actual Results

test disk::low_level::camlib::bindgen_test_layout_nvme_health_information_page ... FAILED

failures:

---- disk::low_level::camlib::bindgen_test_layout_nvme_health_information_page stdout ----
thread 'disk::low_level::camlib::bindgen_test_layout_nvme_health_information_page' panicked at 'assertion failed: `(left == right)`
  left: `516`,
 right: `512`: Size of: nvme_health_information_page', src/disk/low_level/bind/camlib_raw.rs:35508:5

and/or

#[repr(C, packed(4))]
#[derive(Copy, Clone)]
pub struct nvme_health_information_page {
    pub critical_warning:           u8,
    pub temperature:                u16,
    pub available_spare:            u8,
    pub available_spare_threshold:  u8,
    pub percentage_used:            u8,
    pub reserved:                   [u8; 26usize],
    pub data_units_read:            [u64; 2usize],
    pub data_units_written:         [u64; 2usize],
    pub host_read_commands:         [u64; 2usize],
    pub host_write_commands:        [u64; 2usize],
    pub controller_busy_time:       [u64; 2usize],
    pub power_cycles:               [u64; 2usize],
    pub power_on_hours:             [u64; 2usize],
    pub unsafe_shutdowns:           [u64; 2usize],
    pub media_errors:               [u64; 2usize],
    pub num_error_info_log_entries: [u64; 2usize],
    pub warning_temp_time:          u32,
    pub error_temp_time:            u32,
    pub temp_sensor:                [u16; 8usize],
    pub reserved2:                  [u8; 296usize],
}

Expected Results

test disk::low_level::camlib::bindgen_test_layout_nvme_health_information_page ... ok

Insight

Everything tests out properly, when you replace #[repr(C, packed(4))] with #[repr(C, packed)] and correct alignment test accordingly. The true solution would be #[repr(C, packed, align(4))], but Rust explicitly forbids that:

error[E0587]: type has conflicting packed and align representation hints

I think __packed __aligned(4) in C code definitely shouldn't be treated as packed(4) in Rust which has a totally different meaning. __packed works elementwise and __aligned(4) works structure-wise (that is the whole structure is 32-bit aligned, not individual elements). On the other hand, what is the sense in __aligned(4) in packed structure of unaligned values?

[emilio]

Yes, this is a known-ish issue (see #1538). The packed(4) thing is definitely a bug, but last time I looked at fixing I couldn't make it work without breaking the #pragma pack support (since clang doesn't properly expose them :/), so I sent an llvm patch to expose aligned().

With https://reviews.llvm.org/rC356062 we can eventually generate the right thing (since we could detect whether the alignment comes from the aligned() attr or from #pragma), but not sure if it made it to a clang release yet, I suspect not... Still if you want to add support for it (we can just fall back to the current behavior if that function isn't found) it'd be greatly appreciated, and I'm happy to mentor.

Does #[repr(C, packed)] really work in this case? It'd give you the right size, but the wrong alignment, wouldn't it?

The obvious fix is making rust support both packed and aligned attributes, IMO, but if you want to give it a shot, grepping for repr_packed should point you to the right locations.

[p-kraszewski]

Yes, this is a known-ish issue (see #1538).

Seems precisely the case. However, on properly designed structures it should not manifest. Were it 8-8-16 or 16-8-8 sequence, everything would have passed.

Does #[repr(C, packed)] really work in this case? It'd give you the right size, but the wrong alignment, wouldn't it?

Yes, however - as you suspected - I had to disable alignment test. Ultimately, I removed __aligned(4) from source code and regenerated binding and now it compiles correctly. I've pinged FreeBSD guys if alignment pragma is really necessary in this particular (well, slightly malformed) structure. I also intend to harass some compiler gurus on Quora.

As a side-note - I don't actually use nvme layer, it is just side-compiled along cam subsystem I need. So as long as resulting structure is exactly a sector long and its tests pass, I'm fine with it - it is never used in my code.

Additionally, I'm not 100% sure, but when I generated binding last time (some 1-2 years ago), it did pass selftests - either due to older Rust compiler or older bindgen. The structure occurred as-is over 4 years ago, so it definitely was at current state.

On the second thoughts - as I don't use it, I probably should --opaque-type it and leave source as-is ;)

[emilio]

Additionally, I'm not 100% sure, but when I generated binding last time (some 1-2 years ago), it did pass selftests - either due to older Rust compiler or older bindgen. The structure occurred as-is over 4 years ago, so it definitely was at current state.

Right, if we don't support repr(packed(N)), we just treat it as opaque, so...

On the second thoughts - as I don't use it, I probably should --opaque-type it and leave source as-is ;)

... this sounds probably wise :-)

I'll close this as a dupe of #1538, thanks for the report!

[p-kraszewski]

I reported this nuance also to FreeBSD folk as bug 237940. Report got a very positive reaction:

Tl;dr: I think we can drop the __aligned(4) attribute now, and probably on the other structures as well.

So it seems the problem was attacked from both sides and the world just go a bit better. Well, at least for programmers. OK, some programmers.

Have a nice weekend.