On 04/17/19 20:31, Kinney, Michael D wrote:
> Laszlo,
>
> We should also be able to do a consistent fix without
> adding any new unions or macros:
>
> #define FFS_FILE_SIZE(FfsFileHeaderPtr) ((UINT32)( \
> (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[0] ) | \
> (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[1] << 8 ) | \
> (((EFI_FFS_FILE_HEADER *) (UINTN) FfsFileHeaderPtr)->Size[2] << 16 ) ))
>
> #define SECTION_SIZE(SectionHeaderPtr) ((UINT32)( \
> (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[0] )
> | \
> (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[1] << 8 )
> | \
> (((EFI_COMMON_SECTION_HEADER *) (UINTN) SectionHeaderPtr)->Size[2] << 16 )
> ))
I'll adopt this (see my other reply), with one small change: I think
I'll wrap "SectionHeaderPtr" in an extra set of parens.
My main reason for introducing the intermediate macros was that I wanted
to cast each UINT8 *individually* to UINT32, before applying the shift:
#define FFS_FILE_SIZE_ELEMENT(FfsFileHeaderPtr, Index) \
((UINT32) FFS_FILE_SIZE_ARRAY (FfsFileHeaderPtr)[(Index)])
#define FFS_FILE_SIZE(FfsFileHeaderPtr) \
((FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 0) << 0) | \
(FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 1) << 8) | \
(FFS_FILE_SIZE_ELEMENT ((FfsFileHeaderPtr), 2) << 16))
- Such casting is generally a good idea because UINT8 is promoted to
INT32 as part of the integer promotions, and I consider bit-shifting
signed integers bad hygiene (it carries a risk of undefined behavior).
- But then I felt that spelling out the same (UINT32) cast on every line
of the expanded / flattened version of FFS_FILE_SIZE would make the
replacement text simply too wide and hard to read. Hence the helper
macros.
Now, in this particular case however (i.e. producing 24-bit unsigned
integers in the end), you have convinced me that casting to UINT32, as
the outermost operation only, is safe enough. Because, we never shift an
INT32 to the left by more than 16 bits, and that INT32 (pre-shift) is in
[0, UINT8_MAX] inclusive (because it comes from Size[2]). So the result
is always representable in an INT32.
... I've now also checked that no invocation of FFS_FILE_SIZE or
SECTION_SIZE in edk2 passes an argument with side effects, so evaluating
the arguments in the new macros multiple times should be safe. I might
add a new comment about this restriction.
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#39291): https://edk2.groups.io/g/devel/message/39291
Mute This Topic: https://groups.io/mt/31070304/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
