diff options
| author | Jacob Garber <jgarber1@ualberta.ca> | 2019-07-31 13:43:31 -0600 |
|---|---|---|
| committer | Patrick Georgi <pgeorgi@google.com> | 2019-08-20 15:38:26 +0000 |
| commit | c43001eb14c0a417b4c93189b19fd9cb929684bc (patch) | |
| tree | 7605ba5132ad19221c4a8d408150317db1a603fe /src/vendorcode/amd | |
| parent | 1b7b7a36978c4294b00edc96ca7cd195f3f597a5 (diff) | |
| download | coreboot-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.c | 8 |
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); |