From b563050fc8687ea9a3266b4435e246c0a03e8982 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 9 Jun 2024 22:09:20 -0500 Subject: [PATCH] Dropped LFS_ASSERT as a compiler hint I realized the reason asserting on opened/closed file handles added so much extra code was because our LFS_ASSERT macro doesn't properly eliminate side-effects. Consider this assert: LFS_ASSERT(lfsr_opened_isopen(lfs, &dir->o)); Expanded: ((lfsr_opened_isopen(lfs, &dir->o)) ? (void)0 : __builtin_unreachable()); Even though the compiler knows lfsr_opened_isopen must return true here, it doesn't know what possible side-effects calling lfsr_opened_isopen may have, and can't eliminate the function call. This is quite a bit more obvious if you did something like: LFS_ASSERT(lfsr_file_sync(&lfs, &file) == 0); But since lfsr_opened_isopen is a static inline function, it gets a little bit less clear. Even worse, whether or not the call is eliminated probably depends if it's actually inlined and other compiler optimization noise. --- This commit effectively reverts LFS_ASSERT as a compiler hint, making LFS_ASSERT an empty string if LFS_NO_ASSERT is defined. This may not be the optimal solution, but it at least keeps the programmer's intuition that anything in LFS_ASSERT has zero impact when asserts are disabled. It would be nice if there was some way to tell the compiler that an expression should have no side-effects, but as far as I'm aware this is not currently possible. Measurements show that just disabling asserts wins in both code and stack over trying to leverage asserts-as-hints: code stack hint-assert (before): 33904 2584 no-assert (after): 33626 (-0.8%) 2552 (-1.2%) At least both of these win over leaving asserts enabled, so asserts-as-hints does eliminate _most_ code (measured here with a simple assert-loop, since I assume that would have the smallest code footprint): code stack loop-assert: 36874 2616 hint-assert (before): 33904 (-8.1%) 2584 (-1.2%) no-assert (after): 33626 (-8.8%) 2552 (-2.4%) Unfortunately asserts-as-hints was doing quite a bit of heavy lifting at preventing overzealous GCC warnings. It took quite a few tweaks to get GCC to shut up, and I'm still not entirely sure the best way to tell GCC that some functions only return negative values. Currently I just limit certain error checks to only check for negative values, which is not great, but at least gets the code compiling again... --- lfs.c | 51 +++++++++++++++++++++++++++------------------------ lfs_util.h | 2 -- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/lfs.c b/lfs.c index 1314deb7..92d64cdb 100644 --- a/lfs.c +++ b/lfs.c @@ -1140,8 +1140,8 @@ static lfs_ssize_t lfsr_bd_readtag(lfs_t *lfs, } int err = lfsr_bd_read(lfs, block, off, hint, tag_buf, tag_dsize); - if (err) { - LFS_ASSERT(err < 0); + LFS_ASSERT(err <= 0); + if (err < 0) { return err; } @@ -1232,8 +1232,8 @@ static lfs_ssize_t lfsr_bd_progtag(lfs_t *lfs, int err = lfsr_bd_prog(lfs, block, off, tag_buf, d, cksum_, align); - if (err) { - LFS_ASSERT(err < 0); + LFS_ASSERT(err <= 0); + if (err < 0) { return err; } @@ -1328,8 +1328,8 @@ static lfs_ssize_t lfsr_data_read(lfs_t *lfs, lfsr_data_t *data, // note our hint includes the full data range lfsr_data_size(*data), buffer, d); - if (err) { - LFS_ASSERT(err < 0); + LFS_ASSERT(err <= 0); + if (err < 0) { return err; } @@ -1443,8 +1443,8 @@ static lfs_scmp_t lfsr_data_namecmp(lfs_t *lfs, lfsr_data_t data, // first compare the did lfsr_did_t did_; int err = lfsr_data_readleb128(lfs, &data, &did_); - if (err) { - LFS_ASSERT(err < 0); + LFS_ASSERT(err <= 0); + if (err < 0) { return err; } @@ -2352,6 +2352,8 @@ static int lfsr_rbyd_fetch(lfs_t *lfs, lfsr_rbyd_t *rbyd, static int lfsr_rbyd_fetchvalidate(lfs_t *lfs, lfsr_rbyd_t *rbyd, lfs_block_t block, lfs_size_t trunk, lfsr_rid_t weight, uint32_t cksum) { + (void)weight; + int err = lfsr_rbyd_fetch(lfs, rbyd, block, trunk); if (err) { if (err == LFS_ERR_CORRUPT) { @@ -3582,11 +3584,11 @@ static lfs_ssize_t lfsr_rbyd_estimate(lfs_t *lfs, const lfsr_rbyd_t *rbyd, int err = lfsr_rbyd_lookupnext(lfs, rbyd, a_rid, tag+1, &rid_, &tag, &weight_, &data); - if (err) { + LFS_ASSERT(err <= 0); + if (err < 0) { if (err == LFS_ERR_NOENT) { break; } - LFS_ASSERT(err < 0); return err; } if (rid_ > a_rid+lfs_smax32(weight_-1, 0)) { @@ -3884,9 +3886,9 @@ static lfs_scmp_t lfsr_rbyd_namelookup(lfs_t *lfs, const lfsr_rbyd_t *rbyd, // of a weighted rid with this lower_rid + (upper_rid-1-lower_rid)/2, 0, &rid__, &tag__, &weight__, &data__); - if (err) { + LFS_ASSERT(err <= 0); + if (err < 0) { LFS_ASSERT(err != LFS_ERR_NOENT); - LFS_ASSERT(err < 0); return err; } @@ -4889,9 +4891,9 @@ static lfs_scmp_t lfsr_btree_namelookup(lfs_t *lfs, const lfsr_btree_t *btree, lfsr_data_t data__; int err = lfsr_rbyd_sublookup(lfs, &branch, rid__, LFSR_TAG_STRUCT, &tag__, &data__); - if (err) { + LFS_ASSERT(err <= 0); + if (err < 0) { LFS_ASSERT(err != LFS_ERR_NOENT); - LFS_ASSERT(err < 0); return err; } @@ -4902,8 +4904,8 @@ static lfs_scmp_t lfsr_btree_namelookup(lfs_t *lfs, const lfsr_btree_t *btree, // fetch the next branch err = lfsr_data_readbranch(lfs, &data__, weight__, &branch); - if (err) { - LFS_ASSERT(err < 0); + LFS_ASSERT(err <= 0); + if (err < 0) { return err; } @@ -5581,6 +5583,7 @@ static void lfsr_fs_revertgdelta(lfs_t *lfs) { &LFSR_DATA_BUF(lfs->grm_p, LFSR_GRM_DSIZE), &lfs->grm); LFS_ASSERT(!err); + (void)err; } static void lfsr_fs_commitgdelta(lfs_t *lfs) { @@ -6359,11 +6362,11 @@ static lfs_ssize_t lfsr_mdir_estimate__(lfs_t *lfs, const lfsr_mdir_t *mdir, int err = lfsr_rbyd_lookupnext(lfs, &mdir->rbyd, a_rid, tag+1, &rid_, &tag, NULL, &data); - if (err) { + LFS_ASSERT(err <= 0); + if (err < 0) { if (err == LFS_ERR_NOENT) { break; } - LFS_ASSERT(err < 0); return err; } if (rid_ != a_rid) { @@ -6390,8 +6393,8 @@ static lfs_ssize_t lfsr_mdir_estimate__(lfs_t *lfs, const lfsr_mdir_t *mdir, lfsr_shrub_t shrub; err = lfsr_data_readshrub(lfs, &data, mdir, &shrub); - if (err) { - LFS_ASSERT(err < 0); + LFS_ASSERT(err <= 0); + if (err < 0) { return err; } @@ -10279,8 +10282,8 @@ static lfs_ssize_t lfsr_bshrub_estimate(lfs_t *lfs, const lfsr_file_t *file) { lfsr_data_t data; int err = lfsr_mdir_lookupnext(lfs, &file->o.mdir, LFSR_TAG_DATA, &tag, &data); - if (err && err != LFS_ERR_NOENT) { - LFS_ASSERT(err < 0); + LFS_ASSERT(err <= 0); + if (err < 0 && err != LFS_ERR_NOENT) { return err; } @@ -10295,8 +10298,8 @@ static lfs_ssize_t lfsr_bshrub_estimate(lfs_t *lfs, const lfsr_file_t *file) { lfsr_shrub_t shrub; err = lfsr_data_readshrub(lfs, &data, &file->o.mdir, &shrub); - if (err) { - LFS_ASSERT(err < 0); + LFS_ASSERT(err <= 0); + if (err < 0) { return err; } diff --git a/lfs_util.h b/lfs_util.h index caf84b06..8f4f6f77 100644 --- a/lfs_util.h +++ b/lfs_util.h @@ -96,8 +96,6 @@ extern "C" #ifndef LFS_ASSERT #ifndef LFS_NO_ASSERT #define LFS_ASSERT(test) assert(test) -#elif !defined(LFS_NO_BUILTINS) -#define LFS_ASSERT(test) ((test) ? (void)0 : __builtin_unreachable()) #else #define LFS_ASSERT(test) #endif