Make RELEASE_ASSERT use Bun__panic for crash reporting#167
Make RELEASE_ASSERT use Bun__panic for crash reporting#167sosukesuzuki wants to merge 2 commits intomainfrom
Conversation
CRASH_WITH_INFO macro now calls Bun__panic with file, line, and function info when USE(BUN_JSC_ADDITIONS) is enabled. This ensures RELEASE_ASSERT failures are reported to Sentry with full context instead of silently crashing via breakpoint trap.
WalkthroughAdds Bun-specific panic support: declares extern Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/WTF/wtf/Assertions.h`:
- Around line 1019-1022: The BUN macro CRASH_WITH_INFO currently discards
__VA_ARGS__ and omits the type check, so restore the compile-time guard by
invoking WTF::isIntegralOrPointerType(__VA_ARGS__) (or an equivalent
static_assert/type-trait expression) inside the macro while still not using the
values — for example evaluate the trait in a void-cast or unused constexpr to
force the type-check and then call bunPanicFromCrash(__FILE__, __LINE__,
WTF_PRETTY_FUNCTION); ensure the macro name CRASH_WITH_INFO and the
bunPanicFromCrash call remain unchanged and that only the type-check expression
referencing __VA_ARGS__ is added so non-integral/non-pointer arguments cause a
compile error.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
Source/WTF/wtf/Assertions.cppSource/WTF/wtf/Assertions.hSource/cmake/WebKitCompilerFlags.cmake
| #define CRASH_WITH_INFO(...) do { \ | ||
| bunPanicFromCrash(__FILE__, __LINE__, WTF_PRETTY_FUNCTION); \ | ||
| } while (false) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
BUN path silently discards ... args and drops the isIntegralOrPointerType type guard.
The non-BUN CRASH_WITH_INFO calls WTF::isIntegralOrPointerType(__VA_ARGS__) before crashing, which produces a hard compile error if a caller passes a non-integral/non-pointer type. The BUN path accepts any args without type-checking and silently ignores them. Code written in a BUN-only build environment that passes wrong-typed values to CRASH_WITH_INFO will compile cleanly but break non-BUN builds, making it a potential CI blind spot.
Consider preserving the guard even though the values themselves are intentionally dropped:
🛡️ Proposed fix – retain type guard
`#define` CRASH_WITH_INFO(...) do { \
+ WTF::isIntegralOrPointerType(__VA_ARGS__); \
bunPanicFromCrash(__FILE__, __LINE__, WTF_PRETTY_FUNCTION); \
} while (false)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define CRASH_WITH_INFO(...) do { \ | |
| bunPanicFromCrash(__FILE__, __LINE__, WTF_PRETTY_FUNCTION); \ | |
| } while (false) | |
| `#define` CRASH_WITH_INFO(...) do { \ | |
| WTF::isIntegralOrPointerType(__VA_ARGS__); \ | |
| bunPanicFromCrash(__FILE__, __LINE__, WTF_PRETTY_FUNCTION); \ | |
| } while (false) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/WTF/wtf/Assertions.h` around lines 1019 - 1022, The BUN macro
CRASH_WITH_INFO currently discards __VA_ARGS__ and omits the type check, so
restore the compile-time guard by invoking
WTF::isIntegralOrPointerType(__VA_ARGS__) (or an equivalent
static_assert/type-trait expression) inside the macro while still not using the
values — for example evaluate the trait in a void-cast or unused constexpr to
force the type-check and then call bunPanicFromCrash(__FILE__, __LINE__,
WTF_PRETTY_FUNCTION); ensure the macro name CRASH_WITH_INFO and the
bunPanicFromCrash call remain unchanged and that only the type-check expression
referencing __VA_ARGS__ is added so non-integral/non-pointer arguments cause a
compile error.
- Move VA_OPT_SUPPORTED macro definition outside #ifndef CRASH_WITH_INFO so it's always defined even when USE(BUN_JSC_ADDITIONS) is enabled. Fixes compile error in Integrity.h on Windows. - Make Bun__panic a weak symbol with CRASH() fallback so the jsc standalone binary can link without Bun's runtime. Fixes link errors on Linux musl and Linux ARM64 ASAN.
Preview Builds
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Source/WTF/wtf/Assertions.h (1)
1019-1025:⚠️ Potential issue | 🟡 MinorRestore compile‑time type guard in BUN CRASH_WITH_INFO.
Line 1023 dropsWTF::isIntegralOrPointerType(__VA_ARGS__), so non‑integral args compile in BUN builds but fail in non‑BUN. Re‑introduce the guard even if the values are ignored.🛡️ Suggested fix
`#define` CRASH_WITH_INFO(...) do { \ + WTF::isIntegralOrPointerType(__VA_ARGS__); \ bunPanicFromCrash(__FILE__, __LINE__, WTF_PRETTY_FUNCTION); \ } while (false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/WTF/wtf/Assertions.h` around lines 1019 - 1025, The CRASH_WITH_INFO macro lost its compile-time guard allowing non-integral/non-pointer arguments in BUN builds; restore a compile-time type check (using WTF::isIntegralOrPointerType on __VA_ARGS__) inside CRASH_WITH_INFO so any passed values are validated at compile time even though they are ignored, and keep the existing call to bunPanicFromCrash(__FILE__, __LINE__, WTF_PRETTY_FUNCTION); ensure the check is applied in the macro expansion (e.g., via a static_assert or a template helper referenced from CRASH_WITH_INFO) so builds without BUN continue to enforce the same type constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Source/WTF/wtf/Assertions.h`:
- Around line 1019-1025: The CRASH_WITH_INFO macro lost its compile-time guard
allowing non-integral/non-pointer arguments in BUN builds; restore a
compile-time type check (using WTF::isIntegralOrPointerType on __VA_ARGS__)
inside CRASH_WITH_INFO so any passed values are validated at compile time even
though they are ignored, and keep the existing call to
bunPanicFromCrash(__FILE__, __LINE__, WTF_PRETTY_FUNCTION); ensure the check is
applied in the macro expansion (e.g., via a static_assert or a template helper
referenced from CRASH_WITH_INFO) so builds without BUN continue to enforce the
same type constraints.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Source/WTF/wtf/Assertions.cppSource/WTF/wtf/Assertions.h
1 participant
When
USE(BUN_JSC_ADDITIONS)is enabled,CRASH_WITH_INFOnow callsBun__panicinstead of using a breakpoint trap. This meansRELEASE_ASSERTfailures go through Bun's crash handler, which provides:Changes
Assertions.h: RedefineCRASH_WITH_INFOforBUN_JSC_ADDITIONSto callbunPanicFromCrash()Assertions.cpp: ImplementbunPanicFromCrash()which formats a message and callsBun__panicWebKitCompilerFlags.cmake: Add linker flags forBun__panicsymbol