New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: Fix undefined behaviour in C11 atomics #11528
Conversation
IMO this should do it. Did elf_diff show any differences? |
I checked for the Nucleo-F767zi with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Thanks!
This might need a second ACK, too. @SemjonKerner? ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix as disscussed. You could fix the indentions, but feel free to squash right away.
ACK
Casting pointers to volatile memory to pointers to regular memory is permitted, but using those pointers to access the memory results in undefined behavior. This commit changes the casts to no longer drop the volatile qualifier. References: https://en.cppreference.com/w/c/language/volatile
unsigned int mask = irq_disable(); \ | ||
(void)memmodel; \ | ||
I##n tmp = *(volatile I##n *)ptr; \ | ||
*(volatile I##n *)ptr = prefixop(tmp op val); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird, github still shows this line's last backslash one off, but with a direct checkout, it is alright?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my browser (Firefox) they look all aligned.
I also changed the column of the backslashes as requested to be 4 * n
, and not 4 * n + 1
. So merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in github now. You might F5 on this, Kaspar^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I F5'ed the shit out of this, still shows the missing space. Anyhow, it also doesn't let me upload the screenshot. Anyways, directly checking this out looks fine, so let's go!
Contribution description
Casting pointers to
volatile
memory to pointers to regular memory is permitted, but using those pointers to access the memory results in undefined behavior. This PR changes the casts to no longer drop the volatile qualifier.References: https://en.cppreference.com/w/c/language/volatile
Testing procedure
Compile some applications before and after this PR. The output should not differ.
Issues/PRs references
#11514