On Wed, Sep 04, 2019 at 01:46:52PM +0000, Alexey Brodkin wrote: > Hi Faiz, > > > -----Original Message----- > > From: Alexey Brodkin > > Sent: Wednesday, September 4, 2019 4:23 PM > > To: Faiz Abbas <faiz_ab...@ti.com> > > Cc: paule...@forallsecure.com; tr...@konsulko.com; u-boot@lists.denx.de > > Subject: RE: [PATCH] Revert "part: Allocate only one legacy_mbr buffer" > > > > Hi Faiz, > > > > > -----Original Message----- > > > From: Faiz Abbas <faiz_ab...@ti.com> > > > Sent: Wednesday, September 4, 2019 4:09 PM > > > To: Alexey Brodkin <abrod...@synopsys.com> > > > Cc: paule...@forallsecure.com; tr...@konsulko.com; u-boot@lists.denx.de > > > Subject: Re: [PATCH] Revert "part: Allocate only one legacy_mbr buffer" > > > > > > Hi Alexey, > > > > > > On 04/09/19 6:27 PM, Alexey Brodkin wrote: > > > > Hi Faiz, > > > > > > > > [snip] > > > > > > > >>>>> I guess what you really want to do is to allocate buffer for "mbr" > > > >>>>> dynamically of size which is max(sizeof(legacy_mbr), > > > >>>>> dev_desc->blksz). > > > >>>>> > > > >>>> > > > >>>> With the assumption that blksz is always greater than > > > >>>> sizeof(legacy_mbr), this should work: > > > >>>> > > > >>>> ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, > > > >>>> DIV_ROUND_UP(dev_desc->blksz, > > > >>>> sizeof(legacy_mbr))); > > > >>> > > > >>> No this won't work. See ALLOC_CACHE_ALIGN_BUFFER does static > > > >>> allocation > > > >>> in compile-time while blksz is set in runtime. > > > >>> > > > >>> That's why I mentioned switch to runtime allocation. > > > >>> > > > >> > > > >> Hmm. So the problem isn't as much about allocating too much in the > > > >> buffer but about using dynamically determined size for static array > > > >> declaration. > > > > > > > > In fact it was a problem of huge static allocation I discovered just by > > > > chance building U-Boot for a very memory-limited device, see [1]. > > > > > > > >> This is being used all over this disk/part_dos.c file. > > > >> Shouldn't we fix all of that? Not sure how it has been working all > > > >> along. > > > > > > > > That part I don't quite understand. What's being used, where and how? > > > > And what's the problem with dynamic allocation of the buffer for MBR? > > > > > > > > > > Isn't the following line (being used in different functions in > > > disk/part_dos.c) also problematic because we don't know the value of > > > blksz at compile time? > > > > > > ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz); > > > > Ok I see now. Looks like I missed that Variable-Length Array (VLA) feature > > added to C99 so up-to-date GCC apparently supports it. > > > > But I would strongly recommend to get rid of VLA usage by all means, > > see [1] & [2]. > > > > [1] https://lwn.net/Articles/749064/ > > [2] https://lwn.net/Articles/749089/ > > So if I add "-Wvla" to CFLAGS I see 22 usages of them: > -------------------------------->8--------------------------------- > diff --git a/Makefile b/Makefile > index c02accfc26..c6e8d12809 100644 > --- a/Makefile > +++ b/Makefile > @@ -389,7 +389,7 @@ CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix > -D__unix__ \ > > KBUILD_CPPFLAGS := -D__KERNEL__ -D__UBOOT__ > > -KBUILD_CFLAGS := -Wall -Wstrict-prototypes \ > +KBUILD_CFLAGS := -Wvla -Wall -Wstrict-prototypes \ > -Wno-format-security \ > -fno-builtin -ffreestanding $(CSTD_FLAG) > KBUILD_CFLAGS += -fshort-wchar -fno-strict-aliasing > -------------------------------->8--------------------------------- > > So that's what we have: > -------------------------------->8--------------------------------- > # make -j 48 2>&1 | grep "\-Wvla" > disk/part_dos.c:129:2: warning: ISO C90 forbids variable length array > ‘__buffer’ [-Wvla] > disk/part_dos.c:200:2: warning: ISO C90 forbids variable length array > ‘__buffer’ [-Wvla] > drivers/spi/spi-mem.c:350:2: warning: ISO C90 forbids variable length array > ‘op_buf’ [-Wvla] > drivers/input/input.c:341:2: warning: ISO C90 forbids variable length array > ‘temp’ [-Wvla] > drivers/input/input.c:513:2: warning: ISO C90 forbids variable length array > ‘ch’ [-Wvla] > env/attr.c:124:2: warning: ISO C90 forbids variable length array ‘regex’ > [-Wvla] > env/attr.c:129:10: warning: ISO C90 forbids variable length array ‘caps’ > [-Wvla] > common/command.c:29:3: warning: ISO C90 forbids variable length array > ‘cmd_array’ [-Wvla] > drivers/mmc/dw_mmc.c:248:34: warning: ISO C90 forbids variable length array > ‘__cur_idmac’ [-Wvla] > fs/fat/fat.c:61:2: warning: ISO C90 forbids variable length array ‘__buffer’ > [-Wvla] > fs/fat/fat.c:262:3: warning: ISO C90 forbids variable length array ‘__tmpbuf’ > [-Wvla] > fs/fat/fat.c:290:3: warning: ISO C90 forbids variable length array ‘__tmpbuf’ > [-Wvla] > fs/fat/fat_write.c:410:3: warning: ISO C90 forbids variable length array > ‘__tmpbuf’ [-Wvla] > fs/fat/fat_write.c:439:3: warning: ISO C90 forbids variable length array > ‘__tmpbuf’ [-Wvla] > fs/ext4/ext4_common.c:200:2: warning: ISO C90 forbids variable length array > ‘__sec_buf’ [-Wvla] > fs/fs_internal.c:18:2: warning: ISO C90 forbids variable length array > ‘__sec_buf’ [-Wvla] > fs/fs_internal.c:62:3: warning: ISO C90 forbids variable length array ‘__p’ > [-Wvla] > fs/ext4/ext4_common.c:2075:4: warning: ISO C90 forbids variable length array > ‘filename’ [-Wvla] > fs/ext4/ext4_common.c:2213:2: warning: ISO C90 forbids variable length array > ‘fpath’ [-Wvla] > lib/fdtdec.c:395:2: warning: ISO C90 forbids variable length array ‘nodes’ > [-Wvla] > lib/hashtable.c:601:9: warning: ISO C90 forbids variable length array ‘list’ > [-Wvla] > lib/hashtable.c:792:2: warning: ISO C90 forbids variable length array > ‘localvars’ [-Wvla] > -------------------------------->8--------------------------------- > > Given that situation I think it should be fine for starters to implement > a fix proposed by you and later work on VLA removal as a separate project.
Agreed, thanks guys! -- Tom
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot