GNUBoot
/
coreboot-kgpe-d16
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

vc/amd/cimx/sb800: Remove old strict-aliasing workaround 

C strict aliasing rules state that it is undefined behaviour to access
any pointer using another pointer of a different type (with several small
exceptions). Eg.

    uint64_t x = 3;
    uint16_t y = *((uint16_t *)&x);  // undefined behaviour

From an architectural point of view there is often nothing wrong with
pointer aliasing - the problem is that since it is undefined behaviour,
the compiler will often use this as a cop-out to perform unintended or
unsafe optimizations. The "safe" way to perfom the above assignment is
to cast the pointers to a uint8_t * first (which is allowed to alias
anything), and then work on a byte level:

    *((uint8_t *)&y) = *((uint8_t *)&x);
    *((uint8_t *)&y + 1) = *((uint8_t *)&x + 1);

Horribly ugly, but there you go. Anyway, in an attempt to follow these
strict aliasing rules, the ReadMEM() function in SB800 does the above
operation when reading a uint16_t. While perfectly fine, however, it
doesn't have to - all calls to ReadMEM() that read a uint16_t are passed
a uint16_t pointer, so there are no strict aliasing violations to worry
about (the WriteMEM() function is exactly similar). The problem is that
using this unnecessary workaround generates almost 50 false positive
warnings in Coverity. Rather than manually ignore them one-by-one, let's
just remove the workaround entirely. As a side note, this change makes
ReadMEM() and WriteMEM() now match their definitions in the SB900 code.

Change-Id: Ia7e3a1eff88b855a05b33c7dafba16ed23784e43
Signed-off-by: Jacob Garber <jgarber1@ualberta.ca>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34783
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Patrick Georgi <pgeorgi@google.com>
This commit is contained in:
Jacob Garber 2019-07-31 13:43:31 -06:00 committed by Patrick Georgi
parent 1b7b7a3697
commit c43001eb14
1 changed files with 2 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/vendorcode/amd/cimx/sb800/MEMLIB.c
View File

@ -46,9 +46,7 @@ ReadMEM (
*((UINT8*)Value) = *((UINT8*) ((UINTN)Address)); *((UINT8*)Value) = *((UINT8*) ((UINTN)Address));
break; break;
case AccWidthUint16: case AccWidthUint16:
//*((UINT16*)Value) = *((UINT16*) ((UINTN)Address)); //gcc break strict-aliasing rules *((UINT16*)Value) = *((UINT16*) ((UINTN)Address));
*((UINT8*)Value) = *((UINT8*) ((UINTN)Address));
*((UINT8*)Value + 1) = *((UINT8*)((UINTN)Address) + 1);
break; break;
case AccWidthUint32: case AccWidthUint32:
*((UINT32*)Value) = *((UINT32*) ((UINTN)Address)); *((UINT32*)Value) = *((UINT32*) ((UINTN)Address));
@ -69,9 +67,7 @@ WriteMEM (
*((UINT8*) ((UINTN)Address)) = *((UINT8*)Value); *((UINT8*) ((UINTN)Address)) = *((UINT8*)Value);
break; break;
case AccWidthUint16: case AccWidthUint16:
//*((UINT16*) ((UINTN)Address)) = *((UINT16*)Value); //gcc break strict-aliasing rules *((UINT16*) ((UINTN)Address)) = *((UINT16*)Value);
*((UINT8*)((UINTN)Address)) = *((UINT8*)Value);
*((UINT8*)((UINTN)Address) + 1) = *((UINT8*)Value + 1);
break; break;
case AccWidthUint32: case AccWidthUint32:
*((UINT32*) ((UINTN)Address)) = *((UINT32*)Value); *((UINT32*) ((UINTN)Address)) = *((UINT32*)Value);