summaryrefslogtreecommitdiff
path: root/src/vendorcode/amd
diff options
context:
space:
mode:
authorJacob Garber <jgarber1@ualberta.ca>2019-07-31 13:43:31 -0600
committerPatrick Georgi <pgeorgi@google.com>2019-08-20 15:38:26 +0000
commitc43001eb14c0a417b4c93189b19fd9cb929684bc (patch)
tree7605ba5132ad19221c4a8d408150317db1a603fe /src/vendorcode/amd
parent1b7b7a36978c4294b00edc96ca7cd195f3f597a5 (diff)
downloadcoreboot-c43001eb14c0a417b4c93189b19fd9cb929684bc.tar.xz
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>
Diffstat (limited to 'src/vendorcode/amd')
-rw-r--r--src/vendorcode/amd/cimx/sb800/MEMLIB.c8
1 files changed, 2 insertions, 6 deletions
diff --git a/src/vendorcode/amd/cimx/sb800/MEMLIB.c b/src/vendorcode/amd/cimx/sb800/MEMLIB.c
index 5531c627d0..d9eb8fff38 100644
--- a/src/vendorcode/amd/cimx/sb800/MEMLIB.c
+++ b/src/vendorcode/amd/cimx/sb800/MEMLIB.c
@@ -46,9 +46,7 @@ ReadMEM (
*((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 @@ WriteMEM (
*((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);