-
Notifications
You must be signed in to change notification settings - Fork 130
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
2023q1 release #995
2023q1 release #995
Conversation
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.
Thanks, I'll do some hardware testing now.
The test runs into HardFault handler here modm/src/unittest/reporter.cpp Line 42 in d1938eb
before executing the first test.
What...? |
I can reproduce this. It's not a stack overflow, the issue seems that the
|
Something is fundamentally broken here.
with
|
Single stepping through the I have an alternative solution that just removes the controller class and uses the reporter directly. But I'm still investigating the original problem. |
It is very suspicious that this is happening only on the Cortex-M0 device, which uses a special std::atomic for the static initialization guard. |
Could you try running the test with the |
Well… that works fine! So it has something to do with |
You were a minute faster, but I can confirm the observation 😆 |
The implementation of the static initialization guards is undefined behaviour.
It's not allowed to cast an The correct version would be
I don't think it will fix the original issue but we should change it anyway. |
Oh, I fixed that. It doesn't fix the behavior sadly. |
Oh no… The static guard initialization somehow initializes on every call… 😱 |
https://itanium-cxx-abi.github.io/cxx-abi/abi.html EDIT: you were faster 😆 |
I think I thought that the compiler would check the variable first, and only call acquire if it was zero. Guess it was broken for years and nobody cared? |
Seems like it. Have you verified that the change actually fixes the original issue if we keep the function local |
Yes! I quickly backported the fix to the commit before my refactoring and the constructor now is only called once and doesn't overwrite the reporter to zero anymore. We should probably still keep the refactoring. |
Actually, let me add a unit test for this code! |
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.
Thanks!
Once the CI passes, I'll rebase and merge. Thanks for the great release notes, @rleh! |
This removes the useless Controller class.
2ffd362
to
47bff54
Compare
__cxa_guard_acquire