Skip to content

Commit 89fc74d

Browse files
mjp41Copilot
andcommitted
Fix CI failures across release/debug, MSVC, GCC, 32-bit and clang-format
Aggregated fixes for issues surfaced by CI after the main feature commits landed: * arenabins.h Bitmap: add a constexpr default ctor so threadalloc.h's default_alloc satisfies clang require_constant_initialization. * redblacktree.h::print: initialise s_indent (GCC -Wmaybe-uninitialized). * sizeclasstable.h: add operator!= alongside operator== for Apple Clang gnu++17; restore sentinel slot's slab_mask = ~size_t(0) so the bounds-checked memcpy shim treats foreign pointers as unbounded; fix 32-bit overflow in start_of_object by routing through slab_index. * largearenarange test: pass MinBaseSizeBits<Pal>() as the MIN_REFILL_SIZE_BITS so PalRange has enough to reserve on Windows. * smallarenarange test: MSVC rejects alignas(MIN_CHUNK_SIZE) on static storage; oversize the backing buffers and align at runtime. * arena test mock_index: replace the indirect probe in can_consolidate with an explicit in-range guard (GCC -Warray-bounds saw the OOB path even with SNMALLOC_ASSUME). * Wrap assert-only test locals in UNUSED(...) for NDEBUG builds; use size_t{} literals to silence -Wsign-conversion under clang/UBSan/TSan. * Apply clang-format-15 fixes from CI verbatim (13 files). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 00e7a80 commit 89fc74d

16 files changed

Lines changed: 111 additions & 58 deletions

File tree

src/snmalloc/backend_helpers/arena.h

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,8 @@ namespace snmalloc
9494
if (size == UNIT_SIZE)
9595
return ArenaVariant::Min;
9696
if (size == TWO_UNITS)
97-
return ((addr >> MIN_SIZE_BITS) & 1) == 0 ?
98-
ArenaVariant::EvenTwo :
99-
ArenaVariant::OddTwo;
97+
return ((addr >> MIN_SIZE_BITS) & 1) == 0 ? ArenaVariant::EvenTwo :
98+
ArenaVariant::OddTwo;
10099
return ArenaVariant::Large;
101100
}
102101

@@ -365,15 +364,15 @@ namespace snmalloc
365364

366365
for (size_t bin = 0; bin < Bins::Bitmap::TOTAL_BINS; bin++)
367366
{
368-
bin_trees[bin].for_each([&](uintptr_t node) {
369-
auto [a, s] = range_from_addr(node);
370-
if (s >= TWO_UNITS)
371-
{
372-
auto path = range_tree.get_root_path();
373-
SNMALLOC_CHECK(range_tree.find(path, node));
374-
bin_tree_nonmin_count++;
375-
}
376-
});
367+
bin_trees[bin].for_each([&](uintptr_t node) {
368+
auto [a, s] = range_from_addr(node);
369+
if (s >= TWO_UNITS)
370+
{
371+
auto path = range_tree.get_root_path();
372+
SNMALLOC_CHECK(range_tree.find(path, node));
373+
bin_tree_nonmin_count++;
374+
}
375+
});
377376
}
378377

379378
range_tree.for_each([&](uintptr_t node) {

src/snmalloc/backend_helpers/arenabins.h

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -309,16 +309,14 @@ namespace snmalloc
309309
*/
310310
class Bitmap
311311
{
312-
friend struct ArenaBinsTestAccess<
313-
INTERMEDIATE_BITS,
314-
MIN_SIZE_BITS>;
312+
friend struct ArenaBinsTestAccess<INTERMEDIATE_BITS, MIN_SIZE_BITS>;
315313

316314
public:
317315
/// Strict upper bound on bin ids `bin_index` produces. Exposed
318316
/// so callers can size parallel arrays (one RB-tree per bin id).
319317
static constexpr size_t TOTAL_BINS = BINS_PER_EXP * bits::BITS;
320318

321-
Bitmap() : words_{} {}
319+
constexpr Bitmap() : words_{} {}
322320

323321
/**
324322
* Classify `block`, set its bin's bit, return the bin id.
@@ -373,6 +371,7 @@ namespace snmalloc
373371
{
374372
const bitmap_info_t& info = bitmap_info_for_request(n);
375373
SNMALLOC_ASSERT(info.start_word < NUM_BITMAP_WORDS);
374+
SNMALLOC_ASSUME(info.start_word < NUM_BITMAP_WORDS);
376375

377376
// First word: start bin + any within-exp neighbours in same word.
378377
size_t word = info.start_word;

src/snmalloc/backend_helpers/backend_helpers.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,13 @@
22

33
#include "../mem/mem.h"
44
#include "authmap.h"
5-
#include "largearenarange.h"
65
#include "commitrange.h"
76
#include "commonconfig.h"
87
#include "defaultpagemapentry.h"
98
#include "empty_range.h"
109
#include "globalrange.h"
1110
#include "indirectrange.h"
11+
#include "largearenarange.h"
1212
#include "logrange.h"
1313
#include "noprange.h"
1414
#include "pagemap.h"

src/snmalloc/backend_helpers/inplacerep.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,8 +234,7 @@ namespace snmalloc
234234
static ArenaVariant get_variant(uintptr_t addr)
235235
{
236236
auto w = unit_at<0>(addr)->word_one;
237-
return static_cast<ArenaVariant>(
238-
(w & VARIANT_MASK) >> VARIANT_SHIFT);
237+
return static_cast<ArenaVariant>((w & VARIANT_MASK) >> VARIANT_SHIFT);
239238
}
240239

241240
static void set_variant(uintptr_t addr, ArenaVariant v)

src/snmalloc/ds/sizeclasstable.h

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,11 @@ namespace snmalloc
136136
{
137137
return value == other.value;
138138
}
139+
140+
constexpr bool operator!=(sizeclass_t other)
141+
{
142+
return value != other.value;
143+
}
139144
};
140145
141146
/**
@@ -313,6 +318,17 @@ namespace snmalloc
313318
314319
constexpr SizeClassTable()
315320
{
321+
// Sentinel slot (sizeclass_t{} / raw 0) covers any address whose
322+
// pagemap entry is unmapped or owned by the backend — including
323+
// foreign (non-snmalloc) heap addresses reached via the
324+
// bounds-checked memcpy shim before snmalloc has seen them.
325+
// `slab_mask = ~size_t(0)` makes `start_of_object` collapse
326+
// `addr & ~slab_mask` to 0 and `index_in_object` to `addr`, so
327+
// `remaining_bytes = sentinel.size - addr` underflows to a very
328+
// large value and any memcpy bound check trivially passes the
329+
// sentinel through to the destination's native checks.
330+
start_[0].slab_mask = ~size_t(0);
331+
316332
size_t max_capacity = 0;
317333
318334
for (smallsizeclass_t sizeclass(0); sizeclass < NUM_SMALL_SIZECLASSES;
@@ -404,13 +420,16 @@ namespace snmalloc
404420
constexpr SizeClassTable sizeclass_metadata = SizeClassTable();
405421
406422
// Sentinel must remain zero-initialised so fast-path lookups via
407-
// `start(sc)` return zero size and slab_mask without a branch.
423+
// `start(sc)` return zero size without a branch. Slab_mask is
424+
// `~size_t(0)` so foreign-pointer `remaining_bytes` underflows to a
425+
// huge value (see `SizeClassTable::SizeClassTable`).
408426
static_assert(
409427
sizeclass_metadata.start(sizeclass_t{}).size == 0,
410428
"sentinel slot must have size 0");
411429
static_assert(
412-
sizeclass_metadata.start(sizeclass_t{}).slab_mask == 0,
413-
"sentinel slot must have slab_mask 0");
430+
sizeclass_metadata.start(sizeclass_t{}).slab_mask == ~size_t(0),
431+
"sentinel slot must have slab_mask ~0 for foreign-pointer "
432+
"remaining_bytes underflow");
414433
415434
static_assert(
416435
bits::BITS - sizeclass_metadata.DIV_MULT_SHIFT <= MAX_CAPACITY_BITS);
@@ -536,8 +555,7 @@ namespace snmalloc
536555
if (SNMALLOC_LIKELY(osc.offset() == 0))
537556
{
538557
address_t slab_base = addr & ~meta.slab_mask;
539-
size_t in_slab = addr - slab_base;
540-
size_t index = (in_slab * meta.div_mult) >> DIV_MULT_SHIFT;
558+
size_t index = slab_index(osc, addr);
541559
return slab_base + (index * meta.size);
542560
}
543561
address_t alloc_start = (addr & ~meta.slab_mask) - meta.offset_bytes;

src/snmalloc/ds_core/redblacktree.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -457,7 +457,7 @@ namespace snmalloc
457457
// insufficient to accurately display the tree, but it will still be
458458
// memory safe as the search code is bounded by the string size.
459459
static constexpr size_t max_depth = 128;
460-
char s_indent[max_depth];
460+
char s_indent[max_depth] = {};
461461
size_t end = 0;
462462
for (; end < max_depth - 1; end++)
463463
{

src/snmalloc/override/new.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,7 @@ namespace snmalloc
3737

3838
SNMALLOC_ASSERT(
3939
secondary_allocator ||
40-
is_start_of_object(
41-
size_to_sizeclass_full(size), address_cast(p)));
40+
is_start_of_object(size_to_sizeclass_full(size), address_cast(p)));
4241

4342
return p;
4443
}

src/test/func/aligned_dealloc/aligned_dealloc.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@
1919
*/
2020

2121
#include "test/setup.h"
22+
#include "test/snmalloc_testlib.h"
2223

2324
#include <iostream>
24-
#include "test/snmalloc_testlib.h"
2525

2626
using namespace snmalloc;
2727

src/test/func/arena/arena.cc

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ namespace snmalloc
9292
{
9393
size_t idx = addr >> MIN_CHUNK_BITS;
9494
SNMALLOC_ASSERT(idx < MOCK_ARENA_CHUNKS);
95+
SNMALLOC_ASSUME(idx < MOCK_ARENA_CHUNKS);
9596
return idx;
9697
}
9798

@@ -201,14 +202,17 @@ namespace snmalloc
201202

202203
// Mirrors PagemapRep::can_consolidate, which reads
203204
// entry.is_boundary() from the pagemap. The boundary flag lives
204-
// per-chunk in mock_store; mock_index asserts the index is in
205-
// range, so any caller that probes outside the arena trips the
206-
// assertion — this catches accidental out-of-region probes in
207-
// Arena unit tests rather than as a release-build
208-
// segfault.
205+
// per-chunk in mock_store. An out-of-region probe returns false
206+
// (cannot consolidate) — both because that is the right semantic
207+
// (no neighbour exists outside the arena) and because it gives
208+
// GCC's release-mode `-Warray-bounds` analysis a visible guard
209+
// covering the `mock_store[...]` read on this branch.
209210
static bool can_consolidate(uintptr_t addr)
210211
{
211-
return !mock_store[mock_index(addr)].boundary;
212+
size_t idx = addr >> MIN_CHUNK_BITS;
213+
if (idx >= MOCK_ARENA_CHUNKS)
214+
return false;
215+
return !mock_store[idx].boundary;
212216
}
213217
};
214218

@@ -279,7 +283,13 @@ namespace snmalloc
279283
reset_mock_store();
280284
uintptr_t a = chunk_addr(20);
281285

282-
for (size_t s : {3, 7, 15, 63, 255, 1000})
286+
for (size_t s :
287+
{size_t{3},
288+
size_t{7},
289+
size_t{15},
290+
size_t{63},
291+
size_t{255},
292+
size_t{1000}})
283293
{
284294
MockRep::set_large_size(a, s);
285295
SNMALLOC_ASSERT(MockRep::get_large_size(a) == s);
@@ -462,6 +472,7 @@ namespace snmalloc
462472
{
463473
auto r2 = arena.remove_block(chunk_size(1));
464474
SNMALLOC_ASSERT(r2 != 0);
475+
UNUSED(r2);
465476
arena.check_invariant(true);
466477
remaining -= chunk_size(1);
467478
}
@@ -666,6 +677,7 @@ namespace snmalloc
666677
auto& bt0 = ArenaTestAccess::get_bin_trees(arena)[0];
667678
auto p3 = bt0.get_root_path();
668679
SNMALLOC_ASSERT(bt0.find(p3, chunk_addr(11)));
680+
UNUSED(p1, p2, p3);
669681

670682
size_t total = drain_arena(arena);
671683
SNMALLOC_ASSERT(total == 4);
@@ -1062,6 +1074,7 @@ namespace snmalloc
10621074

10631075
auto arena_result = arena.remove_block(chunk_size(n));
10641076
auto oracle_result = oracle.remove(n);
1077+
UNUSED(arena_result);
10651078

10661079
// Both should agree on success/failure.
10671080
if (oracle_result.second == 0)
@@ -1071,7 +1084,6 @@ namespace snmalloc
10711084
else
10721085
{
10731086
SNMALLOC_ASSERT(arena_result != 0);
1074-
// Arena should return the address oracle predicts.
10751087
SNMALLOC_ASSERT(
10761088
arena_result == chunk_addr(BASE + oracle_result.first));
10771089

@@ -1166,6 +1178,7 @@ namespace snmalloc
11661178
// B should now serve a size-12 request from the consolidated block.
11671179
uintptr_t r_addr = arena_b.remove_block(chunk_size(12));
11681180
SNMALLOC_ASSERT(r_addr == chunk_addr(BASE + 20));
1181+
UNUSED(r_addr);
11691182
arena_b.check_invariant(true);
11701183

11711184
printf(" Consolidation after migration: OK\n");
@@ -1262,6 +1275,7 @@ namespace snmalloc
12621275

12631276
auto arena_r = arena.remove_block(chunk_size(n));
12641277
auto oracle_r = oracle.remove(n);
1278+
UNUSED(arena_r);
12651279

12661280
if (oracle_r.second == 0)
12671281
{
@@ -1292,6 +1306,7 @@ namespace snmalloc
12921306
auto& dst_oracle = from_a ? oracle_b : oracle_a;
12931307
uint8_t src_id = from_a ? 1 : 2;
12941308
uint8_t dst_id = from_a ? 2 : 1;
1309+
UNUSED(src_id);
12951310

12961311
size_t n = (rng.next() % 3) + 1;
12971312
uintptr_t src_r = src.remove_block(chunk_size(n));
@@ -1375,6 +1390,7 @@ namespace snmalloc
13751390
SNMALLOC_ASSERT(r1_addr == p_addr);
13761391
auto r2_addr = arena.remove_block(chunk_size(2));
13771392
SNMALLOC_ASSERT(r2_addr == a_addr);
1393+
UNUSED(r1_addr, r2_addr);
13781394

13791395
printf(" Boundary blocks predecessor merge: OK\n");
13801396
}
@@ -1401,6 +1417,7 @@ namespace snmalloc
14011417
SNMALLOC_ASSERT(r1_addr == a_addr);
14021418
auto r2_addr = arena.remove_block(chunk_size(4));
14031419
SNMALLOC_ASSERT(r2_addr == s_addr);
1420+
UNUSED(r1_addr, r2_addr);
14041421

14051422
printf(" Boundary blocks successor merge: OK\n");
14061423
}
@@ -1427,6 +1444,7 @@ namespace snmalloc
14271444
SNMALLOC_ASSERT(r1_addr == chunk_addr(4));
14281445
auto r2_addr = arena.remove_block(chunk_size(2));
14291446
SNMALLOC_ASSERT(r2_addr == chunk_addr(8));
1447+
UNUSED(r1_addr, r2_addr);
14301448

14311449
printf(" Boundary partial (P merges, S blocked): OK\n");
14321450
}
@@ -1454,6 +1472,7 @@ namespace snmalloc
14541472

14551473
auto r1 = arena.remove_block(chunk_size(4));
14561474
SNMALLOC_ASSERT(r1 == top_addr);
1475+
UNUSED(r1);
14571476

14581477
printf(" Block at arena top edge: OK\n");
14591478
}
@@ -1480,6 +1499,7 @@ namespace snmalloc
14801499
SNMALLOC_ASSERT(
14811500
(r1_addr == p_addr && r2_addr == a_addr) ||
14821501
(r1_addr == a_addr && r2_addr == p_addr));
1502+
UNUSED(r1_addr, r2_addr);
14831503

14841504
printf(" Boundary blocks min predecessor merge: OK\n");
14851505
}

src/test/func/arenabins/arenabins.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -542,8 +542,7 @@ namespace
542542
/// (defined directly in terms of `bin_subsets`).
543543
template<size_t B>
544544
size_t reference_find(
545-
size_t n_chunks,
546-
const typename ArenaBinsTestAccess<B, 0>::Bitmap& bm)
545+
size_t n_chunks, const typename ArenaBinsTestAccess<B, 0>::Bitmap& bm)
547546
{
548547
using Bins = ArenaBinsTestAccess<B, 0>;
549548
using Bitmap = typename Bins::Bitmap;

0 commit comments

Comments
 (0)