littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers
BSD 3-Clause "New" or "Revised" License
4.9k stars 771 forks source link

Add some easier util overrides: LFS_MALLOC/FREE/CRC #909

Closed geky closed 5 months ago

geky commented 6 months ago

With this you can override littlefs's malloc/crc implementations with some simple defines:

-DLFS_MALLOC=my_malloc
-DLFS_FREE=my_free
-DLFS_CRC=my_crc

I think these are the main "system-level" utils that users want to override, but am open to feedback.


Note on LFS_CRC:

Overriding LFS_CRC with something that's not CRC32 is discouraged. Your filesystem will no longer be compatible with other tools. This is only intended for providing hardware acceleration, etc.


These are probably what most users expected when wanting to override malloc/free/crc in littlefs, but they haven't been available, since instead littlefs provides a file-level override of builtin utils behind LFS_CONFIG.

The thinking was that there's just too many builtins that could be overriden, lfs_max/min/alignup/npw2/etc/etc/etc, so allowing users to just override the util file provides the best flexibility without a ton of ifdefs.

But it's become clear this is awkward for users that just want to replace malloc/crc.

Maybe the original goal was too optimistic, maybe there's a better way to structure this file, or maybe the best API is just a bunch of ifdefs, I have no idea. This will hopefully continue to evolve.

geky-bot commented 6 months ago
Tests passed ✓, Code: 16820 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%) | | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16820 B (+0.0%) | 1448 B (+0.0%) | 800 B (+0.0%) | Lines | 2357/2533 lines (+0.0%) | | Readonly | 6130 B (+0.0%) | 448 B (+0.0%) | 800 B (+0.0%) | Branches | 1202/1528 branches (+0.0%) | | Threadsafe | 17688 B (+0.0%) | 1448 B (+0.0%) | 808 B (+0.0%) | | **Benchmarks** | | Multiversion | 16884 B (+0.0%) | 1448 B (+0.0%) | 804 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18500 B (+0.0%) | 1752 B (+0.0%) | 804 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17480 B (+0.0%) | 1440 B (+0.0%) | 800 B (+0.0%) | Erased | 1568888832 B (+0.0%) |
armandas commented 3 weeks ago

It seems this feature is one step short to being useful. Simply defining the macros results in an implicitly declared function, which the compiler then assumes is returning an int.

/.build/_deps/littlefs-src/lfs_util.h: In function 'lfs_malloc':
<command-line>: warning: implicit declaration of function 'pvPortMalloc' [-Wimplicit-function-declaration]
/.build/_deps/littlefs-src/lfs_util.h:229:12: note: in expansion of macro 'LFS_MALLOC'
  229 |     return LFS_MALLOC(size);
      |            ^~~~~~~~~~
<command-line>: warning: returning 'int' from a function with return type 'void *' makes pointer from integer without a cast [-Wint-conversion]
/.build/_deps/littlefs-src/lfs_util.h:229:12: note: in expansion of macro 'LFS_MALLOC'
  229 |     return LFS_MALLOC(size);
      |            ^~~~~~~~~~

I need to provide declarations of the functions, in case of FreeRTOS:

extern void * pvPortMalloc(size_t xSize);
extern void vPortFree(void * pv);

So basically this means I need to revert to copying lfs_util.h. Or did I miss something?

geky commented 1 week ago

Hmm, this is an interesting problem, thanks for pointing it out.

I think I only tested with symbols defined in system headers, so I didn't notice definitions would be a problem.

Unfortunately I'm not sure there's an easy solution. You can include arbitrary files in GCC/Clang with -include, though I think this is generally discouraged because of the impact on build times. If you control the build system you may be able to limit this to only littlefs's sources.

Worst case copying lfs_util.h still works. -DLFS_MALLOC, etc, may be limited to cases where modifying the build system is easier than modifying the a source file...

armandas commented 1 week ago

I was thinking it would be nice to allow user to include their own header through a define, a bit like LFS_CONFIG.

But I understand it's impossible to cover everyone's use cases and you have to draw a line somewhere.

geky commented 1 week ago

Yeah, I guess the question is what would make that include file different from LFS_CONFIG.

Maybe there's value in allowing an include file that is just prefixed to lfs_util.h. Such an include file could #define LFS_MALLOC, etc, directly, instead of relying on -D flags...

armandas commented 1 week ago

Yes, it would allow to keep the default functionality as opposed to the current "all or nothing" approach.