Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34783 )
Change subject: vc/amd/cimx/sb800: Remove old strict-aliasing workaround ......................................................................
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 --- M src/vendorcode/amd/cimx/sb800/MEMLIB.c 1 file changed, 2 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/src/vendorcode/amd/cimx/sb800/MEMLIB.c b/src/vendorcode/amd/cimx/sb800/MEMLIB.c index 5531c62..d9eb8ff 100644 --- a/src/vendorcode/amd/cimx/sb800/MEMLIB.c +++ b/src/vendorcode/amd/cimx/sb800/MEMLIB.c @@ -46,9 +46,7 @@ *((UINT8*)Value) = *((UINT8*) ((UINTN)Address)); break; case AccWidthUint16: - //*((UINT16*)Value) = *((UINT16*) ((UINTN)Address)); //gcc break strict-aliasing rules - *((UINT8*)Value) = *((UINT8*) ((UINTN)Address)); - *((UINT8*)Value + 1) = *((UINT8*)((UINTN)Address) + 1); + *((UINT16*)Value) = *((UINT16*) ((UINTN)Address)); break; case AccWidthUint32: *((UINT32*)Value) = *((UINT32*) ((UINTN)Address)); @@ -69,9 +67,7 @@ *((UINT8*) ((UINTN)Address)) = *((UINT8*)Value); break; case AccWidthUint16: - //*((UINT16*) ((UINTN)Address)) = *((UINT16*)Value); //gcc break strict-aliasing rules - *((UINT8*)((UINTN)Address)) = *((UINT8*)Value); - *((UINT8*)((UINTN)Address) + 1) = *((UINT8*)Value + 1); + *((UINT16*) ((UINTN)Address)) = *((UINT16*)Value); break; case AccWidthUint32: *((UINT32*) ((UINTN)Address)) = *((UINT32*)Value);