Fix JIT assert resolving to C runtime assert after jitshared introduction#124977
Fix JIT assert resolving to C runtime assert after jitshared introduction#124977jakobbotsch merged 3 commits intomainfrom
assert resolving to C runtime assert after jitshared introduction#124977Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
…m jitshared headers Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
assert resolving to C runtime assert after jitshared introduction
|
@copilot, the interpreter is now missing a definition for assert. Could you fix that by moving the jit assert API into jitshared? |
… interpreter Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Done in commit 40fb666. Created |
|
@davidwrighton what is missing to land this? It would be nice to get this in since it is affecting our ability to test in CI and evaluate our changes locally. |
|
@jakobbotsch I need someone to review this. If you do, please just merge it, as my review indicates its good. |
There was a problem hiding this comment.
Pull request overview
Fixes an assert-macro regression introduced by the jitshared/ split so JIT/interpreter assertions consistently route to the CoreCLR assertAbort handler (instead of falling back to the C runtime assert).
Changes:
- Introduces
jitshared/jitassert.has the shared source of truth forassert→assertAbort. - Ensures jitshared consumers get the correct assert macro via
jitshared.h(andhistogram.hfor interpreter builds without the jitshared include chain). - Removes direct
<assert.h>/<cassert>includes in jitshared code that were clobbering the custom assert mapping.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jitshared/jitshared.h | Includes jitassert.h so jitshared compilation units get the shared assert behavior by default. |
| src/coreclr/jitshared/jitassert.h | New shared assert definition/declaration that maps assert to assertAbort in DEBUG. |
| src/coreclr/jitshared/histogram.h | Includes jitassert.h directly to support interpreter builds that don’t include jitshared.h. |
| src/coreclr/jitshared/histogram.cpp | Drops <cassert> so it no longer overwrites the JIT/interpreter assert macro. |
| src/coreclr/jitshared/compallocator.h | Removes direct <assert.h> include that could overwrite the custom assert macro. |
| src/coreclr/jitshared/arenaallocator.h | Removes direct <assert.h> include that could overwrite the custom assert macro. |
After PR #123830 introduced the
jitshared/directory,assertin JIT (and interpreter) code was silently falling through to the C runtime'sassert, causing dialog popups on assertion failures instead of usingassertAbort.Root Cause
The JIT sets up
assert → assertAbortinhost.h, whichalloc.hincludes before the jitshared headers. However, three files injitshared/included<assert.h>/<cassert>directly, which unconditionally redefinedassertback to the C runtime version — clobbering the JIT's custom handler:arenaallocator.h—#include <assert.h>compallocator.h—#include <assert.h>histogram.cpp—#include <cassert>The interpreter had a related issue:
histogram.cpp(compiled as part of the interpreter without a PCH) had noassertdefinition at all after removing#include <cassert>, since the interpreter doesn't use a precompiled header to supply it.Changes
jitshared/arenaallocator.h,jitshared/compallocator.h: Remove#include <assert.h>. The correctassertis now provided byjitassert.hviajitshared.h.jitshared/histogram.cpp: Remove#include <cassert>—jitassert.h(included viahistogram.h) now supplies the correct definition.jitshared/jitassert.h(new): Shared header that declaresassertAbort(withANALYZER_NORETURN) and definesassertto use it in DEBUG builds, or((void)0)in release builds. This is the single source of truth for the assert API shared between JIT and interpreter.jitshared/jitshared.h: Includejitassert.hso all jitshared code that goes throughjitshared.hautomatically gets the correctassert.jitshared/histogram.h: Includejitassert.hdirectly, sohistogram.cppin the interpreter build (which doesn't go throughjitshared.h) also gets the correctassert.interpreter/interpreter.h: Reverted to original include order (compiler.hbefore theassertAbortblock) — jitshared code now correctly getsassertfromjitassert.hvia the jitshared include chain, making the ordering no longer relevant.Original prompt
assertresolves to C runtime assert #124954💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.