| Message ID | 20200219193934.28456-2-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | malloc: implement USE_DL_PREFIX using inline functions | expand |
On Wed, 19 Feb 2020 at 12:39, Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> wrote: > > Commit cfda60f99ae2 ("sandbox: Use a prefix for all allocation functions") > introduced preprocessor macros for malloc/free etc. > > This is bad practice as it essentially makes 'free' a reserved keyword and > resulted in quite a bit of renaming to avoid that reserved keyword. > > A better solution is to define the allocation functions as 'static inline'. > > As a side effect, exports.h must not export malloc/free for sandbox. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> > --- > > A side-effect is that exports.h may not declare malloc/free. I'm not really > sure if this is correct, but for sandbox, it should probably be ok? Is it possible to fix this? E.g. don't use inline for these two functions on sandbox? > > include/_exports.h | 2 ++ > include/exports.h | 2 ++ > include/malloc.h | 44 +++++++++++++++++++++++++++++--------------- > 3 files changed, 33 insertions(+), 15 deletions(-) Reviewed-by: Simon Glass <sjg at chromium.org>
Am 20.02.2020 um 04:05 schrieb Simon Glass: > On Wed, 19 Feb 2020 at 12:39, Simon Goldschmidt > <simon.k.r.goldschmidt at gmail.com> wrote: >> >> Commit cfda60f99ae2 ("sandbox: Use a prefix for all allocation functions") >> introduced preprocessor macros for malloc/free etc. >> >> This is bad practice as it essentially makes 'free' a reserved keyword and >> resulted in quite a bit of renaming to avoid that reserved keyword. >> >> A better solution is to define the allocation functions as 'static inline'. >> >> As a side effect, exports.h must not export malloc/free for sandbox. >> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> >> --- >> >> A side-effect is that exports.h may not declare malloc/free. I'm not really >> sure if this is correct, but for sandbox, it should probably be ok? > > Is it possible to fix this? E.g. don't use inline for these two > functions on sandbox? Not using inline for sandbox for these two is *not* an option as these two are exactly the ones offending globally known names. I guess we have to know what we want here: what is exports.h meant for? To me it looks like it is meant for "U-Boot API" applications to link against. If so, why is it included in U-Boot sources (in over 20 files)? I guess one solution would be to move (or copy) the DL prefix handling into exports.h and _exports.h so that applications linking with exports.h implicily call dlmalloc/dlfree, not malloc/free (which would be the OS versions for sandbox). I'll try to prepare v2 in that direction, but I'm still not unsure since exports.h is included in internal U-Boot code. Regards, Simon > >> >> include/_exports.h | 2 ++ >> include/exports.h | 2 ++ >> include/malloc.h | 44 +++++++++++++++++++++++++++++--------------- >> 3 files changed, 33 insertions(+), 15 deletions(-) > > Reviewed-by: Simon Glass <sjg at chromium.org> >
On Wed, Mar 04, 2020 at 09:44:27PM +0100, Simon Goldschmidt wrote: > Am 20.02.2020 um 04:05 schrieb Simon Glass: > > On Wed, 19 Feb 2020 at 12:39, Simon Goldschmidt > > <simon.k.r.goldschmidt at gmail.com> wrote: > >> > >> Commit cfda60f99ae2 ("sandbox: Use a prefix for all allocation functions") > >> introduced preprocessor macros for malloc/free etc. > >> > >> This is bad practice as it essentially makes 'free' a reserved keyword and > >> resulted in quite a bit of renaming to avoid that reserved keyword. > >> > >> A better solution is to define the allocation functions as 'static inline'. > >> > >> As a side effect, exports.h must not export malloc/free for sandbox. > >> > >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> > >> --- > >> > >> A side-effect is that exports.h may not declare malloc/free. I'm not really > >> sure if this is correct, but for sandbox, it should probably be ok? > > > > Is it possible to fix this? E.g. don't use inline for these two > > functions on sandbox? > > Not using inline for sandbox for these two is *not* an option as these > two are exactly the ones offending globally known names. > > I guess we have to know what we want here: what is exports.h meant for? > To me it looks like it is meant for "U-Boot API" applications to link > against. If so, why is it included in U-Boot sources (in over 20 files)? Yes, it's for API users. But, we also need to be sure our exported functions have the right calling interface. So I suspect a whole lot less files should be including it, and we could have a single place to confirm our exported functions have the right ABI, but not also use this file in so many places?
Hi Simon, On Wed, 4 Mar 2020 at 13:45, Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> wrote: > > Am 20.02.2020 um 04:05 schrieb Simon Glass: > > On Wed, 19 Feb 2020 at 12:39, Simon Goldschmidt > > <simon.k.r.goldschmidt at gmail.com> wrote: > >> > >> Commit cfda60f99ae2 ("sandbox: Use a prefix for all allocation functions") > >> introduced preprocessor macros for malloc/free etc. > >> > >> This is bad practice as it essentially makes 'free' a reserved keyword and > >> resulted in quite a bit of renaming to avoid that reserved keyword. > >> > >> A better solution is to define the allocation functions as 'static inline'. > >> > >> As a side effect, exports.h must not export malloc/free for sandbox. > >> > >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> > >> --- > >> > >> A side-effect is that exports.h may not declare malloc/free. I'm not really > >> sure if this is correct, but for sandbox, it should probably be ok? > > > > Is it possible to fix this? E.g. don't use inline for these two > > functions on sandbox? > > Not using inline for sandbox for these two is *not* an option as these > two are exactly the ones offending globally known names. > > I guess we have to know what we want here: what is exports.h meant for? > To me it looks like it is meant for "U-Boot API" applications to link > against. If so, why is it included in U-Boot sources (in over 20 files)? > > I guess one solution would be to move (or copy) the DL prefix handling > into exports.h and _exports.h so that applications linking with > exports.h implicily call dlmalloc/dlfree, not malloc/free (which would > be the OS versions for sandbox). > > I'll try to prepare v2 in that direction, but I'm still not unsure since > exports.h is included in internal U-Boot code. OK sounds good. I do wonder whether we should drop the exports.h inclusions as part of this? It does seem odd. OR perhaps they are there just to make sure that the functions match the API? Regards, Simon
diff --git a/include/_exports.h b/include/_exports.h index 0dee05f077..acfbf97c17 100644 --- a/include/_exports.h +++ b/include/_exports.h @@ -22,9 +22,11 @@ EXPORT_FUNC(dummy, void, install_hdlr, void) EXPORT_FUNC(dummy, void, free_hdlr, void) #endif +#ifndef CONFIG_SANDBOX EXPORT_FUNC(malloc, void *, malloc, size_t) #if !CONFIG_IS_ENABLED(SYS_MALLOC_SIMPLE) EXPORT_FUNC(free, void, free, void *) +#endif #endif EXPORT_FUNC(udelay, void, udelay, unsigned long) EXPORT_FUNC(get_timer, unsigned long, get_timer, unsigned long) diff --git a/include/exports.h b/include/exports.h index cbd16fc518..5d161824c8 100644 --- a/include/exports.h +++ b/include/exports.h @@ -25,10 +25,12 @@ void puts(const char*); int printf(const char* fmt, ...); void install_hdlr(int, interrupt_handler_t, void*); void free_hdlr(int); +#ifndef CONFIG_SANDBOX void *malloc(size_t); #if !CONFIG_IS_ENABLED(SYS_MALLOC_SIMPLE) void free(void*); #endif +#endif void __udelay(unsigned long); unsigned long get_timer(unsigned long); int vprintf(const char *, va_list); diff --git a/include/malloc.h b/include/malloc.h index f66c2e8617..50d4873b08 100644 --- a/include/malloc.h +++ b/include/malloc.h @@ -897,21 +897,6 @@ void malloc_simple_info(void); # define pvALLOc dlpvalloc # define mALLINFo dlmallinfo # define mALLOPt dlmallopt - -/* Ensure that U-Boot actually uses these too */ -#define calloc dlcalloc -#define free(ptr) dlfree(ptr) -#define malloc(x) dlmalloc(x) -#define memalign dlmemalign -#define realloc dlrealloc -#define valloc dlvalloc -#define pvalloc dlpvalloc -#define mallinfo() dlmallinfo() -#define mallopt dlmallopt -#define malloc_trim dlmalloc_trim -#define malloc_usable_size dlmalloc_usable_size -#define malloc_stats dlmalloc_stats - # else /* USE_DL_PREFIX */ # define cALLOc calloc # define fREe free @@ -966,6 +951,35 @@ void malloc_stats(); int mALLOPt(); struct mallinfo mALLINFo(); # endif + +# ifdef USE_DL_PREFIX +/* Ensure that U-Boot actually uses the redefined functions: */ +static inline void *calloc(size_t n, size_t elem_size) +{ + return dlcalloc(n, elem_size); +} + +static inline void free(void *ptr) { dlfree(ptr); } +static inline void *malloc(size_t bytes) { return dlmalloc(bytes); } +static inline void *memalign(size_t alignment, size_t bytes) +{ + return dlmemalign(alignment, bytes); +} + +static inline void *realloc(void *oldmem, size_t bytes) +{ + return dlrealloc(oldmem, bytes); +} + +static inline void *valloc(size_t bytes) { return dlvalloc(bytes); } +static inline void *pvalloc(size_t bytes) { return dlpvalloc(bytes); } +static inline struct mallinfo mallinfo(void) { return dlmallinfo(); } +static inline int mallopt(int param_number, int value) +{ + return dlmallopt(param_number, value); +} +# endif + #endif #pragma GCC visibility pop
Commit cfda60f99ae2 ("sandbox: Use a prefix for all allocation functions") introduced preprocessor macros for malloc/free etc. This is bad practice as it essentially makes 'free' a reserved keyword and resulted in quite a bit of renaming to avoid that reserved keyword. A better solution is to define the allocation functions as 'static inline'. As a side effect, exports.h must not export malloc/free for sandbox. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> --- A side-effect is that exports.h may not declare malloc/free. I'm not really sure if this is correct, but for sandbox, it should probably be ok? include/_exports.h | 2 ++ include/exports.h | 2 ++ include/malloc.h | 44 +++++++++++++++++++++++++++++--------------- 3 files changed, 33 insertions(+), 15 deletions(-)