Page MenuHomeFreeBSD

Declare packed struct ata_params as 2-byte-aligned
ClosedPublic

Authored by rlibby on Dec 20 2019, 6:49 AM.

Details

Summary

This avoids gcc9 warning about unaligned access to the structure when
casting to uint16_t pointer type.

Test Plan

make CROSS_TOOLCHAIN=amd64-gcc9 buildworld

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

It is actually properly aligned. Let's not do silly crap like this. Let's say it really is properly aligned.

In D22888#501055, @imp wrote:

It is actually properly aligned. Let's not do silly crap like this. Let's say it really is properly aligned.

It is indeed properly aligned, and gcc doesn't know it. How would you prefer to silence the warning? I felt it was just as much effort to do it in the Makefile.

A non-intrusive Makefile solution could be to lower camcontrol's WARNS to 3, either unconditionally or conditional on gcc.

For more background, this is to get gcc9 working. This is the only instance of Waddress-of-packed-variable in the tree for a user program that uses WARNS > 3. We suppress it completely for the kernel (D22876) and for programs using WARNS <= 3 (D22889).

I am open to any solution.

In D22888#501055, @imp wrote:

It is actually properly aligned. Let's not do silly crap like this. Let's say it really is properly aligned.

It is indeed properly aligned, and gcc doesn't know it. How would you prefer to silence the warning? I felt it was just as much effort to do it in the Makefile.

I'd prefer to mark the buffer as properly aligned in the code.

iff --git a/sbin/camcontrol/camcontrol.c b/sbin/camcontrol/camcontrol.c
index 4266c35ee03..79ab12e196b 100644
--- a/sbin/camcontrol/camcontrol.c
+++ b/sbin/camcontrol/camcontrol.c
@@ -2355,7 +2355,7 @@ ataidentify(struct cam_device *device, int retry_count, int timeout)
        if (arglist & CAM_ARG_VERBOSE) {
                printf("%s%d: Raw identify data:\n",
                    device->device_name, device->dev_unit_num);
-               dump_data((void*)ident_buf, sizeof(struct ata_params));
+               dump_data((uint16_t *)ident_buf, sizeof(struct ata_params));
        }

        if (ident_buf->support.command1 & ATA_SUPPORT_PROTECTED) {
diff --git a/sys/sys/ata.h b/sys/sys/ata.h
index 22edb5573d1..82d180b5491 100644
--- a/sys/sys/ata.h
+++ b/sys/sys/ata.h
@@ -311,7 +311,7 @@ struct ata_params {
 /*223*/ u_int16_t       transport_minor;
        u_int16_t       reserved224[31];
 /*255*/ u_int16_t       integrity;
-} __packed;
+} __packed __aligned(2);

 /* ATA Dataset Management */
 #define ATA_DSM_BLK_SIZE       512

I think is the right fix.

Ah. Yes. I'll check if that builds. Thanks.

Ah. Yes. I'll check if that builds. Thanks.

It builds for me with clang 9.0 :)

So it worked! Yippy! Of course I'd love you to commit this :)

This revision is now accepted and ready to land.Dec 20 2019, 7:58 PM
rlibby retitled this revision from camcontrol: avoid gcc9 Waddress-of-packed-member to Declare packed struct ata_params as 2-byte-aligned.Dec 20 2019, 7:59 PM
rlibby edited the summary of this revision. (Show Details)
In D22888#501195, @imp wrote:

So it worked! Yippy! Of course I'd love you to commit this :)

Yep. Okay, will do.

I don't know the commit log etiquette here, committing your suggestion, will this be okay?

Submitted by:	imp
Reviewed by:	imp
Sponsored by:	Dell EMC Isilon
Differential Revision:	https://reviews.freebsd.org/D22888
This revision was automatically updated to reflect the committed changes.