Skip to content

Commit 6ee6234

Browse files
committed
cachedb_perf: say uint64_t where 64 bits are meant, for ILP32 targets
The arm32 CI jobs fail to build pcache_htable.c with six -Werror errors, while arm64 and every native job pass. The difference is not the architecture but the data model: `unsigned long` is 64 bits on LP64 and 32 on ILP32, and two places in this file genuinely need 64. The routing word packs the linear-hashing state as (level << 32) | split. Declared `unsigned long`, so on arm32 or i386 every shift is undefined and the packing collapses into itself - which is exactly what "left/right shift count >= width of type" reports at :105, :1451, :1507, :1509 and :1599. The SWAR tag scan is the same mistake with worse consequences. tag_matches() memcpy()s the eight tag bytes into an `unsigned long` and masks with 0x0101010101010101. On ILP32 the destination is four bytes - clang says "'memcpy' will always overflow; destination buffer has size 4, but size argument is 8" - and every constant is silently truncated, so the filter that is supposed to reject 255 of 256 slots would return garbage instead of failing loudly. Both become uint64_t, which is what the code always meant. Every other `unsigned long` here is a size or a pointer-range bound, correct under both models, and is deliberately left alone. No change on LP64, where the two types are the same width - the fleet binaries differ only by the name of a type. The routing word was already being read with __atomic_load_n; ARMv7 has LDREXD/STREXD so an 8-byte atomic stays lock-free there, and where it is not the compiler emits a libatomic call, which is still correct. Verified with `gcc -m32`, which reproduces the identical six errors before the change and none after, with the native 64-bit build clean throughout. The other cachedb_perf sources were swept the same way and are unaffected.
1 parent e7529c8 commit 6ee6234

2 files changed

Lines changed: 26 additions & 18 deletions

File tree

‎modules/cachedb_perf/pcache_htable.c‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,11 @@ static inline unsigned char tag_of(unsigned int h)
9797
}
9898

