From 3c13afd5c2e2edaf84317a96b8b7c2650bf7452f Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 21 Jan 2024 23:48:32 -0600 Subject: [PATCH] Added explicit test over unreachable tag holes Unreachable tag holes, null tags that _should_ be unreachable but actually are reachable, are an unfortunate quirk to our alt tag encoding. Because we only have an altgt, not altge, our "unreachable" tag ends up encoded with an altgt 0, an alt, which you may notice, does not guarantee unreachability. Fortunately, tag 0, the null tag, should intentionally be unused. So as long as we never lookup tag 0, nothing should break. If you do lookup tag 0, you end up with spurious null tags, which can complicate things. The solution here is a tag_ = max(tag, 1) in lfsr_rbyd_lookupnext. --- One interesting thing to note, as I was writing these tests I discovered that setting tag=max(tag,1) in lfsr_rbyd_appendattr had no effect. appendattr needs zip the rbyd tree to keep everything connected during range removals, so tag=0/tag=1 both end up with the same tree. So might as well drop the tag=max(tag,1) in lfsr_rbyd_appendattr. A side effect of this, both before and after this commit, is that any null tag holes created during range removals sort of stick around until the next compaction. --- Why altgt and not altge? altgt is the inverse of altle, requiring only a single bit flip to flip between the two. And trust me, it would be much more costly to make altle/altgt flips more complicated than a bit flip. --- Why altgt/altle and not altge/altlt? This is because our rbyds are right-leaning, that is, lookups always find the requested rid+tag, or the next smallest rid+tag. Consider a simple tree: <5 .----'| >=2 | |'-. | 1 2 5 What should lookup(3) return? If we are right-leaning, the answer _should_ be 5. But we need to take the <5 branch to determine if there is a hidden 3 or 4 in that subtree. altgt/altle does not have that problem: <=2 .----'| >1 | |'-. | 1 2 5 It might seem like you can workaround this by conservatively using the neighbor +1 as the alt target, but this runs into tag overflow problems. UATTR(0xff)+1 (0x057f+1) becomes UATTR(0x100) (0x0580) which is not allowed due to reserving bit 7 for future subtype extensions. Maybe you can workaround this workaround by using (tag+0x81)&~0x80 anywhere you need to increment (including lookupnext/iteration calls!), but this becomes a bit of a mess. And there are still concerns about overflows at the 0x77f boundary and 0xf7f boundary. --- lfs.c | 9 +- tests/test_rbyd.toml | 263 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 268 insertions(+), 4 deletions(-) diff --git a/lfs.c b/lfs.c index 4c833523..cd2dd022 100644 --- a/lfs.c +++ b/lfs.c @@ -968,6 +968,8 @@ static lfs_ssize_t lfsr_bd_progtag(lfs_t *lfs, // check for underflow issues LFS_ASSERT(weight < 0x80000000); LFS_ASSERT(size < 0x80000000); + // bit 7 is reserved for future subtype extensions + LFS_ASSERT(!(tag & 0x80)); // make sure to include the parity of the current crc tag |= (lfs_popc(*cksum_) & 1) << 15; @@ -2528,7 +2530,7 @@ static int lfsr_rbyd_appendattr(lfs_t *lfs, lfsr_rbyd_t *rbyd, LFS_ASSERT(lfsr_rbyd_isfetched(rbyd)); // tag must not be internal at this point LFS_ASSERT(!lfsr_tag_isinternal(tag)); - // reserve bit 7 to allow leb128 subtypes in the future + // bit 7 is reserved for future subtype extensions LFS_ASSERT(!(tag & 0x80)); // we can't do anything if we're not erased @@ -2577,8 +2579,7 @@ static int lfsr_rbyd_appendattr(lfs_t *lfs, lfsr_rbyd_t *rbyd, other_rid_ = rid; } - // note these tags MUST NOT be zero, due to unreachable tag holes - tag_ = 0x1; + tag_ = 0; other_tag_ = tag_; } else { @@ -2590,7 +2591,7 @@ static int lfsr_rbyd_appendattr(lfs_t *lfs, lfsr_rbyd_t *rbyd, // note both normal and rm wide-tags have the same bounds, really it's // the normal non-wide-tags that are an outlier here if (lfsr_tag_issupwide(tag)) { - tag_ = 0x1; + tag_ = 0; other_tag_ = tag_ + 0x800; } else if (lfsr_tag_issubwide(tag)) { tag_ = lfsr_tag_supkey(tag); diff --git a/tests/test_rbyd.toml b/tests/test_rbyd.toml index 43e89b83..4019f9c3 100644 --- a/tests/test_rbyd.toml +++ b/tests/test_rbyd.toml @@ -13071,3 +13071,266 @@ code = ''' free(backup_block); } ''' + + +# Some very specific cases we want to cover + +# One downside of having only altgt tags (not altge) is that we can end +# up with an awkward null tag in our rbyd. Need to test we handle this +# correctly. +[cases.test_rbyd_unreachable_hole] +in = 'lfs.c' +code = ''' + lfs_t lfs; + lfs_init(&lfs, CFG) => 0; + + lfsr_rbyd_t init_rbyd = { + .blocks[0] = 0, + .eoff = 0, + .cksum = 0, + .trunk = 0, + .weight = 0, + }; + lfsr_rbyd_t rbyd; + + // create a null tag hole + rbyd = init_rbyd; + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(-1, UATTR(0), 0, BUF("\xaa\xaa\xaa\xaa", 4)), + LFSR_ATTR(-1, UATTR(1), 0, BUF("\xbb\xbb\xbb\xbb", 4)), + LFSR_ATTR(-1, RM(UATTR(0)), 0, NULL()))) => 0; + assert(rbyd.weight == 0); + + // can we still access things? + lfs_ssize_t rid_; + lfsr_tag_t tag_; + lfs_size_t weight_; + lfsr_data_t data_; + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, 0, + &rid_, &tag_, &weight_, &data_) => 0; + assert(rid_ == -1); + assert(tag_ == LFSR_TAG_UATTR(1)); + assert(weight_ == 0); + assert(lfsr_data_size(&data_) == 4); + uint8_t rbuf[32]; + lfsr_data_read(&lfs, &data_, rbuf, 32) => 4; + assert(memcmp(rbuf, "\xbb\xbb\xbb\xbb", 4) == 0); + + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, LFSR_TAG_UATTR(1)+1, + &rid_, &tag_, &weight_, &data_) => LFS_ERR_NOENT; +''' + +[cases.test_rbyd_unreachable_hole_rm] +in = 'lfs.c' +code = ''' + lfs_t lfs; + lfs_init(&lfs, CFG) => 0; + + lfsr_rbyd_t init_rbyd = { + .blocks[0] = 0, + .eoff = 0, + .cksum = 0, + .trunk = 0, + .weight = 0, + }; + lfsr_rbyd_t rbyd; + + // create a null tag hole + rbyd = init_rbyd; + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(-1, UATTR(0), 0, BUF("\xaa\xaa\xaa\xaa", 4)), + LFSR_ATTR(-1, UATTR(1), 0, BUF("\xbb\xbb\xbb\xbb", 4)), + LFSR_ATTR(-1, UATTR(2), 0, BUF("\xcc\xcc\xcc\xcc", 4)), + LFSR_ATTR(-1, RM(UATTR(0)), 0, NULL()))) => 0; + assert(rbyd.weight == 0); + + // remove a neighbor to the hole + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(-1, RM(UATTR(1)), 0, NULL()))) => 0; + assert(rbyd.weight == 0); + + // can we still access things? + lfs_ssize_t rid_; + lfsr_tag_t tag_; + lfs_size_t weight_; + lfsr_data_t data_; + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, 0, + &rid_, &tag_, &weight_, &data_) => 0; + assert(rid_ == -1); + assert(tag_ == LFSR_TAG_UATTR(2)); + assert(weight_ == 0); + assert(lfsr_data_size(&data_) == 4); + uint8_t rbuf[32]; + lfsr_data_read(&lfs, &data_, rbuf, 32) => 4; + assert(memcmp(rbuf, "\xcc\xcc\xcc\xcc", 4) == 0); + + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, LFSR_TAG_UATTR(2)+1, + &rid_, &tag_, &weight_, &data_) => LFS_ERR_NOENT; +''' + +[cases.test_rbyd_unreachable_hole_delete] +in = 'lfs.c' +code = ''' + lfs_t lfs; + lfs_init(&lfs, CFG) => 0; + + lfsr_rbyd_t init_rbyd = { + .blocks[0] = 0, + .eoff = 0, + .cksum = 0, + .trunk = 0, + .weight = 0, + }; + lfsr_rbyd_t rbyd; + + // create a null tag hole + rbyd = init_rbyd; + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(-1, UATTR(0), 0, BUF("\xaa\xaa\xaa\xaa", 4)), + LFSR_ATTR(0, REG, +1, BUF("\xbb\xbb\xbb\xbb", 4)), + LFSR_ATTR(1, REG, +1, BUF("\xcc\xcc\xcc\xcc", 4)), + LFSR_ATTR(-1, RM(UATTR(0)), 0, NULL()))) => 0; + assert(rbyd.weight == 2); + + // delete a neighbor to the hole + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(0, RM, -1, NULL()))) => 0; + assert(rbyd.weight == 1); + + // can we still access things? + lfs_ssize_t rid_; + lfsr_tag_t tag_; + lfs_size_t weight_; + lfsr_data_t data_; + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, 0, + &rid_, &tag_, &weight_, &data_) => 0; + assert(rid_ == 0); + assert(tag_ == LFSR_TAG_REG); + assert(weight_ == 1); + assert(lfsr_data_size(&data_) == 4); + uint8_t rbuf[32]; + lfsr_data_read(&lfs, &data_, rbuf, 32) => 4; + assert(memcmp(rbuf, "\xcc\xcc\xcc\xcc", 4) == 0); + + lfsr_rbyd_lookupnext(&lfs, &rbyd, 0, LFSR_TAG_REG+1, + &rid_, &tag_, &weight_, &data_) => LFS_ERR_NOENT; +''' + +[cases.test_rbyd_unreachable_hole_subwide] +in = 'lfs.c' +code = ''' + lfs_t lfs; + lfs_init(&lfs, CFG) => 0; + + lfsr_rbyd_t init_rbyd = { + .blocks[0] = 0, + .eoff = 0, + .cksum = 0, + .trunk = 0, + .weight = 0, + }; + lfsr_rbyd_t rbyd; + + // create a null tag hole + rbyd = init_rbyd; + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(-1, UATTR(0), 0, BUF("\xaa\xaa\xaa\xaa", 4)), + LFSR_ATTR(-1, UATTR(1), 0, BUF("\xbb\xbb\xbb\xbb", 4)), + LFSR_ATTR(-1, SATTR(0), 0, BUF("\xdd\xdd\xdd\xdd", 4)), + LFSR_ATTR(-1, RM(UATTR(0)), 0, NULL()))) => 0; + assert(rbyd.weight == 0); + + // subwide replace a neighbor to the hole + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(-1, + SUBWIDE(UATTR(2)), 0, + BUF("\xcc\xcc\xcc\xcc", 4)))) => 0; + assert(rbyd.weight == 0); + + // can we still access things? + lfs_ssize_t rid_; + lfsr_tag_t tag_; + lfs_size_t weight_; + lfsr_data_t data_; + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, 0, + &rid_, &tag_, &weight_, &data_) => 0; + assert(rid_ == -1); + assert(tag_ == LFSR_TAG_UATTR(2)); + assert(weight_ == 0); + assert(lfsr_data_size(&data_) == 4); + uint8_t rbuf[32]; + lfsr_data_read(&lfs, &data_, rbuf, 32) => 4; + assert(memcmp(rbuf, "\xcc\xcc\xcc\xcc", 4) == 0); + + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, LFSR_TAG_UATTR(2)+1, + &rid_, &tag_, &weight_, &data_) => 0; + assert(rid_ == -1); + assert(tag_ == LFSR_TAG_SATTR(0)); + assert(weight_ == 0); + assert(lfsr_data_size(&data_) == 4); + lfsr_data_read(&lfs, &data_, rbuf, 32) => 4; + assert(memcmp(rbuf, "\xdd\xdd\xdd\xdd", 4) == 0); + + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, LFSR_TAG_SATTR(0)+1, + &rid_, &tag_, &weight_, &data_) => LFS_ERR_NOENT; +''' + +[cases.test_rbyd_unreachable_hole_supwide] +in = 'lfs.c' +code = ''' + lfs_t lfs; + lfs_init(&lfs, CFG) => 0; + + lfsr_rbyd_t init_rbyd = { + .blocks[0] = 0, + .eoff = 0, + .cksum = 0, + .trunk = 0, + .weight = 0, + }; + lfsr_rbyd_t rbyd; + + // create a null tag hole + rbyd = init_rbyd; + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(-1, UATTR(0), 0, BUF("\xaa\xaa\xaa\xaa", 4)), + LFSR_ATTR(-1, UATTR(1), 0, BUF("\xbb\xbb\xbb\xbb", 4)), + LFSR_ATTR(0, REG, +1, BUF("\xdd\xdd\xdd\xdd", 4)), + LFSR_ATTR(-1, RM(UATTR(0)), 0, NULL()))) => 0; + assert(rbyd.weight == 1); + + // supwide replace a neighbor to the hole + lfsr_rbyd_commit(&lfs, &rbyd, LFSR_ATTRS( + LFSR_ATTR(-1, + SUPWIDE(UATTR(2)), 0, + BUF("\xcc\xcc\xcc\xcc", 4)))) => 0; + assert(rbyd.weight == 1); + + // can we still access things? + lfs_ssize_t rid_; + lfsr_tag_t tag_; + lfs_size_t weight_; + lfsr_data_t data_; + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, 0, + &rid_, &tag_, &weight_, &data_) => 0; + assert(rid_ == -1); + assert(tag_ == LFSR_TAG_UATTR(2)); + assert(weight_ == 0); + assert(lfsr_data_size(&data_) == 4); + uint8_t rbuf[32]; + lfsr_data_read(&lfs, &data_, rbuf, 32) => 4; + assert(memcmp(rbuf, "\xcc\xcc\xcc\xcc", 4) == 0); + + lfsr_rbyd_lookupnext(&lfs, &rbyd, -1, LFSR_TAG_UATTR(2)+1, + &rid_, &tag_, &weight_, &data_) => 0; + assert(rid_ == 0); + assert(tag_ == LFSR_TAG_REG); + assert(weight_ == 1); + assert(lfsr_data_size(&data_) == 4); + lfsr_data_read(&lfs, &data_, rbuf, 32) => 4; + assert(memcmp(rbuf, "\xdd\xdd\xdd\xdd", 4) == 0); + + lfsr_rbyd_lookupnext(&lfs, &rbyd, 0, LFSR_TAG_REG+1, + &rid_, &tag_, &weight_, &data_) => LFS_ERR_NOENT; +''' +