Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41154 )
Change subject: amdfwtool: Unpack struct `_bios_directory_entry` ......................................................................
amdfwtool: Unpack struct `_bios_directory_entry`
gcc (Debian 10-20200502-1) 10.0.1 20200502 (prerelease) [gcc-10 revision d128356d460:a9ed47dd155:0118d0397f94c307b76aa14abec99347a93da621] shows the warning below.
/coreboot/util/amdfwtool/amdfwtool.c: In function 'integrate_bios_firmwares': /dev/shm/coreboot/util/amdfwtool/amdfwtool.c:964:7: warning: taking address of packed member of 'struct _bios_directory_entry' may result in an unaligned pointer value [-Waddress-of-packed-member] 964 | &biosdir->entries[count].source, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /coreboot/util/amdfwtool/amdfwtool.c:965:7: warning: taking address of packed member of 'struct _bios_directory_entry' may result in an unaligned pointer value [-Waddress-of-packed-member] 965 | &biosdir->entries[count].size)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Change-Id: I375ffec589cca8fe9d07dbe908c79348e863aa13 Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M util/amdfwtool/amdfwtool.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/41154/1
diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 1ff86e6..c4acf81 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -484,7 +484,7 @@ uint32_t size; uint64_t source; uint64_t dest; -} __attribute__((packed)) bios_directory_entry; +} bios_directory_entry;
typedef struct _bios_directory_table { bios_directory_hdr header;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41154 )
Change subject: amdfwtool: Unpack struct `_bios_directory_entry` ......................................................................
Patch Set 1:
This is just build tested. I have no idea how to verify that the tool still works correctly.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41154 )
Change subject: amdfwtool: Unpack struct `_bios_directory_entry` ......................................................................
Patch Set 1: Code-Review-1
This might just break the tool. Also I wonder why those accesses aren't aligned; from counting the bits they should be aligned
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41154 )
Change subject: amdfwtool: Unpack struct `_bios_directory_entry` ......................................................................
Patch Set 1:
What does smell weird about that struct is that the 1 bit wide fields are signed integers while they should be unsigned integers
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41154 )
Change subject: amdfwtool: Unpack struct `_bios_directory_entry` ......................................................................
Patch Set 1:
I agree that unpacking could have a bad effect.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41154 )
Change subject: amdfwtool: Unpack struct `_bios_directory_entry` ......................................................................
Patch Set 1:
Patch Set 1:
What does smell weird about that struct is that the 1 bit wide fields are signed integers while they should be unsigned integers
Generally one should avoid bit-fields when having to match with external spec. AFAIR it is not dictated in the standard that the first declared bit-field will consume LSB position. I was under impression that bit-fields would allocate minimum size of int but apparently, with packed attribute at least, only uint8_t is consumed here.
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41154?usp=email )
Change subject: amdfwtool: Unpack struct `_bios_directory_entry` ......................................................................
Abandoned
-
Felix Held (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)