lfs_fs_preporphans makes invalid assumtions on bool type

[elupus]

In the function lfs_fs_preporphans, the value of lfs_gstate_hasorphans is left shifted 31 steps to set the high bit of the tag.

littlefs/lfs.c

Line 4895 in 8e251dd

((uint32_t)lfs_gstate_hasorphans(&lfs->gstate) << 31));

While lfs_gstate_hasorphans is defined as bool, it's value is actually same as lfs_tag_size, in other word 0 or any nonzero value.

littlefs/lfs.c

Line 420 in 8e251dd

return lfs_tag_size(a->tag);

This causes the high bit, to not be set at all. I've not investigated what issues this might cause. It was found while analyzing a weird clang-tidy warning:

"The result of the left shift is undefined due to shifting '0' by '31', which is unrepresentable in the unsigned version of the return type 'uint32_t'" on the left shift.

[elupus]

Should be noted that, for a compiler with _Bool support. This is unlikely to end up being a bug since that will ensure 0 and 1. But many embedded compilers will use int for bool types.

[elupus]

Should be noted. That the clang-tidy warning is likely a false positive. see llvm/llvm-project#56253. But might be better to just do hasorphans ? 0x8000000u : 0u anyway.

[geky]

Hi @elupus, thanks for creating a PR.

This has come up a couple times before (#772, #677), but I'm hesitant to change it as it would just hide other unintentional reliances on _Bool in the codebase.

lfs_init at least contains an assert to hopefully catch non-c99 conforming compilers:

littlefs/lfs.c

Lines 4208 to 4212 in 8e251dd

	// check that bool is a truthy-preserving type
	//
	// note the most common reason for this failure is a before-c99 compiler,
	// which littlefs currently does not support
	LFS_ASSERT((bool)0x80000000);

I'd be happy changing it if we actually tested with non-_Bool bool, but that raises the question what standard would we actually be testing for? c89 is high-effort low-value because of the amount of missing features.

[elupus]

Would you be ok with "hasorphans ? 0x8000000u : 0u" left shifting a boolean value is rather weird and will be an integer promotion.

[geky]

It does have an explicit cast though?

    ((uint32_t)lfs_gstate_hasorphans(&lfs->gstate) << 31));
     ^^^^^^^^^^

It's probably not worth caring about, but it saves a constant from a constant pool.

[elupus]

I will put up a pull later, then you can decide :).

Anyway, that assert would not catch the real bool case I think. I would beed to be
LFS_ASSERT(((bool)0x80000000) == (bool)1)

[geky]

Anyway, that assert would not catch the real bool case I think. I would beed to be
LFS_ASSERT(((bool)0x80000000) == (bool)1)

Oh, good point. typedef uint32_t bool would be weird, but not entirely unreasonable.

[BenBE]

There'S WORDBOOL on Win32 …

Anyway, writing !!cond ? true_val : false_val ist most clear AND should even catch most older compilers.

Regarding the standard: Are there any (good) reasons to stop below C99? And C99 guarantees a proper boolean type, thus resolving the whole problem …

[geky]

Regarding the standard: Are there any (good) reasons to stop below C99?

In theory niche embedded compilers, but I haven't really seen many requests for C89 that weren't fixed by changing a configuration. This may be because littlefs is only adopted in new designs, but it seems the embedded space is converging around C99 as a reasonable baseline.

C99 is also when variables could be declared whenever (including for loops), and // for comments. I like both those things.

It might come up if you try to compile littlefs as C++ code, but I would think the designated initializers would cause problems first. C99 was also when C++ and C started diverging heavily.