PACKED attribute unnecessarily (IDFGH-14532)

[safocl]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

Some structures are defined with PACKED_ATTR, but have a valid offset within themselves (no padding):

esp-idf/components/hal/include/hal/key_mgr_types.h

Line 88 in c586527

typedef struct PACKED_ATTR {

esp-idf/components/hal/include/hal/key_mgr_types.h

Line 95 in c586527

typedef struct PACKED_ATTR {

esp-idf/components/hal/include/hal/key_mgr_types.h

Line 101 in c586527

typedef struct WORD_ALIGNED_ATTR PACKED_ATTR {

typedef struct PACKED_ATTR {
#define HUK_INFO_LEN 384
    uint8_t info[HUK_INFO_LEN];
    uint32_t crc;
} esp_key_mgr_huk_info_t;

384/4=96

typedef struct PACKED_ATTR {
#define KEY_INFO_LEN 64
    uint8_t info[KEY_INFO_LEN];
    uint32_t crc;
} esp_key_mgr_key_info_t;

64/4=16

typedef struct WORD_ALIGNED_ATTR PACKED_ATTR {
#define KEY_HUK_SECTOR_MAGIC 0xDEA5CE5A
    uint32_t magic;
    uint32_t version; // for backward compatibility
    uint8_t key_type;
    uint8_t reserved[15];
    esp_key_mgr_huk_info_t huk_info;
    esp_key_mgr_key_info_t key_info[2]; // at most 2 key info (XTS-512_1 and XTS-512_2), at least use 1
} esp_key_mgr_key_recovery_info_t;

(4+4+16)/4=6, (4+4+16+384+4)/4=103

guaranteed with no padding.

maybe remove the PACKED_ATTR?

[safocl]

it's same

esp-idf/components/esp_phy/esp32/include/phy_init_data.h

Line 47 in c586527

} __attribute__ ((packed)) phy_control_info_data_t;

struct contains only single-byte types, but the PACKED attribute is applied

esp-idf/components/bt/esp_ble_mesh/core/include/mesh/hci.h

Line 116 in c586527

} __attribute__((packed));

esp-idf/components/bt/esp_ble_mesh/core/include/mesh/hci.h

Line 122 in c586527

} __attribute__((packed));

[KaeLL]

Just out of morbid curiosity, did you check generated asm for memory access to see how gcc handles it? I'd like to know if it honors or ignores the attribute in this case.

[igrr]

guaranteed with no padding.

The problem that if the structure ever changes, somebody might forget to think about padding. Preemptively adding the attribute for certain structures (typically the ones which have to be transmitted over wire or exchanged with hardware) prevents this mistake.

maybe remove the PACKED_ATTR?

Seems to me to be not necessary to remove it. Why do you suggest that?

[KaeLL]

Seems to me to be not necessary to remove it.

I'm not sure if that indirectly answers my question, which is itself the the only reasonable reason for removing the attribute. Yay or nay, @igrr ?

[igrr]

@KaeLL I don't understand your original comment, sorry. Could you clarify what exactly do you mean by "honors or ignores" in "I'd like to know if it honors or ignores the attribute in this case."

[KaeLL]

In the cases which the attribute is redundant due to the struct not needing any padding in practice, is GCC aware of this and ignores the attribute?

[safocl]

Just out of morbid curiosity, did you check generated asm for memory access to see how gcc handles it? I'd like to know if it honors or ignores the attribute in this case.

that shows what's the same?

[safocl]

The problem that if the structure ever changes, somebody might forget to think about padding.

then they'll have to think about alignment (access to packed memes) anyway

[AdityaHPatwardhan]

@safocl One more concern that was considered while adding this PACKED_ATTR attribute is that this structure is shared with the ROM code and the esp-idf sdk. If the structure ever changes due to padding by compiler (maybe in future some reserved bytes are used) and the compiler padding is missed at the time (which is easy to miss). Then it may create bigger issue in maintaining compatibility across multiple revisions of key manager.

@KaeLL, since we have defined the attribute, the compiler must always honour it. In this case as the packed struct would be same as the unpacked struct, the compiler won't make any changes. But we shouldn't remove the attribute due to this reasoning as it may be required in future.

I hope this resolves relevant comments on this issue, Please help to close the issue if no more concerns.
Thanks

[safocl]

Then it may create bigger issue in maintaining compatibility across multiple revisions of key manager.

however, developers will still have to carefully monitor such code, since access to unaligned memory is undefined behavior.
But I understand the reason for attributes -- OK.
I would probably suggest a portable and carefree way to translate to structure members as arrays instead of packed regular data.

I am closing this issue and hope that the above will not happen -- and we will all keep a close eye on this if possible -- thanks to everyone for the discussion.