-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Wrong alignement for function pointers (callbacks) causes CPU fault on Coretex-M4 #17
Comments
Hi, Thank you for reporting the issues. I will investigate that. PS. But why 8 and not 4 ? |
What if to play with CCR.STKALIGN=1 in your code? Is it the issue with C++ classes only? What about C-style API? Just checked the code on Cortex-A53, no issues with alignment. Another idea is to add |
…much code in FreeRTOS sources has no alignment for callbacks in Cortex-M implementations #17
Thanks for quick reply. I use Zephyr RTOS and it has already set 8 bytes stack alignment (because MPU is used for stack overflow protection). I did not try C API yet. I also tried packed structures and it works but it makes access to their fields quite expensive (comparing to unpacked/aligned structures). IMHO if code is doing pointer arithemetic and then code uses such pointer as an addres of structure (i.e. _init.buf assignment) then it is required that such (calulated) address is properly aligned. Unfortunately in 99.99% cases you will get proper results but i.e. compiler version change can trigger unaligned access. |
Can you capture the compiler options in both cases?
Almost all code in C / C++ does pointer arithmetic, and uses such pointers. The raw pointers are the basic of C. And doing manual alignment every time you use the pointer would be real headache for all developers. But that's not true. Of course, it is possible to make quick fix with PS. On ARM website, I have found 2 compiler options PPS. And I'm thinking if PPPS. uintptr_t m_data[(S + sizeof(uintptr_t ) - 1) / sizeof(uintptr_t )] |
You are right - STM32WB55 is obviously Cortex-M4. I attached compiler command line for both platforms. I also found something similiar problem described on stackoverflow: I did 8 bytes alignment in example because I also tried it on x86_64 and there is 8 bytes alignement for some pointers. At least on my platform |
I added some alignment implementation on the master branch. |
Hi, I've tested revision 46993c8, issue is still visible. What I noticed is that calling setWindowSize function with argument different than 4 will cause HF. Disassembly shows that HF is caused by STRD instruction. Pointer ptr value that is problematic came from calculation in tiny_fd_init() before call of hdlc_ll_init(). |
Hi, Ok, what we have:
And here is the link to documntation: https://developer.arm.com/documentation/100748/0609/writing-optimized-code/packing-data-structures
But it does almost nothing, just sets member field of the class. How it can fail? void setWindowSize(uint8_t window)
{
m_window = window;
}
Currently alignment is set to 4 bytes for Cortex-M4: #if defined(__TARGET_CPU_CORTEX_M0) || defined(__TARGET_CPU_CORTEX_M0_) || defined(__ARM_ARCH_6M__) || \
defined(__TARGET_CPU_CORTEX_M3) || defined(__TARGET_CPU_CORTEX_M4) || defined(__ARM_ARCH_7EM__) || \
defined(__ARM_ARCH_7M__)
#define TINY_ALIGNED_PTR TINY_ALIGNED(sizeof(uintptr_t))
#else
#define TINY_ALIGNED_PTR TINY_ALIGNED(sizeof(uintptr_t))
#endif But you can change that to 8 bytes by replacing
|
It seems that last commit fixed issue, at least I'm not able to reproduce it. Let's wait for final confirmation from @r2r0 |
HI @Zzzwierzak That's nice to hear that. I added alignment checks for the provided buffer to hdlc and fd initialization code. However, from the API standpoint maybe a better way is to remove so strict requirements for the user buffer, and automatically allocate a aligned space for the structures inside the buffer, as @r2r0 mentioned in initial post. |
Hi @lexus2k Thanks for your support - present version works well in our application. In fact alignment requirements for user supplied bufer can be sometimes problematic. Probably the definition of TINY_ALIGN_STRUCT_VALUE can be unified to sizeof(uintptr_t) - I used in my sample hardcoded value of 8 only for quick test on x86 platform but sizeof(uintptr_t) seems to be much more elegant solution. I wonder if allocation in FdD::FdD should also use TINY_ALIGN_STRUCT_VALUE (istead of sizeof(uintptr_t)). |
HI
You're absolutely correct. I've just fixed that in last commit.
Ok, I will remove strict verififcations from HDLC and FD.
Can you confirm that |
Yes, I confirm that sizeof(uintptr_t) works for me. |
@r2r0 Thank you very much. |
I'm closing the issue for now, as the problem is considered to be fixed. |
I got usage fault beacuse of unaligned memory on STM32WB55 (Cortex-M4) when accessing structure fields holding function pointers (callbacks) i.e. function hdlc_ll_reset is failing at line:
handle->rx.state = hdlc_ll_read_start;
.For confirmation I added (dirty hack) alignment to memory pointers used to initialize buf field in hdlc_ll_init_t and such change allows code to execute properly:
TinyProtocolFd.h:301
TinyProtocolFd.h:317
tiny_fd.c:584:
Compiler GCC 10.2.0
The text was updated successfully, but these errors were encountered: