From 9c2c101e7f90d76f0733f3f1ee5e83392f7a5b47 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 10 Jun 2024 00:40:49 -0500 Subject: [PATCH] Attempting to enable asserts-as-hints with isopen as an exception The theory is that lfsr_opened_isopen is a relatively special case, and the main cause of LFS_ASSERT side-effects. All other LFS_ASSERTs are either limited to simple expressions, or small static-inline functions. Unfortunately, even without lfsr_opened_isopen asserts, it seems asserts-as-hints still results in worse code/stack costs: code stack no-assert (before): 33626 2552 hint-assert (after): 33670 (+0.1%) 2592 (+1.6%) Digging around in the low-level assembly, it seems that what is happening is the increased number of calls to static-inline functions is causing the compiler to prefer to not-inline functions more often. Then, even if calls would be eliminated as dead-code, the damage is done to the containing function. It's not entirely clear how this could be avoided. Maybe a separate LFS_ASSERT for only pure expressions? This may not be worth trying to solve outside of the compiler... --- lfs.c | 93 ++++++++++++++++++++++++++++++++++++++---------------- lfs_util.h | 2 ++ 2 files changed, 68 insertions(+), 27 deletions(-) diff --git a/lfs.c b/lfs.c index 92d64cdb..f468009d 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); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { + LFS_ASSERT(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); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { + LFS_ASSERT(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); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { + LFS_ASSERT(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_); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { + LFS_ASSERT(err < 0); return err; } @@ -2352,8 +2352,6 @@ 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) { @@ -3584,11 +3582,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); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { if (err == LFS_ERR_NOENT) { break; } + LFS_ASSERT(err < 0); return err; } if (rid_ > a_rid+lfs_smax32(weight_-1, 0)) { @@ -3886,9 +3884,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__); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); + LFS_ASSERT(err < 0); return err; } @@ -4891,9 +4889,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__); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); + LFS_ASSERT(err < 0); return err; } @@ -4904,8 +4902,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); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { + LFS_ASSERT(err < 0); return err; } @@ -5152,13 +5150,17 @@ static bool lfsr_opened_isopen(lfs_t *lfs, const lfsr_opened_t *o) { } static void lfsr_opened_add(lfs_t *lfs, lfsr_opened_t *o) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(!lfsr_opened_isopen(lfs, o)); +#endif o->next = lfs->opened; lfs->opened = o; } static void lfsr_opened_remove(lfs_t *lfs, lfsr_opened_t *o) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, o)); +#endif for (lfsr_opened_t **o_ = &lfs->opened; *o_; o_ = &(*o_)->next) { if (*o_ == o) { *o_ = (*o_)->next; @@ -5583,7 +5585,6 @@ 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) { @@ -6362,11 +6363,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); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { if (err == LFS_ERR_NOENT) { break; } + LFS_ASSERT(err < 0); return err; } if (rid_ != a_rid) { @@ -6393,8 +6394,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); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { + LFS_ASSERT(err < 0); return err; } @@ -9689,8 +9690,10 @@ int lfsr_stat(lfs_t *lfs, const char *path, struct lfs_info *info) { static int lfsr_dir_rewind_(lfs_t *lfs, lfsr_dir_t *dir); int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) { +#ifndef LFS_NO_ASSERT // already open? LFS_ASSERT(!lfsr_opened_isopen(lfs, &dir->o)); +#endif // setup dir state dir->o.type = LFS_TYPE_DIR; @@ -9746,7 +9749,9 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) { } int lfsr_dir_close(lfs_t *lfs, lfsr_dir_t *dir) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &dir->o)); +#endif // remove from tracked mdirs lfsr_opened_remove(lfs, &dir->o); @@ -9754,7 +9759,9 @@ int lfsr_dir_close(lfs_t *lfs, lfsr_dir_t *dir) { } int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &dir->o)); +#endif // was our dir removed? if (lfsr_f_iszombie(dir->o.flags)) { @@ -9828,7 +9835,9 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) { } int lfsr_dir_seek(lfs_t *lfs, lfsr_dir_t *dir, lfs_soff_t off) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &dir->o)); +#endif // do nothing if removed if (lfsr_f_iszombie(dir->o.flags)) { @@ -9858,7 +9867,9 @@ int lfsr_dir_seek(lfs_t *lfs, lfsr_dir_t *dir, lfs_soff_t off) { lfs_soff_t lfsr_dir_tell(lfs_t *lfs, lfsr_dir_t *dir) { (void)lfs; +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &dir->o)); +#endif return dir->pos; } @@ -9891,7 +9902,9 @@ static int lfsr_dir_rewind_(lfs_t *lfs, lfsr_dir_t *dir) { } int lfsr_dir_rewind(lfs_t *lfs, lfsr_dir_t *dir) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &dir->o)); +#endif return lfsr_dir_rewind_(lfs, dir); } @@ -10038,8 +10051,10 @@ static lfs_ssize_t lfsr_bshrub_read(lfs_t *lfs, const lfsr_file_t *file, int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, const char *path, uint32_t flags, const struct lfs_file_config *cfg) { +#ifndef LFS_NO_ASSERT // already open? LFS_ASSERT(!lfsr_opened_isopen(lfs, &file->o)); +#endif // don't allow the forbidden mode! LFS_ASSERT((flags & 3) != 3); // these flags require a writable file @@ -10230,7 +10245,9 @@ int lfsr_file_open(lfs_t *lfs, lfsr_file_t *file, int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file); int lfsr_file_close(lfs_t *lfs, lfsr_file_t *file) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif // don't call lfsr_file_sync if we're readonly or desynced int err = 0; @@ -10282,8 +10299,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); - LFS_ASSERT(err <= 0); - if (err < 0 && err != LFS_ERR_NOENT) { + if (err && err != LFS_ERR_NOENT) { + LFS_ASSERT(err < 0); return err; } @@ -10298,8 +10315,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); - LFS_ASSERT(err <= 0); - if (err < 0) { + if (err) { + LFS_ASSERT(err < 0); return err; } @@ -11452,7 +11469,9 @@ fragment:; lfs_ssize_t lfsr_file_read(lfs_t *lfs, lfsr_file_t *file, void *buffer, lfs_size_t size) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif // can't read from writeonly files LFS_ASSERT(!lfsr_o_iswronly(file->o.flags)); LFS_ASSERT(file->pos + size <= 0x7fffffff); @@ -11544,7 +11563,9 @@ lfs_ssize_t lfsr_file_read(lfs_t *lfs, lfsr_file_t *file, lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file, const void *buffer, lfs_size_t size) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif // can't write to readonly files LFS_ASSERT(!lfsr_o_isrdonly(file->o.flags)); @@ -11699,7 +11720,9 @@ failed:; } int lfsr_file_flush(lfs_t *lfs, lfsr_file_t *file) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif // readonly files should do nothing LFS_ASSERT(!lfsr_o_isrdonly(file->o.flags) || !lfsr_f_isunflush(file->o.flags) @@ -11745,7 +11768,9 @@ failed:; } int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif // removed? we can't sync if (lfsr_f_iszombie(file->o.flags)) { return LFS_ERR_NOENT; @@ -11911,7 +11936,9 @@ failed:; int lfsr_file_desync(lfs_t *lfs, lfsr_file_t *file) { (void)lfs; +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif file->o.flags |= LFS_O_DESYNC; return 0; @@ -11919,7 +11946,9 @@ int lfsr_file_desync(lfs_t *lfs, lfsr_file_t *file) { lfs_soff_t lfsr_file_seek(lfs_t *lfs, lfsr_file_t *file, lfs_soff_t off, uint8_t whence) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif // TODO check for out-of-range? @@ -11947,14 +11976,18 @@ lfs_soff_t lfsr_file_seek(lfs_t *lfs, lfsr_file_t *file, lfs_soff_t lfsr_file_tell(lfs_t *lfs, lfsr_file_t *file) { (void)lfs; +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif return file->pos; } lfs_soff_t lfsr_file_rewind(lfs_t *lfs, lfsr_file_t *file) { (void)lfs; +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif file->pos = 0; return 0; @@ -11962,13 +11995,17 @@ lfs_soff_t lfsr_file_rewind(lfs_t *lfs, lfsr_file_t *file) { lfs_soff_t lfsr_file_size(lfs_t *lfs, lfsr_file_t *file) { (void)lfs; +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif return lfsr_file_size_(file); } int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif // exceeds our file limit? if (size_ > lfs->file_limit) { @@ -12073,7 +12110,9 @@ failed:; } int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { +#ifndef LFS_NO_ASSERT LFS_ASSERT(lfsr_opened_isopen(lfs, &file->o)); +#endif // exceeds our file limit? if (size_ > lfs->file_limit) { diff --git a/lfs_util.h b/lfs_util.h index 8f4f6f77..caf84b06 100644 --- a/lfs_util.h +++ b/lfs_util.h @@ -96,6 +96,8 @@ 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