| Message ID | 1344201713-10291-3-git-send-email-ilya.yanok@cogentembedded.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Sun, Aug 5, 2012 at 2:21 PM, Ilya Yanok <ilya.yanok@cogentembedded.com> wrote: > __u_boot_cmd* symbols are not used in SPL so there is no need > to tell the linker that they are undefined. With these symbols > marked as undefined linker fails to garbage collect some unused > functions and even fails to build the resulting image. I don't like this because it causes SPL to bloat when the commands aren't also removed from the build. But I assume a number of commands were being pulled in as part of the networking stack?
Hi Tom, On Mon, Aug 6, 2012 at 2:36 AM, Tom Rini <trini@ti.com> wrote: > On Sun, Aug 5, 2012 at 2:21 PM, Ilya Yanok > <ilya.yanok@cogentembedded.com> wrote: > > __u_boot_cmd* symbols are not used in SPL so there is no need > > to tell the linker that they are undefined. With these symbols > > marked as undefined linker fails to garbage collect some unused > > functions and even fails to build the resulting image. > > I don't like this because it causes SPL to bloat when the commands > aren't also removed from the build. But I assume a number of commands > Nah. As far as I understand it, UNDEF_SYM stuff is there to protect commands from being purged by linker garbage collector. This is needed for main U-Boot as commands are referenced inderectly. I seems to me that this stuff was just copy-pasted into SPL Makefile. As far as we don't need commands in SPL we don't care about them being garbage collected (well, actually we want them to be collected). So it has nothing to do about bloating, actually SPL image is smaller with this patch applied. were being pulled in as part of the networking stack? > Not really, only some functions. Regards, Ilya.
On 08/06/2012 08:10 AM, Ilya Yanok wrote: > Hi Tom, > > On Mon, Aug 6, 2012 at 2:36 AM, Tom Rini <trini@ti.com > <mailto:trini@ti.com>> wrote: > > On Sun, Aug 5, 2012 at 2:21 PM, Ilya Yanok > <ilya.yanok@cogentembedded.com > <mailto:ilya.yanok@cogentembedded.com>> wrote: > > __u_boot_cmd* symbols are not used in SPL so there is no need > > to tell the linker that they are undefined. With these symbols > > marked as undefined linker fails to garbage collect some unused > > functions and even fails to build the resulting image. > > I don't like this because it causes SPL to bloat when the commands > aren't also removed from the build. But I assume a number of commands > > > Nah. As far as I understand it, UNDEF_SYM stuff is there to protect > commands from being purged by linker garbage collector. This is needed > for main U-Boot as commands are referenced inderectly. > I seems to me that this stuff was just copy-pasted into SPL Makefile. As > far as we don't need commands in SPL we don't care about them being > garbage collected (well, actually we want them to be collected). So it > has nothing to do about bloating, actually SPL image is smaller with > this patch applied. What toolchain are you using? In my tests they have not been collected.
On Mon, Aug 6, 2012 at 7:30 PM, Tom Rini <trini@ti.com> wrote: > On 08/06/2012 08:10 AM, Ilya Yanok wrote: > > Hi Tom, > > > > On Mon, Aug 6, 2012 at 2:36 AM, Tom Rini <trini@ti.com > > <mailto:trini@ti.com>> wrote: > > > > On Sun, Aug 5, 2012 at 2:21 PM, Ilya Yanok > > <ilya.yanok@cogentembedded.com > > <mailto:ilya.yanok@cogentembedded.com>> wrote: > > > __u_boot_cmd* symbols are not used in SPL so there is no need > > > to tell the linker that they are undefined. With these symbols > > > marked as undefined linker fails to garbage collect some unused > > > functions and even fails to build the resulting image. > > > > I don't like this because it causes SPL to bloat when the commands > > aren't also removed from the build. But I assume a number of > commands > > > > > > Nah. As far as I understand it, UNDEF_SYM stuff is there to protect > > commands from being purged by linker garbage collector. This is needed > > for main U-Boot as commands are referenced inderectly. > > I seems to me that this stuff was just copy-pasted into SPL Makefile. As > > far as we don't need commands in SPL we don't care about them being > > garbage collected (well, actually we want them to be collected). So it > > has nothing to do about bloating, actually SPL image is smaller with > > this patch applied. > > What toolchain are you using? In my tests they have not been collected. > ELDK 5.2 Regards, Ilya.
On Mon, Aug 6, 2012 at 8:31 AM, Ilya Yanok <ilya.yanok@cogentembedded.com> wrote: > On Mon, Aug 6, 2012 at 7:30 PM, Tom Rini <trini@ti.com> wrote: > >> On 08/06/2012 08:10 AM, Ilya Yanok wrote: >> > Hi Tom, >> > >> > On Mon, Aug 6, 2012 at 2:36 AM, Tom Rini <trini@ti.com >> > <mailto:trini@ti.com>> wrote: >> > >> > On Sun, Aug 5, 2012 at 2:21 PM, Ilya Yanok >> > <ilya.yanok@cogentembedded.com >> > <mailto:ilya.yanok@cogentembedded.com>> wrote: >> > > __u_boot_cmd* symbols are not used in SPL so there is no need >> > > to tell the linker that they are undefined. With these symbols >> > > marked as undefined linker fails to garbage collect some unused >> > > functions and even fails to build the resulting image. >> > >> > I don't like this because it causes SPL to bloat when the commands >> > aren't also removed from the build. But I assume a number of >> commands >> > >> > >> > Nah. As far as I understand it, UNDEF_SYM stuff is there to protect >> > commands from being purged by linker garbage collector. This is needed >> > for main U-Boot as commands are referenced inderectly. >> > I seems to me that this stuff was just copy-pasted into SPL Makefile. As >> > far as we don't need commands in SPL we don't care about them being >> > garbage collected (well, actually we want them to be collected). So it >> > has nothing to do about bloating, actually SPL image is smaller with >> > this patch applied. >> >> What toolchain are you using? In my tests they have not been collected. >> > > ELDK 5.2 OK, installed and it's still larger with this change than without and it's not garbage collecting and dropping commands if I un-guard the nandecc command for example. Tested with omap3_beagle.
Hi Tom, On Mon, Aug 6, 2012 at 9:10 PM, Tom Rini <trini@ti.com> wrote: > > OK, installed and it's still larger with this change than without and > it's not garbage collecting and dropping commands if I un-guard the > nandecc command for example. Tested with omap3_beagle. > Did some testing as well. master branch, omap3_beagle config. Trying clean master, master + remove undef patch, master + remove undef patch + un-guard nandecc & master + un-guard nandecc. Here is my results: $ ls -l ../out/master/spl/u-boot-spl.bin -rwxrwxr-x 1 ilya ilya 44692 Aug 6 21:57 ../out/master/spl/u-boot-spl.bin $ ls -l ../out/undef/spl/u-boot-spl.bin -rwxrwxr-x 1 ilya ilya 44692 Aug 6 21:52 ../out/undef/spl/u-boot-spl.bin $ ls -l ../out/undef+nandecc/spl/u-boot-spl.bin -rwxrwxr-x 1 ilya ilya 44844 Aug 6 21:53 ../out/undef+nandecc/spl/u-boot-spl.bin (and master + un-guard ecc actually failed to build with: $ ./MAKEALL omap3_beagle Configuring for omap3_beagle board... make[1]: *** [/work/u-boot/spl/u-boot-spl] Error 1 make: *** [spl/u-boot-spl.bin] Error 2 text data bss dec hex filename 326458 8460 266904 601822 92ede ./u-boot arch/arm/cpu/armv7/omap3/libomap3.o: In function `do_switch_ecc': /work/u-boot/arch/arm/cpu/armv7/omap3/board.c:312: undefined reference to `omap_nand_switch_ecc' /work/u-boot/arch/arm/cpu/armv7/omap3/board.c:314: undefined reference to `omap_nand_switch_ecc' make[1]: *** [/work/u-boot/spl/u-boot-spl] Error 1 make: *** [spl/u-boot-spl.bin] Error 2 which shows that without remove undef patch do_switch_ecc() function is preserved while with this patch it's garbage collected). My interpretation: first two builds are equal, that's to be expected as in mainline U-Boot all command definitions are carefully guarded so remove undef patch has no effect at all. You said in your testing patched version produced bigger image... Probably you patched it by hand without commiting and got "-dirty" difference? (I did so initially ;) ) Next, when we unguard the command we definitely get bigger image... But the code is not here: $ arm-linux-gnueabi-nm ../out/undef+nandecc/spl/u-boot-spl |grep do_switch_ecc $ By comparing of two images I've found that the difference comes from ro-strings (two help strings in U_BOOT_CMD, string in printf, "sw" & "hw"). It looks like the linker doesn't collect ro-strings referenced from collected functions... Probably that's a bug but I'm not sure... Regards, Ilya.
On Mon, Aug 06, 2012 at 11:15:25PM +0400, Ilya Yanok wrote: > Hi Tom, > > On Mon, Aug 6, 2012 at 9:10 PM, Tom Rini <trini@ti.com> wrote: > > > > > OK, installed and it's still larger with this change than without and > > it's not garbage collecting and dropping commands if I un-guard the > > nandecc command for example. Tested with omap3_beagle. > > > > Did some testing as well. [snip] > My interpretation: first two builds are equal, that's to be expected as in > mainline U-Boot all command definitions are carefully guarded so remove > undef patch has no effect at all. You said in your testing patched version > produced bigger image... Probably you patched it by hand without commiting > and got "-dirty" difference? (I did so initially ;) ) That would be it, yes :) > Next, when we unguard the command we definitely get bigger image... But the > code is not here: > > $ arm-linux-gnueabi-nm ../out/undef+nandecc/spl/u-boot-spl |grep > do_switch_ecc > $ > > By comparing of two images I've found that the difference comes from > ro-strings (two help strings in U_BOOT_CMD, string in printf, "sw" & "hw"). > It looks like the linker doesn't collect ro-strings referenced from > collected functions... Probably that's a bug but I'm not sure... Yes. What I meant was that not all of the stuff that is guarded today is garbage collected so the resulting image is larger than it must be. And yes, this is a toolchain issue of sorts (not being aggressive enough in collecting unreferenced data). > > Regards, Ilya. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Hi Tom, On Tue, Aug 7, 2012 at 12:52 AM, Tom Rini <trini@ti.com> wrote: > > By comparing of two images I've found that the difference comes from > > ro-strings (two help strings in U_BOOT_CMD, string in printf, "sw" & > "hw"). > > It looks like the linker doesn't collect ro-strings referenced from > > collected functions... Probably that's a bug but I'm not sure... > > Yes. What I meant was that not all of the stuff that is guarded today > is garbage collected so the resulting image is larger than it must be. > Yep. And that's actually goes beyond the subject of this patch: as long as we rely on linker GC and not putting guards by hands we will get unused string literals compiled in. I still think that this patch does the correct things (UNDEF_SYM is there to protect commands from GC and we don't really want commands in SPL so there is nothing to protect). But I will try to add guards to make net-spl compile even without this patch. > And yes, this is a toolchain issue of sorts (not being aggressive enough > in collecting unreferenced data). > Looks like it can be fixed by forcing the compiler to emit symbols for string literals... But I'm not really compiler guy... Regards, Ilya.
Hi Tom, On Tue, Aug 7, 2012 at 1:11 AM, Ilya Yanok <ilya.yanok@cogentembedded.com>wrote: > > Yes. What I meant was that not all of the stuff that is guarded today >> is garbage collected so the resulting image is larger than it must be. >> > > Yep. And that's actually goes beyond the subject of this patch: as long as > we rely on linker GC and not putting guards by hands we will get unused > string literals compiled in. > > I still think that this patch does the correct things (UNDEF_SYM is there > to protect commands from GC and we don't really want commands in SPL so > there is nothing to protect). But I will try to add guards to make net-spl > compile even without this patch. > I've just posted v5 for patch 4/5 that doesn't depend on this patch. It really has some improvement wrt size (~50KB net-only SPL image) so probably it could be useful not only for AM33xx... Regards, Ilya.
diff --git a/spl/Makefile b/spl/Makefile index ea7d475..8576d56 100644 --- a/spl/Makefile +++ b/spl/Makefile @@ -126,8 +126,6 @@ $(obj)u-boot-spl.bin: $(obj)u-boot-spl $(OBJCOPY) $(OBJCFLAGS) -O binary $< $@ GEN_UBOOT = \ - UNDEF_SYM=`$(OBJDUMP) -x $(LIBS) | \ - sed -n -e 's/.*\($(SYM_PREFIX)__u_boot_cmd_.*\)/-u\1/p'|sort|uniq`;\ cd $(obj) && $(LD) $(LDFLAGS) $(LDFLAGS_$(@F)) $$UNDEF_SYM $(__START) \ --start-group $(__LIBS) --end-group $(PLATFORM_LIBS) \ -Map u-boot-spl.map -o u-boot-spl
__u_boot_cmd* symbols are not used in SPL so there is no need to tell the linker that they are undefined. With these symbols marked as undefined linker fails to garbage collect some unused functions and even fails to build the resulting image. Signed-off-by: Ilya Yanok <ilya.yanok@cogentembedded.com> --- spl/Makefile | 2 -- 1 file changed, 2 deletions(-)