[1/8] malloc: implement USE_DL_PREFIX via inline functions

Message ID 20200219193934.28456-2-simon.k.r.goldschmidt@gmail.com
State New
Headers show
Series
  • malloc: implement USE_DL_PREFIX using inline functions
Related show

Commit Message

Simon Goldschmidt Feb. 19, 2020, 7:39 p.m.
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(-)

Comments

Simon Glass Feb. 20, 2020, 3:05 a.m. | #1
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>
Simon Goldschmidt March 4, 2020, 8:44 p.m. | #2
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>
>
Tom Rini March 4, 2020, 8:49 p.m. | #3
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?
Simon Glass March 4, 2020, 8:50 p.m. | #4
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

Patch

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