Hi, On 5 February 2015 at 10:53, Hans de Goede <hdego...@redhat.com> wrote: > Hi, > > > On 05-02-15 12:24, Siarhei Siamashka wrote: >> >> On Wed, 4 Feb 2015 13:05:50 +0100 >> Hans de Goede <hdego...@redhat.com> wrote: >> >>> All callers of malloc should already do error checking, and may even be >>> able >>> to continue without the alloc succeeding. >>> >>> Moreover, common/malloc_simple.c is the only user of .rodata.str1.1 in >>> common/built-in.o when building the SPL, triggering this gcc bug: >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54303 >>> >>> Causing .rodata to grow with e.g. 0xc21 bytes, nullifying all benefits of >>> using malloc_simple in the first place. >>> >>> Signed-off-by: Hans de Goede <hdego...@redhat.com> >>> --- >>> common/malloc_simple.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/common/malloc_simple.c b/common/malloc_simple.c >>> index afdacff..64ae036 100644 >>> --- a/common/malloc_simple.c >>> +++ b/common/malloc_simple.c >>> @@ -19,7 +19,7 @@ void *malloc_simple(size_t bytes) >>> >>> new_ptr = gd->malloc_ptr + bytes; >>> if (new_ptr > gd->malloc_limit) >>> - panic("Out of pre-reloc memory"); >>> + return NULL; >>> ptr = map_sysmem(gd->malloc_base + gd->malloc_ptr, bytes); >>> gd->malloc_ptr = ALIGN(new_ptr, sizeof(new_ptr)); >>> return ptr; >> >> >> The other patches look great, but I'm not convinced that requiring the >> malloc callers to do error checking is such a great idea. This means a >> lot of checks in a lot of places with extra code paths instead of just >> a single check in one place for the "impossible to happen" critical >> failure. I think that we should normally assume that malloc always >> succeeds in the production code and the panic may only happen while >> debugging. >> >> If the malloc pool is in the DRAM, then we usually have orders of >> magnitude more space than necessary. While the code might be still >> in the SRAM at the same time (the extra branching code logic for >> errors checking may be wasting the scarce SRAM space). >> >> If the malloc pool is in the SRAM and potentially may fail allocations, >> then this is a major reliability problem by itself. The malloc pool is >> always inefficient, has fragmentation problems, etc. If this is the >> case, then IMHO the only right solution is to replace such problematic >> dynamic allocations with static reservations in the ".data" section. >> Otherwise the reliability critical things (something like Mars rovers >> for example) will be sometimes failing. The Murphy law exists for >> a reason :-) > > > Actually we had a discussion about this at the u-boot miniconf at ELCE, > I was actually advocating for having malloc behave as gmalloc from > glib2 does, so basically what you're advocating for, but I lost the > discussion. All this patch does is bring the simple, light-weight malloc > from malloc_simple.c inline with the regular malloc implementation which > can already fail. > >> The workaround for the GCC compiler bug is orthogonal to this. >> Maybe there is some other solution? > > > To workaround this gcc bug we need to not use any const strings. > I guess we could do something like this and still panic (): > > char str[4]; > > str[0] = 'O'; > str[1] = 'O'; > str[2] = 'M'; > str[3] = 0; > > But I think that just removing the panic is better, as it makes > malloc_simple.c consistent with the regular malloc. In the end > we should really get the gcc bug fixed, this will likely > trim down .rodata significantly.
Yes I'd like to apply this one. It works around a really annoying gcc bug. Hans I think you are right that we can deal with the errors at the top level, as we do with other things. SPL error handling is probably not great, but that's a separate issue. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
