Add builtin_unreachable to the FW_ASSERT macros#5228
Conversation
Updated FW_UNREACHABLE definition to provide a no-op fallback for unknown compilers.
There was a problem hiding this comment.
Pull request overview
This PR updates the framework assertion macros to optionally include a compiler “unreachable” hint after Fw::SwAssert, enabling better optimization/analysis when assertions are configured to always abort.
Changes:
- Introduces
FW_UNREACHABLE()andFW_ASSERT_UNREACHABLE()macros to provide a portable (and configurable) unreachable hint. - Updates
FW_ASSERT(...)expansions to invokeFW_ASSERT_UNREACHABLE()afterFw::SwAssert(...)in the failing-assert branch.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Automated review summary (run 1)
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 0 | 0 | 0 | 0 | Go |
| Supply Chain / Runner Safety | 0 | 0 | 1 | 0 | 1 | Go |
| F Prime C/C++ Design | 0 | 0 | 0 | 0 | 0 | Go |
| Documentation Currency | 1 | 0 | 0 | 1 | 2 | No-Go |
| Design | 0 | 0 | 0 | 0 | 0 | Go |
| Architecture | 0 | 0 | 0 | 0 | 0 | Go |
| Test Quality | 0 | 0 | 0 | 0 | 0 | Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 1 | 0 | 1 | 1 | 3 | No-Go |
Supply-chain surfaces
| Surface | Outstanding |
|---|---|
| Dependencies | clean |
| Vendored / submodule | clean |
| Build / test infrastructure | clean |
| Workflows / actions / scripts | clean |
| Generator output | clean |
| Prompt-injection | 1 could-fix — hidden HTML comment in PR body targets AI agents |
| Review-system integrity | clean |
Outstanding must-fix items (1)
Documentation Currency
- New public macro
FW_UNREACHABLE()and changedFW_ASSERTbehavior not documented indocs/user-manual/framework/assert.md— link
Merge readiness
Merge readiness: No-Go — Documentation Currency has 1 outstanding must-fix item.
Clear skies ahead once the docs catch up — this is a solid improvement to the assert infrastructure.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| // Define FW_UNREACHABLE to hint to the compiler that a code path is unreachable. | ||
| // Falls back to a no-op on compilers that do not support __builtin_unreachable. | ||
| // (1) __has_builtin — the modern check, available in Clang and GCC >= 10. | ||
| // (2) defined(__GNUC__) — catches older GCC (4.5–9) which lack __has_builtin | ||
| #ifdef __has_builtin | ||
| #if __has_builtin(__builtin_unreachable) | ||
| #define FW_UNREACHABLE() __builtin_unreachable() | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
The Copilot review correctly identified that this branch can redefine a user-provided FW_UNREACHABLE. Other macros in this file (e.g., FW_ASSERT_NORETURN at line 88, NOINLINE at line 102) use #ifndef guards. Wrapping the __has_builtin detection in #ifndef FW_UNREACHABLE prevents redefinition warnings and allows intentional overrides.
| // Define FW_UNREACHABLE to hint to the compiler that a code path is unreachable. | |
| // Falls back to a no-op on compilers that do not support __builtin_unreachable. | |
| // (1) __has_builtin — the modern check, available in Clang and GCC >= 10. | |
| // (2) defined(__GNUC__) — catches older GCC (4.5–9) which lack __has_builtin | |
| #ifdef __has_builtin | |
| #if __has_builtin(__builtin_unreachable) | |
| #define FW_UNREACHABLE() __builtin_unreachable() | |
| #endif | |
| #endif | |
| // Define FW_UNREACHABLE to hint to the compiler that a code path is unreachable. | |
| // Users may pre-define FW_UNREACHABLE to override the detection below. | |
| // Falls back to a no-op on compilers that do not support __builtin_unreachable. | |
| // (1) __has_builtin — the modern check, available in Clang and GCC >= 10. | |
| // (2) defined(__GNUC__) — catches older GCC (4.5–9) which lack __has_builtin | |
| #ifndef FW_UNREACHABLE | |
| #ifdef __has_builtin | |
| #if __has_builtin(__builtin_unreachable) | |
| #define FW_UNREACHABLE() __builtin_unreachable() | |
| #endif | |
| #endif | |
| #endif |
Documentation updates (from automated review)The review flagged two documentation gaps. Below are the changes needed. They are also available on branch 1.
|
Change Description
Adds
builtin_unreachable(available on GCC, Clang, maybe others) toFW_ASSERT()to signal that this is unreachable code. This should help resolve some false positives on static analyzers, and maybe other things.