-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
toolchain: gcc: Add compiler barrier at the end of UNALIGNED_PUT() #8251
toolchain: gcc: Add compiler barrier at the end of UNALIGNED_PUT() #8251
Conversation
@therealprof, @mazhenke: FYI. |
Codecov Report
@@ Coverage Diff @@
## master #8251 +/- ##
==========================================
- Coverage 64.54% 64.53% -0.01%
==========================================
Files 420 420
Lines 40117 40117
Branches 6762 6762
==========================================
- Hits 25892 25891 -1
Misses 11108 11108
- Partials 3117 3118 +1
Continue to review full report at Codecov.
|
@pfalcon curious how you figured out a compiler_barrier was the thing to do. |
@galak, @mazhenke actually did, I guess I should have given him credit in the commit message. He suggested it in #6307 (comment) , and I googled it up as indeed suggested way to deal with related problems: #6307 (comment) . It's somewhat stronger than we need, but as we deal with apparent compiler bug, there's little choice, e.g. volatile would be even worse (didn't try if it helps though). |
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.
If this is happening, that's a compiler bug. A packed struct is by definition allowed to be misaligned.
Can you predicate this on the known-bad compiler/version so we don't take the performance hit everywhere? A memory clobber is very expensive (in the general case it means the compiler needs to re-fill every register it's populated).
Actually thinking about this, I think this might be our brain damage. A field in a packed struct is allowed to be missaligned, and the compiler is required to generate correct code to load it. I think the struct itself isn't subject to that rule and the compiler is free to assume it's naturally aligned. Maybe. If we want to do this portably we probably need to be smarter about it, and maybe make these macros part of the arch layer. |
Thinking more.... we can save the use of this trick by putting a single char in front of the __v element in the struct to force 1-byte alignment. Then the compiler is required to generate code for arbitrary alignment. Might work without the need for a memory clobber. |
@andyross, you might be interested to read comments on the original issue, e.g. I followed just the same stream of thought yesterday: #6307 (comment) |
Maybe try something like this? Note that it adds the char, offsets the pointer. It also removes a senseless typeof in a cast expression as void* works fine for that.
|
Linking directly to the pertinent comments if you don't feel like reading all of them: #6307 (comment) |
Hah, OK. If it's a bug it's a bug. I'm happy if you just #if it to ARM + GCC 7/8. |
I can do that, but that doesn't guarantee that another arch won't hit it. |
I 100% guarantee x86 won't hit it. :) |
What about xtensa or arc, who'll debug them? ;-) No problem, I'll do the #if. |
if its a bug we should file a bug w/gcc arm toolchain: |
@galak :
I guess it would need to go directly to gcc (maybe via hops, yeah). I can try to do it in Le Mans ;-). |
compiler_barrier() is itself defined down in this file. Without adding it, newer versions of GCC (7+) for ARM Cortex-M may mistakenly coalesce multiple strb/strh/str (store byte/half-word/word) instructions, which support unaligned access on some sub-architectures (Cortex-M3 and higher, but not on Cortex-M0), into strd (store double), which doesn't support unaligned access. Fixes: zephyrproject-rtos#6307 Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
f05dbdf
to
35fc5f0
Compare
Done. |
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.
Tempted to whine that it would have been cleaner to #define a BARRIER_WORKAROUND() to put in there that could have been an empty macro on other platforms and saved a bunch of lines in the patch. But style arguments shouldn't keep crash fixes out of the tree.
compiler_barrier() is itself defined down in this file. Without
adding it, newer versions of GCC (7+) for ARM Cortex-M may mistakenly
coalesce multiple strb/strh/str (store byte/half-word/word)
instructions, which support unaligned access on some
sub-architectures (Cortex-M3 and higher, but not on Cortex-M0),
into strd (store double), which doesn't support unaligned access.
Fixes: #6307
Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org