Change in coreboot[master]: arch/x86/include/arch/pci_io_cfg.h: Fix UndefinedBehavior sanitizer c...
Shawn C has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49076 )
Change subject: arch/x86/include/arch/pci_io_cfg.h: Fix UndefinedBehavior sanitizer complain ......................................................................
arch/x86/include/arch/pci_io_cfg.h: Fix UndefinedBehavior sanitizer complain
GCC build option "-fsanitize=shift" will complain: "runtime error: left shift of 1 by 31 places cannot be represented in type 'int'"
Signed-off-by: Shawn C citypw@hardenedlinux.org Change-Id: I6eeee1e92b616f68aa60eb74771912dd00ff7712 --- M src/arch/x86/include/arch/pci_io_cfg.h 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/49076/1
diff --git a/src/arch/x86/include/arch/pci_io_cfg.h b/src/arch/x86/include/arch/pci_io_cfg.h index 61dd106..5e9761e 100644 --- a/src/arch/x86/include/arch/pci_io_cfg.h +++ b/src/arch/x86/include/arch/pci_io_cfg.h @@ -10,7 +10,7 @@ static __always_inline uint32_t pci_io_encode_addr(pci_devfn_t dev, uint16_t reg) { - uint32_t addr = 1 << 31; + uint32_t addr = 1UL << 31;
addr |= dev >> 4; addr |= reg & 0xfc;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49076 )
Change subject: arch/x86/include/arch/pci_io_cfg.h: Fix UndefinedBehavior sanitizer complain ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49076 )
Change subject: arch/x86/include/arch/pci_io_cfg.h: Fix UndefinedBehavior sanitizer complain ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Hmmm, I remember some resistance towards these correct-by-definition but unnecessary-given-we-know-the-compiler changes. I guess we should have a broader discussion about it at some point.
Personally, I'm ok with it. Would prefer `1u`, though (lower case to be more distinct from the number part, and the `L` doesn't seem needed).
https://review.coreboot.org/c/coreboot/+/49076/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/49076/1//COMMIT_MSG@7 PS1, Line 7: arch/x86/include/arch/pci_io_cfg.h: Fix UndefinedBehavior sanitizer complain "Fix <tool> complaining" isn't a good description. Such changes sometimes turn out to be wrong even if they silence something. Better describe what is actually wrong, e.g.
arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift
(the prefix doesn't have to be a full path)
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Paul Menzel, Kyösti Mälkki,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49076
to look at the new patch set (#2).
Change subject: arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift ......................................................................
arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift
GCC build option "-fsanitize=shift" will complain: "runtime error: left shift of 1 by 31 places cannot be represented in type 'int'"
Signed-off-by: Shawn C citypw@hardenedlinux.org Change-Id: I6eeee1e92b616f68aa60eb74771912dd00ff7712 --- M src/arch/x86/include/arch/pci_io_cfg.h 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/49076/2
Shawn C has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49076 )
Change subject: arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift ......................................................................
Patch Set 2: Code-Review+1
Patch Set 1: Code-Review+1
(1 comment)
Hmmm, I remember some resistance towards these correct-by-definition but unnecessary-given-we-know-the-compiler changes. I guess we should have a broader discussion about it at some point.
Personally, I'm ok with it. Would prefer `1u`, though (lower case to be more distinct from the number part, and the `L` doesn't seem needed).
Thanks for reviewing. I updated the short description. I'll keep the code since both upper/lower-cases of "unsigned" symbols exists in the current coreboot code base. We can change it if we have an unify coding style.
Indeed we might need to discuss how are we process the errors produced by sanitizers( coreboot have two types: undefined behavior and address-specific. There are still a few compliants( not sure if its real bug now) from UBsan. I'll file the bug then.
Attention is currently required from: Shawn C. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49076 )
Change subject: arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift ......................................................................
Patch Set 2:
(1 comment)
File src/arch/x86/include/arch/pci_io_cfg.h:
https://review.coreboot.org/c/coreboot/+/49076/comment/091acdf3_ad7ecaf5 PS2, Line 13: 1UL `1u` is enough.
Attention is currently required from: Shawn C. Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Paul Menzel, Kyösti Mälkki,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49076
to look at the new patch set (#3).
Change subject: arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift ......................................................................
arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift
GCC build option "-fsanitize=shift" will complain: "runtime error: left shift of 1 by 31 places cannot be represented in type 'int'"
Signed-off-by: Shawn C citypw@hardenedlinux.org Change-Id: I6eeee1e92b616f68aa60eb74771912dd00ff7712 --- M src/arch/x86/include/arch/pci_io_cfg.h 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/49076/3
Attention is currently required from: Angel Pons. Shawn C has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49076 )
Change subject: arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift ......................................................................
Patch Set 3:
(2 comments)
Patchset:
PS3: Please review it again.
File src/arch/x86/include/arch/pci_io_cfg.h:
https://review.coreboot.org/c/coreboot/+/49076/comment/4bb8dd42_e654fff3 PS2, Line 13: 1UL
`1u` is enough.
done!
Attention is currently required from: Shawn C. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49076 )
Change subject: arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Patchset:
PS3: There's an ongoing discussion on the mailing list about this.
Attention is currently required from: Shawn C.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49076 )
Change subject: arch/x86/pci_io_cfg.h: Fix undefined behaviour in left-shift ......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3: Please see change-set https://review.coreboot.org/c/coreboot/+/51292.
-
Angel Pons (Code Review) -
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Shawn C (Code Review)