Compile-time Override of Default Assert Hooks#5058
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| #define fileIdFs "Assert: \"%s:%" PRI_FwSizeType "\"" | ||
| #endif | ||
|
|
||
| void Fw::defaultPrintAssert(const CHAR* msg) { |
| (void)fputs("\n", stderr); | ||
| } | ||
|
|
||
| void Fw::defaultReportAssert(FILE_NAME_ARG file, |
| FwAssertArgType arg4, | ||
| FwAssertArgType arg5, | ||
| FwAssertArgType arg6, | ||
| CHAR* destBuffer, |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
thomas-bc
added
Breaking Changes / Needs Release Notes
| #include <Fw/Types/format.hpp> | ||
| #include <cassert> | ||
| #include <cstdio> | ||
| #include <cstdlib> |
| assert(0); | ||
| abort(); | ||
| } | ||
|
|
| assert(0); | ||
| abort(); | ||
| } | ||
|
|
LeStarch
added
C++
|
Remember to merge the following PRs upon merging of this current PRs: |
thomas-bc
left a comment
thomas-bc
left a comment
There was a problem hiding this comment.
I like that this leverages the new F´ link-time override mechanism. My recommendation is to consolidate on this and discard the old registerHook pattern altogether, but this will need some refactor of AssertFatalAdapter. TBD on how - will discuss offline.
| @@ -0,0 +1,77 @@ | |||
| // ====================================================================== | |||
| // \title fputs_assert_hook.cpp | |||
There was a problem hiding this comment.
nit: I would call this file assert_hook_fputs.cpp so it can be listed closer alongside assert_hook.hpp
| assert(0); | ||
| defaultDoAssert(); | ||
| } else { | ||
| registeredHook->reportAssert(file, lineNo, numArgs, arg1, arg2, arg3, arg4, arg5, arg6); |
There was a problem hiding this comment.
blocking on design decision / discussion:
we should figure out what to do with the old runtime hook pattern. We probably don't want to keep 2 different patterns to override this behavior (this new link-time pattern, plus the old runtime registerHook pattern). The thing that needs to be considered is how to update AssertFatalAdapter which currently uses the registerHook to register itself at runtime.
| //! | ||
| //! Handle an assertion failure or FATAL. This function is called after the defaultPrintAssert has | ||
| //! been called. The default implementation calls `assert(0)`. | ||
| [[noreturn]] void defaultDoAssert(); |
There was a problem hiding this comment.
If we're in the business of refactoring things, we should pick better names for these functions. "reportAssert", "doAssert" etc. is confusing at best, and most likely just not correctly describing what it actually does. I think I recall Rob said something about this.
4 participants
Change Description
Implements #5057 (assuming it passes CCB, close this otherwise...)
Rationale
See issue. TLDR, for small platform
Testing/Review Recommendations
Future Work
Need to update platforms to point to this default assert implementation.
AI Usage (see policy)
Claude code read the issue and drafted solution. I made some naming changes but in general looks pretty good.
Closes #5057