Jacob Garber has uploaded this change for review. ( 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 --- M src/vendorcode/amd/cimx/sb800/MEMLIB.c 1 file changed, 2 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/34783/1
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);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34783 )
Change subject: vc/amd/cimx/sb800: Remove old strict-aliasing workaround ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34783/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34783/1//COMMIT_MSG@35 PS1, Line 35: ReadMEM() and WriteMEM() now match their definitions in the SB900 code. So older GCC gave an error?
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34783 )
Change subject: vc/amd/cimx/sb800: Remove old strict-aliasing workaround ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34783/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34783/1//COMMIT_MSG@35 PS1, Line 35: ReadMEM() and WriteMEM() now match their definitions in the SB900 code.
So older GCC gave an error?
I'm not sure. GCC does have -Wstrict-aliasing to try to warn about these sort of things, but that shouldn't have been a problem since these functions are compiled separately from where they are called (so GCC would have no way of knowing if they actually alias or not). The code was like this when it was checked into coreboot, so it was probably a workaround for some AMD issue back in ye olden days.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34783 )
Change subject: vc/amd/cimx/sb800: Remove old strict-aliasing workaround ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34783/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34783/1//COMMIT_MSG@35 PS1, Line 35: ReadMEM() and WriteMEM() now match their definitions in the SB900 code.
I'm not sure. […]
AGESA platforms build with -fno-strict-aliasing.
I should patch it, it's not required for coreboot proper at least, not sure about vendorcode/.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34783 )
Change subject: vc/amd/cimx/sb800: Remove old strict-aliasing workaround ......................................................................
Patch Set 1:
FYI: CB:34798 CB:34799
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34783 )
Change subject: vc/amd/cimx/sb800: Remove old strict-aliasing workaround ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/34783/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34783/1//COMMIT_MSG@35 PS1, Line 35: ReadMEM() and WriteMEM() now match their definitions in the SB900 code.
AGESA platforms build with -fno-strict-aliasing. […]
AGESA is built with GCC not trying to apply strict aliasing rules, SB900 never claimed to have an issue to begin with, so I guess this is good to go in.
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);
-
Jacob Garber (Code Review) -
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)