Changeset 590cb6d2 in mainline

Timestamp:

2023-10-22T15:53:32Z (14 months ago)

Author:

Jiří Zárevúcky <zarevucky.jiri@…>

Branches:

master, ticket/834-toolchain-update, topic/msim-upgrade, topic/simplify-dev-export

Children:

71b4444, abb70fc3

Parents:

f4a42661

git-author:

Jiří Zárevúcky <zarevucky.jiri@…> (2023-10-22 15:39:53)

git-committer:

Jiří Zárevúcky <zarevucky.jiri@…> (2023-10-22 15:53:32)

Message:

Remove some inappropriate uses of attribute((packed))

attribute((packed)) means "ignore all alignment requirements
on members of this structure". This is useful if one needs to
map onto a structure in memory that has misaligned fields on purpose,
but those cases are extremely rare.

The side effect of the attribute is that taking a pointer to any
field longer than a single byte, and dereferencing that pointer,
is unsound and may crash the program on architectures that care
about memory alignment when reading/writing multibyte values.

Newer GCC versions can detect some of those unsafe cases and
produce a warning for it. This commit only removes those cases.

However, most, if not all, uses of ((packed)) in HelenOS
are unnecessary and a product of misunderstanding what the
attribute actually does. A common misconception is that it is
needed to avoid compiler adding arbitrary padding into the
structure, but that is simply not true. There is exactly one
correct memory layout for any C structure, because there must
be one layout for binary interoperability to exist and the one
everyone uses (except perhaps some goblin who just wants to break
things for fun) is the trivial best layout possible with given
constraints.

Location:

uspace

Files:

• drv/bus/usb/xhci/hw_struct/common.h (modified) (1 diff)
• drv/bus/usb/xhci/hw_struct/context.h (modified) (9 diffs)
• lib/ext4/include/ext4/types.h (modified) (4 diffs)

uspace/drv/bus/usb/xhci/hw_struct/common.h

@@ -53 +53 @@
  * 4 bytes, little-endian.
  */
-typedef ioport32_t xhci_dword_t __attribute__((aligned(4)));
+typedef ioport32_t xhci_dword_t;
 
 /**
  * 8 bytes, little-endian.
  */
-typedef volatile uint64_t xhci_qword_t __attribute__((aligned(8)));
+typedef volatile uint64_t xhci_qword_t;
 
 #define XHCI_DWORD_EXTRACT(field, hi, lo) \

uspace/drv/bus/usb/xhci/hw_struct/context.h

@@ -53 +53 @@
         xhci_dword_t data3;
         xhci_dword_t reserved[3];
+} xhci_ep_ctx_t;
 
 #define XHCI_EP_COUNT 31
@@ -107 +108 @@
 #define XHCI_EP_MAX_ESIT_PAYLOAD_HI(ctx) XHCI_DWORD_EXTRACT((ctx).data[0], 31, 24)
 
-} __attribute__((packed)) xhci_ep_ctx_t;
-
 enum {
         EP_STATE_DISABLED = 0,
@@ -123 +122 @@
         xhci_dword_t data [4];
         xhci_dword_t reserved [4];
+} xhci_slot_ctx_t;
 
 #define XHCI_SLOT_ROUTE_STRING_SET(ctx, val) \
@@ -165 +165 @@
 #define XHCI_SLOT_STATE(ctx)            XHCI_DWORD_EXTRACT((ctx).data[3], 31, 27)
 
-} __attribute__((packed)) xhci_slot_ctx_t;
-
 enum {
         SLOT_STATE_DISABLED = 0,
@@ -213 +211 @@
 typedef struct xhci_stream_ctx {
         uint64_t data [2];
+} xhci_stream_ctx_t;
+
 #define XHCI_STREAM_DCS(ctx)     XHCI_QWORD_EXTRACT((ctx).data[0],  0, 0)
 #define XHCI_STREAM_SCT(ctx)     XHCI_QWORD_EXTRACT((ctx).data[0],  3, 1)
@@ -222 +222 @@
 #define XHCI_STREAM_DEQ_PTR_SET(ctx, val) \
         xhci_qword_set_bits(&(ctx).data[0], (val >> 4), 63, 4)
-} __attribute__((packed)) xhci_stream_ctx_t;
 
 /**
@@ -234 +233 @@
 typedef struct xhci_input_ctrl_ctx {
         uint32_t data [8];
+} __attribute__((packed)) xhci_input_ctrl_ctx_t;
+
 #define XHCI_INPUT_CTRL_CTX_DROP(ctx, idx) \
     XHCI_DWORD_EXTRACT((ctx).data[0], (idx), (idx))
@@ -252 +253 @@
 #define XHCI_INPUT_CTRL_CTX_ALTER_SETTING(ctx) \
     XHCI_DWORD_EXTRACT((ctx).data[7], 23, 16)
-} __attribute__((packed)) xhci_input_ctrl_ctx_t;
 
 /**
@@ -281 +281 @@
         uint8_t reserved;
         uint8_t ports [];
-} __attribute__((packed)) xhci_port_bandwidth_ctx_t;
+} xhci_port_bandwidth_ctx_t;
 
 #endif

uspace/lib/ext4/include/ext4/types.h

@@ -141 +141 @@
         uint32_t encrypt_algos;             /* Encrypt algorithm in use */
         uint32_t padding[105];              /* Padding to the end of the block */
-} __attribute__((packed)) ext4_superblock_t;
+} ext4_superblock_t;
 
 #define EXT4_GOOD_OLD_REV 0
@@ -335 +335 @@
                         uint32_t author;
                 } hurd2;
-        } __attribute__((packed)) osd2;
+        } osd2;
 
         uint16_t extra_isize;
@@ -345 +345 @@
         uint32_t crtime_extra;  /* Extra file creation time (nsec << 2 | epoch) */
         uint32_t version_hi;    /* High 32 bits for 64-bit version */
-} __attribute__((packed)) ext4_inode_t;
+} ext4_inode_t;
 
 #define EXT4_INODE_MODE_FIFO       0x1000
@@ -418 +418 @@
                 uint8_t name_length_high;  /* Higher 8 bits of name length */
                 uint8_t inode_type;        /* Type of referenced inode (in rev >= 0.5) */
-        } __attribute__((packed));
+        };
 
         uint8_t name[EXT4_DIRECTORY_FILENAME_LEN];  /* Entry name */
-} __attribute__((packed)) ext4_directory_entry_ll_t;
+} ext4_directory_entry_ll_t;
 
 typedef struct ext4_directory_iterator {