fix: Resolve MSVC Debug build failure in JobQueue.h; re-enable _CRTDBG_MAP_ALLOC in CI - #6993
Conversation
`boost/coroutine2/all.hpp` transitively pulls in `boost/context/pooled_fixedsize_stack.hpp`, which calls `.malloc()` and `.free()` as member functions on `boost::pool`. With `_CRTDBG_MAP_ALLOC` defined in non-CI Debug MSVC builds (cmake/XrplCompiler.cmake), the CRT redefines `malloc` / `free` as macros that expand textually, mangling the member calls into a syntax error. `pooled_fixedsize_stack` is unused in the project. Replace the umbrella include with `coroutine2/coroutine.hpp` (provides `coroutine`, `pull_type`, `push_type`, `asymmetric_coroutine`) and `coroutine2/protected_fixedsize_stack.hpp`, sidestepping the broken header entirely.
|
@ximinez Try building this branch locally on Windows, and let me know if it works fine. |
PR #6562 disabled `_CRTDBG_MAP_ALLOC` in CI to work around a macro collision in `boost/context/pooled_fixedsize_stack.hpp`. The previous commit resolves that collision at the source by narrowing the Boost.Coroutine2 include so the broken header is no longer pulled in. With the root cause fixed, restore the original define scope. CI now exercises the same Debug preprocessor configuration as local MSVC builds, guarding against regressions of the same bug.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6993 +/- ##
=========================================
- Coverage 81.9% 81.9% -0.0%
=========================================
Files 1010 1010
Lines 76383 76383
Branches 7530 7529 -1
=========================================
- Hits 62522 62521 -1
- Misses 13861 13862 +1
🚀 New features to boost your workflow:
|
ximinez
left a comment
There was a problem hiding this comment.
Wonderful! I'm testing a few different configurations, but the first ones I've tried have worked perfectly.
Co-authored-by: Ed Hennis <ed@ripple.com>
…d-issue-on-windows
|
Let's get this merged ASAP. Thanks! |
…d-issue-on-windows
There was a problem hiding this comment.
Pull request overview
This PR addresses an MSVC Debug build failure caused by _CRTDBG_MAP_ALLOC interacting badly with a transitive Boost header, by narrowing Boost.Coroutine2 includes in JobQueue.h to avoid pulling in the problematic boost/context/pooled_fixedsize_stack.hpp. With the root cause mitigated, it also restores _CRTDBG_MAP_ALLOC for CI Debug builds to keep CI behavior aligned with local Windows Debug builds.
Changes:
- Replace
boost/coroutine2/all.hppwith narrower Boost.Coroutine2 headers ininclude/xrpl/core/JobQueue.h. - Re-enable
_CRTDBG_MAP_ALLOCfor MSVC Debug builds in CI by removing theis_ciexclusion incmake/XrplCompiler.cmake.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| include/xrpl/core/JobQueue.h | Narrows Boost includes to avoid transitively including pooled_fixedsize_stack under _CRTDBG_MAP_ALLOC. |
| cmake/XrplCompiler.cmake | Restores _CRTDBG_MAP_ALLOC definition for all MSVC Debug builds (including CI) to remove the prior workaround. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include <boost/context/protected_fixedsize_stack.hpp> | ||
| #include <boost/coroutine2/all.hpp> | ||
| #include <boost/coroutine2/coroutine.hpp> | ||
| #include <boost/coroutine2/protected_fixedsize_stack.hpp> |
There was a problem hiding this comment.
boost/coroutine2/protected_fixedsize_stack.hpp appears unused here (the actual stack allocator used by JobQueue::Coro is boost::context::protected_fixedsize_stack in include/xrpl/core/Coro.ipp). Keeping this extra include undermines the stated goal of narrowing transitive Boost headers; consider removing it (or switching the implementation to use the coroutine2 wrapper) so the header set matches what’s actually referenced.
| #include <boost/coroutine2/protected_fixedsize_stack.hpp> |
There was a problem hiding this comment.
@pratikmankawde I suppose this header is necessary or clang-tidy would also have flagged it, wouldn't you think?
There was a problem hiding this comment.
Yeah, this could be one of those false negatives, because boost/coroutine2/protected_fixedsize_stack.hpp contains
using protected_fixedsize_stack = boost::context::protected_fixedsize_stack;. Just the aliasing. So keeping the include doesn't really change much, so does removing. I'll remove it anyway, since I am not using the aliased type later.
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
…dows' of github.com:XRPLF/rippled into pratik/fix-msvc-crtdbg-boost-context-build-issue-on-windows
…G_MAP_ALLOC in CI (XRPLF#6993) Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Co-authored-by: Ed Hennis <ed@ripple.com>
…G_MAP_ALLOC in CI (XRPLF#6993) Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Co-authored-by: Ed Hennis <ed@ripple.com>
High Level Overview of Change
MSVC Debug builds fail to compile
JobQueue.cppdue to a preprocessor collision between the CRT debug allocator macros and Boost.Context's pool-backed stack allocator. This PR:boost/coroutine2include ininclude/xrpl/core/JobQueue.hto only the sub-headers the project actually uses, avoiding the brokenboost/context/pooled_fixedsize_stack.hppentirely._CRTDBG_MAP_ALLOCin CI to paper over this same collision. With the root cause fixed, the gate is no longer needed.Context of Change
Reported build failure (local MSVC Debug build):
Root cause:
cmake/XrplCompiler.cmakedefines_CRTDBG_MAP_ALLOCfor Debug MSVC builds._CRTDBG_MAP_ALLOCmakes<crtdbg.h>redefinemalloc/freeas function-like preprocessor macros that expand to_malloc_dbg/_free_dbg.boost/context/pooled_fixedsize_stack.hpp(Boost 1.90) calls.malloc()/.free()as member functions onboost::pool, without theBOOST_PREVENT_MACRO_SUBSTITUTIONguard or defensive(p.malloc)()parenthesization that protects other Boost pool headers.PR #6562 previously disabled
_CRTDBG_MAP_ALLOCin CI as a symptomatic workaround (reasoning: CI doesn't consume Debug artifacts). That left the bug reproducible on any developer doing a local Windows Debug build — which is exactly the failure mode reported here.Why this fix:
pooled_fixedsize_stack— onlycoroutine,pull_type,push_type,asymmetric_coroutine, andprotected_fixedsize_stack. The umbrellaboost/coroutine2/all.hppwas dragging in three unused stack allocators, one of which is broken under_CRTDBG_MAP_ALLOC.boost/coroutine2/coroutine.hpp+boost/coroutine2/protected_fixedsize_stack.hppavoids the broken header entirely. No preprocessor fences or_MSC_VERgymnastics needed.boost/context/pooled_fixedsize_stack.hpphas only four transitive entry points in Boost (coroutine2/all.hpp,coroutine2/pooled_fixedsize_stack.hpp,fiber/all.hpp,fiber/pooled_fixedsize_stack.hpp) — none of which the project now uses.JobQueue.h.Type of Change
Test Plan
clangLinux builds unaffected (symbols still resolve via the narrower headers)windows-amd64-releasematrix leg).github/scripts/strategy-matrix/generate.py, Windows Debug only runs in the scheduledallmatrix (on merges todevelop/release), not in PRminimalmode. This PR should be validated locally on Windows Debug before merge, or expect Debug coverage to land only post-merge via the nightly matrix.Future Tasks
Consider adding a Windows Debug leg to the PR matrix for faster validation of this class of bug — or at minimum, document in
CONTRIBUTING.mdthat Windows Debug regressions will only surface on nightly runs.