Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ConorWilliams
force-pushed
the
v4-uts-port
branch
2 times, most recently
from
2010d15 to
948912f
Compare
ConorWilliams
force-pushed
the
v4-uts-port
branch
from
948912f to
aa49a11
Compare
ConorWilliams
force-pushed
the
v4-slab-2
branch
2 times, most recently
from
ffc365c to
03c498b
Compare
ConorWilliams
force-pushed
the
v4-slab-2
branch
3 times, most recently
from
d1adcf0 to
e91cd5d
Compare
ConorWilliams
force-pushed
the
v4-slab-2
branch
from
e91cd5d to
d493533
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/batteries/slab_stack.cxx`:
- Around line 109-114: The push() routine currently does pointer arithmetic
diff_type free_bytes = node_size * (m_hi - m_sp) which is UB if init_slab failed
in release() leaving m_ctrl/m_lo/m_sp/m_hi == nullptr; modify push() to first
check that m_hi and m_sp are non-null (e.g., if (m_hi == nullptr || m_sp ==
nullptr) throw std::bad_alloc{} or handle as out-of-space) before performing
m_hi - m_sp, and apply the same null-safety check in the analogous location
around lines 158-163; also update the comment in release() to note that pointers
are cleared and that callers must handle null pointer state so subsequent
pointer arithmetic is guarded.
In `@test/src/stack.cpp`:
- Around line 129-137: Wrap tests that depend on exceptions with conditional
compilation and avoid catching std::bad_alloc for stacks that allocate
dynamically: enclose the entire "slab_stack - throws when full" test in `#if`
LF_COMPILER_EXCEPTIONS ... `#endif`, and in the TEMPLATE_TEST_CASEs that iterate
over geometric_stack, adaptor_stack and slab_stack, remove or guard the
try/catch(std::bad_alloc) blocks with `#if` LF_COMPILER_EXCEPTIONS so only
slab_stack is allowed to trigger and catch bad_alloc; alternatively use
type-specific branches (e.g., if constexpr or trait for fixed_capacity_stack) to
only perform exception-based early-exit logic for slab_stack and use standard
allocation failure checks for geometric_stack/adaptor_stack.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d47dbc6-0c30-4623-a7d3-0840622eb4b7
📒 Files selected for processing (6)
CMakeLists.txtbenchmark/src/libfork_benchmark/fib/libfork.cppsrc/batteries/batteries.cxxsrc/batteries/geometric_stack.cxxsrc/batteries/slab_stack.cxxtest/src/stack.cpp
| // Optimized to just the subtraction because multiplication cancels the implicit division. | ||
| diff_type free_bytes = node_size * (m_hi - m_sp); | ||
|
|
||
| if (push_bytes > free_bytes) [[unlikely]] { | ||
| LF_THROW(std::bad_alloc{}); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the failed-release path and push null-state handling stay in sync.
rg -n -C4 'm_ctrl = nullptr|m_lo = nullptr|m_sp = nullptr|m_hi = nullptr|m_hi - m_sp|LF_THROW\(std::bad_alloc' src/batteries/slab_stack.cxxRepository: ConorWilliams/libfork
Length of output: 2224
Fix null pointer subtraction in push() after failed release() reallocation.
When init_slab(next_size) throws in release(), all pointers (m_ctrl, m_lo, m_sp, m_hi) are set to nullptr and the exception is swallowed. The next push() then attempts m_hi - m_sp with both pointers null, which is undefined behavior under C++ (pointer subtraction is only defined for pointers into the same array). This allows compiler optimizations to eliminate the safety check at line 112-113, leaving the stack in a corrupted state.
Add an explicit null check before pointer arithmetic:
Proposed fix
constexpr auto push(std::size_t size) -> void_ptr {
LF_ASSUME(size > 0);
constexpr diff_type node_size = sizeof(node);
diff_type push_bytes = safe_cast<diff_type>(round_to_multiple<sizeof(node)>(size));
LF_ASSUME(push_bytes >= node_size);
LF_ASSUME(push_bytes % node_size == 0);
+ if (m_ctrl == nullptr) [[unlikely]] {
+ LF_THROW(std::bad_alloc{});
+ }
+
// Optimized to just the subtraction because multiplication cancels the implicit division.
diff_type free_bytes = node_size * (m_hi - m_sp);Also update the comment in release() to reflect the new check:
} LF_CATCH_ALL {
- // If ^ throws, swallow the exception — push will see no space
- // i.e. (m_hi - m_sp == 0) and throw instead.
+ // If ^ throws, swallow the exception — the next push observes m_ctrl == nullptr
+ // and reports allocation failure.
}Also applies to: 158-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/batteries/slab_stack.cxx` around lines 109 - 114, The push() routine
currently does pointer arithmetic diff_type free_bytes = node_size * (m_hi -
m_sp) which is UB if init_slab failed in release() leaving m_ctrl/m_lo/m_sp/m_hi
== nullptr; modify push() to first check that m_hi and m_sp are non-null (e.g.,
if (m_hi == nullptr || m_sp == nullptr) throw std::bad_alloc{} or handle as
out-of-space) before performing m_hi - m_sp, and apply the same null-safety
check in the analogous location around lines 158-163; also update the comment in
release() to note that pointers are cleared and that callers must handle null
pointer state so subsequent pointer arithmetic is guarded.
| // Push phase — break early if slab_stack exhausts its fixed capacity | ||
| for (std::size_t j = 0; j < depth; ++j) { | ||
| std::size_t s = size_dist(rng); | ||
| void *p = stack.push(s); | ||
| void *p = nullptr; | ||
| try { | ||
| p = stack.push(s); | ||
| } catch (std::bad_alloc const &) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify exception-dependent test paths are guarded consistently.
rg -n -C3 'try\s*\{|catch\s*\(|REQUIRE_THROWS_AS|LF_COMPILER_EXCEPTIONS' test/src/stack.cppRepository: ConorWilliams/libfork
Length of output: 2565
🏁 Script executed:
# Get fuller context around the problematic lines and check test structure
rg -n -B10 -A5 'REQUIRE_THROWS_AS|try\s*\{' test/src/stack.cpp | head -150Repository: ConorWilliams/libfork
Length of output: 4983
🏁 Script executed:
# Check which stack types are instantiated in the tests
rg -n 'TEST_CASE|stack\.push|slab_stack|geometric_stack|adaptor_stack' test/src/stack.cpp | head -80Repository: ConorWilliams/libfork
Length of output: 1882
🏁 Script executed:
# Check the exact range of "slab_stack - throws when full" test
sed -n '267,278p' test/src/stack.cppRepository: ConorWilliams/libfork
Length of output: 453
🏁 Script executed:
# Check if there's any conditional compilation around the catch blocks in template tests
sed -n '109,140p' test/src/stack.cppRepository: ConorWilliams/libfork
Length of output: 1048
Guard exception-only test and refine multi-stack exception handling.
The test "slab_stack - throws when full" (lines 267–278) and the three TEMPLATE_TEST_CASEs that run against geometric_stack, adaptor_stack, and slab_stack (lines 111, 153, 202) contain unguarded try/catch(std::bad_alloc) or REQUIRE_THROWS_AS blocks. These fail to compile when LF_COMPILER_EXCEPTIONS == 0, unlike the later tests guarded at line 316.
Additionally, catching bad_alloc from geometric_stack and adaptor_stack (which use dynamic allocation) masks unexpected failures; only slab_stack's fixed-capacity exhaustion should trigger early exit.
- Wrap "slab_stack - throws when full" in
#if LF_COMPILER_EXCEPTIONS. - In the three template tests, either guard the catch blocks with
#if LF_COMPILER_EXCEPTIONSor refactor to differentiate slab_stack behavior (e.g., via conditional compilation or type-specific handling).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/src/stack.cpp` around lines 129 - 137, Wrap tests that depend on
exceptions with conditional compilation and avoid catching std::bad_alloc for
stacks that allocate dynamically: enclose the entire "slab_stack - throws when
full" test in `#if` LF_COMPILER_EXCEPTIONS ... `#endif`, and in the
TEMPLATE_TEST_CASEs that iterate over geometric_stack, adaptor_stack and
slab_stack, remove or guard the try/catch(std::bad_alloc) blocks with `#if`
LF_COMPILER_EXCEPTIONS so only slab_stack is allowed to trigger and catch
bad_alloc; alternatively use type-specific branches (e.g., if constexpr or trait
for fixed_capacity_stack) to only perform exception-based early-exit logic for
slab_stack and use standard allocation failure checks for
geometric_stack/adaptor_stack.
1 participant
Summary by CodeRabbit
New Features
slab_stack, a fixed-size, single-slab user-space stack implementation for memory allocation.slab_stackvariants.Bug Fixes
geometric_stackmethod signature.Tests
slab_stackacross multiple scenarios, including allocation limits and checkpoint/acquire/release cycles.