9999
static inline unsigned int route_idx(pcache_htable_t *ht, unsigned int h,
100-
unsigned long *route_out)
100+
uint64_t *route_out)
101101
{
102102
/* acquire pairs with the release-publish in pcache_ht_split: seeing a
103103
* new routing word implies the partner bucket's slots are visible */
104-
unsigned long r = __atomic_load_n(&ht->route, __ATOMIC_ACQUIRE);
104+
uint64_t r = __atomic_load_n(&ht->route, __ATOMIC_ACQUIRE);
105105
unsigned int level = (unsigned int)(r >> 32);
106106
unsigned int split = (unsigned int)r;
107107
unsigned int idx = h & ((1U << level) - 1);
@@ -148,14 +148,18 @@ static inline void hint_update(pcache_htable_t *ht, unsigned int idx,
148148
/* one 8-byte load of tags[6]+meta; 0x80 at byte i = tags[i] matches.
149149
* The SWAR borrow can produce a false positive after a true match byte -
150150
* filtered by the key compare, never a false negative. */
151-
static inline unsigned long tag_matches(const pcache_bucket_t *b,
151+
static inline uint64_t tag_matches(const pcache_bucket_t *b,
152152
unsigned char tag)
153153
{
154-
unsigned long w, x;
154+
/* uint64_t, not unsigned long: this word IS the 8 tag bytes, and the
155+
* masks below are 64-bit constants. On an ILP32 target unsigned long
156+
* is 4 bytes, so the memcpy would overflow and every constant would be
157+
* truncated - the filter would return garbage rather than fail loudly. */
158+
uint64_t w, x;
155159

156160
memcpy(&w, b->tags, 8);
157-
x = w ^ (0x0101010101010101UL * tag);
158-
return (x - 0x0101010101010101UL) & ~x & 0x0000808080808080UL;
161+
x = w ^ (0x0101010101010101ULL * tag);
162+
return (x - 0x0101010101010101ULL) & ~x & 0x0000808080808080ULL;
159163
}
160164

161165
/* meta helpers - writers only, under the bucket lock */
@@ -304,7 +308,7 @@ static int _pcache_ht_fetch_buf(pcache_htable_t *ht, const str *key,
304308
unsigned int now, unsigned int *exp_out, long long *ll_out)
305309
{
306310
pcache_bucket_t *b;
307-
unsigned long route;
311+
uint64_t route;
308312
unsigned int hash, idx, v1, v2, vlen = 0, exp = 0, tries;
309313
unsigned char tag, fl = 0;
310314
long long ll;
@@ -564,7 +568,7 @@ int pcache_ht_store(pcache_htable_t *ht, const str *key, const str *val,
564568
pcache_bucket_t *b;
565569
pcache_rec_t *nr, *old = NULL;
566570
struct povf *node = NULL, *on;
567-
unsigned long route;
571+
uint64_t route;
568572
unsigned int hash, idx, used;
569573
unsigned char tag;
570574
int i, inserted = 0;
@@ -729,7 +733,7 @@ int pcache_ht_add(pcache_htable_t *ht, const str *key, long long delta,
729733
pcache_bucket_t *b;
730734
pcache_rec_t *nr, *r, *old = NULL;
731735
struct povf *node = NULL, *on;
732-
unsigned long route;
736+
uint64_t route;
733737
unsigned int hash, idx, used;
734738
unsigned char tag;
735739
long long cur;
@@ -897,7 +901,7 @@ int pcache_ht_touch(pcache_htable_t *ht, const str *key, unsigned int expires)
897901
pcache_bucket_t *b;
898902
pcache_rec_t *r;
899903
struct povf *on;
900-
unsigned long route;
904+
uint64_t route;
901905
unsigned int hash, idx;
902906
unsigned char tag;
903907
int i, rc = 0;
@@ -952,7 +956,7 @@ int pcache_ht_remove(pcache_htable_t *ht, const str *key)
952956
pcache_bucket_t *b;
953957
pcache_rec_t *dead = NULL;
954958
struct povf *on = NULL, **prev;
955-
unsigned long route;
959+
uint64_t route;
956960
unsigned int hash, idx, used;
957961
unsigned char tag;
958962
int i;
@@ -1447,7 +1451,7 @@ static int ensure_segment(pcache_htable_t *ht, unsigned int idx)
14471451
*/
14481452
static int pcache_ht_split(pcache_htable_t *ht)
14491453
{
1450-
unsigned long r = ht->route, nr;
1454+
uint64_t r = ht->route, nr;
14511455
unsigned int level = (unsigned int)(r >> 32);
14521456
unsigned int split = (unsigned int)r;
14531457
unsigned int sidx = split, pidx = split + (1U << level);
@@ -1504,9 +1508,9 @@ static int pcache_ht_split(pcache_htable_t *ht)
15041508
* moved key is guaranteed to see the new route on its 3.4 re-read
15051509
* and re-route to the partner - no false-miss window. */
15061510
if (split + 1 == (1U << level))
1507-
nr = (unsigned long)(level + 1) << 32; /* level up, split 0 */
1511+
nr = (uint64_t)(level + 1) << 32; /* level up, split 0 */
15081512
else
1509-
nr = ((unsigned long)level << 32) | (split + 1);
1513+
nr = ((uint64_t)level << 32) | (split + 1);
15101514
__atomic_store_n(&ht->route, nr, __ATOMIC_RELEASE);
15111515
ht->nbuckets++;
15121516

@@ -1596,7 +1600,7 @@ pcache_htable_t *pcache_htable_new(unsigned int size_log2)
15961600
return NULL;
15971601

15981602
ht->nbuckets = nbuckets;
1599-
ht->route = (unsigned long)size_log2 << 32;
1603+
ht->route = (uint64_t)size_log2 << 32;
16001604

16011605
LM_DBG("table ready: %u buckets in %u segments\n", nbuckets, s);
16021606
return ht;
@@ -1645,7 +1649,7 @@ static int st_walk_cb(const str *key, const str *val, unsigned int exp,
16451649

16461650
static pcache_rec_t *st_slot_of(pcache_htable_t *ht, const str *key)
16471651
{
1648-
unsigned long route;
1652+
uint64_t route;
16491653
unsigned int hash = core_hash((str *)key, NULL, 0);
16501654
pcache_bucket_t *b = bucket_at(ht, route_idx(ht, hash, &route));
16511655
int i = find_slot(b, key, hash, tag_of(hash));
@@ -1659,7 +1663,7 @@ int pcache_htable_selftest(void)
16591663
pcache_rec_t *r0, *r1;
16601664
pcache_bucket_t *b;
16611665
str k, v, out;
1662-
unsigned long route;
1666+
uint64_t route;
16631667
unsigned int i, ver0, ver1, nb_used;
16641668
char kb[32], vb[512];
16651669
int rc;

‎modules/cachedb_perf/pcache_htable.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#ifndef _PCACHE_HTABLE_H_
2424
#define _PCACHE_HTABLE_H_
2525

26+
#include <stdint.h>
2627
#include <stddef.h>
2728

2829
#include "../../str.h"
@@ -135,7 +136,10 @@ typedef struct pcache_ht_totals {
135136
typedef struct pcache_htable {
136137
/* the 3.4 routing word: (level << 32) | split, published whole.
137138
* On its own line - everything else here mutates */
138-
volatile unsigned long route;
139+
/* (level << 32) | split - genuinely 64 bits, so NOT unsigned long:
140+
* that is 32 bits on every ILP32 target (arm32, i386) and the packing
141+
* would collapse silently. */
142+
volatile uint64_t route;
139143
char _pad0[56];
140144

141145
unsigned int nbuckets;

0 commit comments

Comments
 (0)