feat: Memory management refactoring and MSVC compilation fixes #162
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
…r semantics ROOT CAUSE: The allocate() function was incorrectly constructing objects during memory allocation, violating C++ allocator requirements. MSVC's std::_Tree_node has a deleted default constructor, causing compilation failure. CHANGES: - container_allocator::allocate() now only allocates raw memory without constructing objects (removed mem::$new and ipc::construct calls) - container_allocator::deallocate() now only frees memory without destroying objects (removed mem::$delete and ipc::destroy_n calls) WHY THIS FIXES THE ISSUE: C++ allocator semantics require strict separation: * allocate() -> raw memory allocation only * construct() -> object construction with proper arguments * destroy() -> object destruction * deallocate() -> memory deallocation only Standard containers (like std::map) call construct() with proper arguments (key, value) to initialize nodes, not allocate(). Since std::_Tree_node in MSVC has no default constructor (= delete), attempting to construct it without arguments always fails. Fixes MSVC 2017 compilation error: error C2280: 'std::_Tree_node<...>::_Tree_node(void)': attempting to reference a deleted function
IMPROVEMENTS: 1. Add explicit zero-argument overload to avoid SFINAE ambiguity 2. Require at least one argument (A1) for parameterized overloads 3. Better separation between direct initialization and aggregate initialization BENEFITS: - Clearer intent: zero-argument construction is explicitly handled - Avoids potential SFINAE ambiguity when empty parameter pack is used - More maintainable: easier to understand which overload is selected - Consistent with modern C++ best practices for variadic templates TECHNICAL DETAILS: - Zero-arg overload: Always uses T() for value initialization - One-or-more-arg overload: Uses SFINAE to choose between: * T(args...) for types with matching constructor * T{args...} for aggregate types or types with initializer_list ctor This is a code quality improvement and does not fix any compilation issues, but provides better template overload resolution. ROOT CAUSE: Array placement new (::new(buffer) T[N]) adds a hidden cookie (array size) before the array elements in some compiler implementations (particularly MSVC). The cookie is used for proper array destruction. However, the data_set buffer was sized only for sizeof(T[N]), not accounting for the cookie overhead. ISSUE: - Buffer allocated: sizeof(rand_buf[LoopCount]) - Actual space needed: sizeof(cookie) + sizeof(rand_buf[LoopCount]) - Result: Cookie and part of array written beyond buffer boundary - Consequence: Memory corruption, leading to invalid pointers in buffer objects SYMPTOM: In IPC.1v1 test, memcpy(buf, data, size) crashed because 'data' pointer (from buffer::data()) pointed to corrupted/invalid memory address. SOLUTION: Replace array placement new with individual element placement new: - Cast buffer to array pointer directly (no cookie needed) - Construct each element individually with placement new - Manually destroy each element in destructor This approach: - Eliminates cookie overhead - Provides precise control over object lifetime - Works consistently across all compilers Fixes crash in IPC.1v1 test case on MSVC.
…er names ISSUE: MSVC compiler reports warning C4138: '*/' found outside of comment for patterns like 'void */*p*/' where the pointer asterisk is immediately followed by a comment start. AFFECTED FILES: - include/libipc/mem/new.h (line 30) - src/libipc/platform/win/mutex.h (line 54) - src/libipc/platform/win/semaphore.h (line 53) CHANGES: Changed 'type */*param*/' to 'type * /*param*/' (added space before comment) Examples: - void */*p*/ → void * /*p*/ - char const */*name*/ → char const * /*name*/ This resolves the MSVC warning while maintaining code functionality and keeping the commented-out parameter names for documentation.
After rebasing onto master, test.h was moved to test/archive/. Updated include paths in test subdirectories: - test/imp/*.cpp: "test.h" -> "../archive/test.h" - test/mem/*.cpp: "test.h" -> "../archive/test.h" - test/concur/*.cpp: "test.h" -> "../archive/test.h" This ensures all test files can properly find the test header after the directory reorganization in master branch.
The acquire() function allocates id_info_t using mem::$new<id_info_t>(), so the release() function must use mem::$delete(ii) to deallocate it, not mem::free(ii). This ensures proper allocation/deallocation pairing. Issue: Memory allocated with mem::$new must be freed with mem::$delete to maintain consistent memory management semantics.
…ail.h Fixed two critical issues from the rebase: 1. Added LIBIPC_OS_FREEBSD macro definition in detect_plat.h to enable FreeBSD platform detection alongside other OS checks 2. Added missing #include "libipc/imp/detect_plat.h" in detail.h to properly include platform detection macros These fixes ensure FreeBSD compilation will work correctly with the unified platform detection system.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Overview
This PR introduces a comprehensive memory management refactoring and fixes critical MSVC compilation issues, ensuring cross-platform compatibility.
Key Changes
🔧 Memory Management Refactoring (41 commits)
Core Memory System
mem::$newandmem::$deletewith proper pairing semanticsmemory_resourceandmonotonic_buffer_resourcefor flexible memory managementblock_poolfor fixed-size allocationscontainer_allocatorto follow C++ allocator requirements correctlylibipc/memory/→libipc/mem/for consistencyMemory Allocator Improvements
intrusive_stackfor lock-free data structures🐛 MSVC Compilation Fixes
Critical Fixes
container_allocator semantics (Commit 4338557): Fixed allocator to only allocate/deallocate memory, not construct/destroy objects
allocate()was incorrectly callingmem::$newwhich constructs objectsstd::_Tree_nodeon MSVC 2017uninitialized.h overload resolution (Commit 11973b9): Improved
construct()template overloadsArray placement new buffer overflow (Commit a83ba83): Fixed
data_setclass in testsC4138 warning (Commit 43b20b2): Fixed
*/*pattern in commented parametersvoid */*p*/→void * /*p*/to avoid warningshm_win.cpp memory pairing (Commit 636d84b): Fixed memory allocation/deallocation mismatch
acquire()usesmem::$new→release()must usemem::$delete, notmem::free🧪 Test Infrastructure Updates
test/imp/,test/mem/,test/concur/: Updated"test.h"→"../archive/test.h"🔧 Implementation Utilities
New Utilities (imp directory)
detect_plat.h: Unified platform detection with FreeBSD supportfmt: String formatting supportcodecvt: Character encoding conversionerror&log: Error handling and logging utilitiesresult: Result type for error handlingexpected: Expected type implementationnameof&scope_exit: Utility macrosCode Improvements
std::hashfmt🌐 Platform Support
LIBIPC_OS_FREEBSDmacroIPC_EXPORT→LIBIPC_EXPORT, platform-specific macros →LIBIPC_OS_*Testing
Platforms Tested
Test Cases
Migration Notes
Breaking Changes
mem::$new/$deletepairs instead ofmem::alloc/freewhere appropriatelibipc/memory/→libipc/mem/IPC_OS_*→LIBIPC_OS_*Upgrade Path
No API changes for library users. Internal refactoring only.
Related Issues
Checklist
Commits Summary
Total commits: 41
Key commit categories: