From 590cb6d21c857534d940c4c0a165a9997826a0b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Z=C3=A1rev=C3=BAcky?= Date: Sun, 22 Oct 2023 17:39:53 +0200 Subject: [PATCH] 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. --- uspace/drv/bus/usb/xhci/hw_struct/common.h | 4 ++-- uspace/drv/bus/usb/xhci/hw_struct/context.h | 14 +++++++------- uspace/lib/ext4/include/ext4/types.h | 10 +++++----- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/uspace/drv/bus/usb/xhci/hw_struct/common.h b/uspace/drv/bus/usb/xhci/hw_struct/common.h index c997f773e9..3a019147c7 100644 --- a/uspace/drv/bus/usb/xhci/hw_struct/common.h +++ b/uspace/drv/bus/usb/xhci/hw_struct/common.h @@ -52,12 +52,12 @@ /** * 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) \ (BIT_RANGE_EXTRACT(uint32_t, hi, lo, xhci2host(32, field))) diff --git a/uspace/drv/bus/usb/xhci/hw_struct/context.h b/uspace/drv/bus/usb/xhci/hw_struct/context.h index 0db76305c6..09293d2c68 100644 --- a/uspace/drv/bus/usb/xhci/hw_struct/context.h +++ b/uspace/drv/bus/usb/xhci/hw_struct/context.h @@ -52,6 +52,7 @@ typedef struct xhci_endpoint_ctx { xhci_qword_t data2; xhci_dword_t data3; xhci_dword_t reserved[3]; +} xhci_ep_ctx_t; #define XHCI_EP_COUNT 31 @@ -106,8 +107,6 @@ typedef struct xhci_endpoint_ctx { #define XHCI_EP_MAX_ESIT_PAYLOAD_LO(ctx) XHCI_DWORD_EXTRACT((ctx).data3, 31, 16) #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, EP_STATE_RUNNING = 1, @@ -122,6 +121,7 @@ enum { typedef struct xhci_slot_ctx { xhci_dword_t data [4]; xhci_dword_t reserved [4]; +} xhci_slot_ctx_t; #define XHCI_SLOT_ROUTE_STRING_SET(ctx, val) \ xhci_dword_set_bits(&(ctx).data[0], (val & 0xFFFFF), 19, 0) @@ -164,8 +164,6 @@ typedef struct xhci_slot_ctx { #define XHCI_SLOT_DEVICE_ADDRESS(ctx) XHCI_DWORD_EXTRACT((ctx).data[3], 7, 0) #define XHCI_SLOT_STATE(ctx) XHCI_DWORD_EXTRACT((ctx).data[3], 31, 27) -} __attribute__((packed)) xhci_slot_ctx_t; - enum { SLOT_STATE_DISABLED = 0, SLOT_STATE_DEFAULT = 1, @@ -212,6 +210,8 @@ static inline char *xhci_device_ctx_to_charptr(const xhci_device_ctx_t *ctx) */ 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) #define XHCI_STREAM_DEQ_PTR(ctx) (XHCI_QWORD_EXTRACT((ctx).data[0], 63, 4) << 4) @@ -221,7 +221,6 @@ typedef struct xhci_stream_ctx { xhci_qword_set_bits(&(ctx).data[0], val, 3, 1) #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; /** * Input control context: section 6.2.5.1 @@ -233,6 +232,8 @@ typedef struct xhci_stream_ctx { */ 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)) @@ -251,7 +252,6 @@ typedef struct xhci_input_ctrl_ctx { XHCI_DWORD_EXTRACT((ctx).data[7], 15, 8) #define XHCI_INPUT_CTRL_CTX_ALTER_SETTING(ctx) \ XHCI_DWORD_EXTRACT((ctx).data[7], 23, 16) -} __attribute__((packed)) xhci_input_ctrl_ctx_t; /** * Input context: section 6.2.5 @@ -280,6 +280,6 @@ static inline char *xhci_input_ctx_to_charptr(const xhci_input_ctx_t *ctx) typedef struct xhci_port_bandwidth_ctx { uint8_t reserved; uint8_t ports []; -} __attribute__((packed)) xhci_port_bandwidth_ctx_t; +} xhci_port_bandwidth_ctx_t; #endif diff --git a/uspace/lib/ext4/include/ext4/types.h b/uspace/lib/ext4/include/ext4/types.h index f237c9a768..a1bbbc2461 100644 --- a/uspace/lib/ext4/include/ext4/types.h +++ b/uspace/lib/ext4/include/ext4/types.h @@ -140,7 +140,7 @@ typedef struct ext4_superblock { uint32_t backup_bgs[2]; /* Block groups containing superblock backups (if SPARSE_SUPER2) */ 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 #define EXT4_DYNAMIC_REV 1 @@ -334,7 +334,7 @@ typedef struct ext4_inode { uint16_t gid_high; uint32_t author; } hurd2; - } __attribute__((packed)) osd2; + } osd2; uint16_t extra_isize; uint16_t pad1; @@ -344,7 +344,7 @@ typedef struct ext4_inode { uint32_t crtime; /* File creation time */ 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 #define EXT4_INODE_MODE_CHARDEV 0x2000 @@ -417,10 +417,10 @@ typedef struct ext4_directory_entry_ll { union { 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 { ext4_inode_ref_t *inode_ref;