struct nvme_health_information_page { uint8_t critical_warning; uint16_t temperature; uint8_t available_spare; /* ... */ } __packed __aligned(4); has issues with alignment: `temperature` field can't be properly aligned on any current architecture which seems to defeat the `__aligned(4)` hint. What has bitten me today is that it confuses some code-generating tools, like Rust's bindgen. I already filed a bug to them, as they treat __packed __aligned(4) as __packed(4) - which has a *totally* different meaning - but it would be nice to have some BSD compiler guru took a glimpse at this particular structure and shed some light on whether __aligned(4) is OK here. And yes, I know this structure layout is not BSD's idea ¯\_(ツ)_/¯
Yes, the temperature field is inherently misaligned. The __aligned(4) was added in r240671 as a hack for printing out the nvme_controller_data and nvme_namespace_data pages as 32-bit hex values with a naive cast -- avoiding a -Wcast-align warning. At the time, nvme_health_information_page wasn't used by nvmecontrol -- it was probably just added at the same time because it was also __packed. Today we mostly print the page using a specialized printer, print_log_health, rather than any uint32 cast hex hack. print_hex -> print_dwords still uses this hack for the log page data if -x flag is specified, but print functions take the raw void* buffer pointer rather than a structure pointer, so we can probably drop the __aligned attribute. Additionally, get_log_buffer() allocates the buffer using plain malloc() with the log page size. This implicitly provides 4-byte alignment because FreeBSD only supports 32-bit and 64-bit architectures, and the log page is >4 bytes in size, so any standard C compliant malloc must provide 4-byte alignment on any platform FreeBSD runs on. It might be more clear to use aligned_alloc() to provide that guarantee explicitly instead, but I don't feel strongly about it. Tl;dr: I think we can drop the __aligned(4) attribute now, and probably on the other structures as well.
I'll look into this
OK. The code is correct. __packed means that the structure is tightly packed, with no space between elements. This is necessary for the layout of the structure. __aligned(4) means that the start of the structure is longword (32-bit) aligned. It's up to the compiler to figure out whether or not 'temperature' has proper alignment for the underlying machine to access with a 'natural' instruction or if it has to resort to 'bcopy' to get the data out of it. So if I assume that this pointer is 4-byte aligned, then the compiler will know which thing to use. All supported compilers get this right. IIRC, early versions of clang on some lessor supported architectures had s bug that's sense been fixed. So I'm closing with 'not a bug' in FreeBSD and bindgen needs some work.