Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 19, 2024
Merged

Conversation

geky
Copy link
Member

@geky geky commented Dec 19, 2023

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 added 2 commits December 19, 2023 13:57
Now you can override littlefs's malloc with some simple defines:

  -DLFS_MALLOC=my_malloc
  -DLFS_FREE=my_free

This is probably what most users expected when wanting to override
malloc/free in littlefs, but it hasn't been available, since instead
littlefs provides a file-level override of builtin utils.

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.

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.
Now you can override littlefs's CRC implementation with some simple
defines:

  -DLFS_CRC=lfs_crc

The motivation for this is the same for LFS_MALLOC/LFS_FREE. I think
these are the main "system-level" utils that users want to override.

Don't override with this something that's not CRC32! Your filesystem
will no longer be compatible with other tools! This is only intended for
provided hardware acceleration!
@geky-bot
Copy link
Collaborator

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%)

@geky geky added next minor and removed needs minor version new functionality only allowed in minor versions labels Jan 17, 2024
@geky geky added this to the v2.9 milestone Jan 17, 2024
@geky geky merged commit 1195d60 into devel Jan 19, 2024
32 checks passed
@geky geky mentioned this pull request Jan 19, 2024
@armandas
Copy link

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
Copy link
Member Author

geky commented Jun 25, 2024

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
Copy link

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
Copy link
Member Author

geky commented Jun 25, 2024

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
Copy link

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

@geky
Copy link
Member Author

geky commented Sep 19, 2024

@yamt put up a PR which improves this over here: #1004

Once released, LFS_DEFINES should let you prefix lfs_util.h with an arbitrary file without disabling the default builtins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants