cpu/sam0_common: fix potential undefined result with sercom_id #12576
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aabadie
added
Platform: ARM
benpicco
reviewed
Oct 25, 2019
| return -1; | ||
| assert(ret < SERCOM_INST_NUM); | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
You could just return SERCOM_INST_NUM; here.
benpicco
reviewed
Oct 25, 2019
| #endif | ||
|
|
||
| return -1; | ||
| assert(ret < SERCOM_INST_NUM); |
There was a problem hiding this comment.
or assert(false) if you don't introduce a ret variable.
benpicco
reviewed
Oct 25, 2019
| @@ -342,40 +342,51 @@ void gpio_init_mux(gpio_t pin, gpio_mux_t mux); | |||
| */ | |||
| static inline int sercom_id(const void *sercom) | |||
| { | |||
| int ret = SERCOM_INST_NUM; | |||
There was a problem hiding this comment.
I think you don't need this.
There was a problem hiding this comment.
You are right. I also added the assert(false); before returning. Not that there's an increase of code size by 48 bytes with this PR.
There was a problem hiding this comment.
I would assume the increase in size is only with DEVHELP = 1
|
Looks good, please squash. |
Scan-build detected that sercom_id could return -1 and the value of this function is affected to uint8_t variables. Since these variables are used for shitfing bit in registers, this could lead to undefined behavior
aabadie
force-pushed
the
pr/cpu/sam0_scan_build_fix
branch
from
c663171 to
1c41618
Compare
Done! |
benpicco
approved these changes
Oct 27, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
As reported in #11852 there are tons of scan-build warning when building an application for a sam0 based boards.
Scan-build is detecting that sercom_id could return -1 and the value of this function is affected to uint8_t variables. Since these variables are used for shitfing bit in registers, this could lead to undefined results.
This PR is fixing that by using the SERCOM_INST_NUM macro defined in the CMSIS instead of -1. There's an assert added to check that the value returned is not SERCOM_INST_NUM.
This change also adds 16 extra bytes in ROM.
Testing procedure
Run
BOARD=samr21-xpro TOOLCHAIN=llvm make -C examples/hello-world/ scan-buildon master it raises tons of warning, with this PR the warning are gone
Trigger CI: run tests on Murdock and verify they work (on samr21-xpro which is sam0 based).
Issues/PRs references
Tick one item in #11852