You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In armv7-m/arm_cache.c (potentially other arch's arm_cache.c files as well, haven't looked), up_disable_dcache() reads some loop variables from memory, disables d cache, then cleans and invalidates the cache in a loop. When the cache had been configured to operate in WRITE_BACK mode, I am seeing that these variables:
ccsidr=getreg32(NVIC_CCSIDR);
sets=CCSIDR_SETS(ccsidr); /* (Number of sets) - 1 */sshift=CCSIDR_LSSHIFT(ccsidr) +4; /* log2(cache-line-size-in-bytes) */ways=CCSIDR_WAYS(ccsidr); /* (Number of ways) - 1 */
, which are set only once before the do-while loops, have their values changed mid-loop from ways = 3, sets = 255 on the imxrt-1064, to gigantic large numbers like 1 billion, causing the loop to execute incorrectly and causing a crash. This behavior is consistent and reproducible.
I had to look at the NXP-provided code for disabling dcache to help understand the error:
Anyway, I modified nuttx's up_dcache_disable() to declare every local variable as volatile, which has the effects of disabling compiler optimizations such as out-of-order execution for that variable. For what it's worth, I was compiling on gcc with -Os optimization level. From cppreference on volatile:
Every access (both read and write) made through an lvalue expression of volatile-qualified type is considered an observable side effect for the purpose of optimization and is evaluated strictly according to the rules of the abstract machine (that is, all writes are completed at some time before the next sequence point). This means that within a single thread of execution, a volatile access cannot be optimized out or reordered relative to another visible side effect that is separated by a sequence point from the volatile access.
This change fixed the bug and stopped my app from crashing. Maybe someone more familiar with cache and c/cpp language can better explain why register and volatile worked? If it's just about preventing reordering of executions by the compiler then maybe instead of volatile, some ARM_ISB() or ARM_DMB()'s would work as well? Haven't tested but just wanted to let you guys know of this issue and that it may be present in other arm arch files.
The text was updated successfully, but these errors were encountered:
Hi @danielappiagyei-bc you said it is easy to reproduce, do you have a simple example? Maybe it could be included in the testing ostest to catch this kind of issue.
Maybe @davids5 or @patacongo can have some ideas why register/volatile prevents the issue from happening.
In armv7-m/arm_cache.c (potentially other arch's arm_cache.c files as well, haven't looked),
up_disable_dcache()
reads some loop variables from memory, disables d cache, then cleans and invalidates the cache in a loop. When the cache had been configured to operate in WRITE_BACK mode, I am seeing that these variables:, which are set only once before the do-while loops, have their values changed mid-loop from
ways = 3
,sets = 255
on the imxrt-1064, to gigantic large numbers like 1 billion, causing the loop to execute incorrectly and causing a crash. This behavior is consistent and reproducible.I had to look at the NXP-provided code for disabling dcache to help understand the error:
They use the register keyword when declaring the function's local variables to somehow prevent the invalidation and cleaning of dcache from mangling their values. (My understanding is that the compiler isn't guaranteed to put the value in a register, it's just a hint which modern compilers often ignore since they're better at optimization. The keyword is also deprecated in c++.).
Anyway, I modified nuttx's
up_dcache_disable()
to declare every local variable as volatile, which has the effects of disabling compiler optimizations such as out-of-order execution for that variable. For what it's worth, I was compiling on gcc with-Os
optimization level. From cppreference onvolatile
:This change fixed the bug and stopped my app from crashing. Maybe someone more familiar with cache and c/cpp language can better explain why
register
andvolatile
worked? If it's just about preventing reordering of executions by the compiler then maybe instead ofvolatile
, someARM_ISB()
orARM_DMB()
's would work as well? Haven't tested but just wanted to let you guys know of this issue and that it may be present in other arm arch files.The text was updated successfully, but these errors were encountered: