Github/littlefs
1
Fork 0
Code Actions Activity

Dropped LFS_ASSERT as a compiler hint

Browse Source 
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...
This commit is contained in:
Christopher Haster
2024-06-09 22:09:20 -05:00
parent fd41d296db
commit b563050fc8
2 changed files with 27 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+27 -24
View File
@@ -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;
}
lfs_util.h
-2
View File
@@ -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