On Tue, Aug 21, 2018 at 09:26:53PM +0200, Alexander Graf wrote: > > > On 21.08.18 19:30, Simon Glass wrote: > > Hi Alex, > > > > On 20 August 2018 at 06:23, Alexander Graf <ag...@suse.de> wrote: > >> > >> On 08/17/2018 02:49 PM, Simon Glass wrote: > >>> > >>> Hi, > >>> > >>> On 9 August 2018 at 23:45, Bin Meng <bmeng...@gmail.com> wrote: > >>>> > >>>> Hi Alex, > >>>> > >>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <ag...@suse.de> wrote: > >>>>> > >>>>> > >>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <s...@chromium.org>: > >>>>>> > >>>>>> Hi Alex, > >>>>>> > >>>>>>> On 11 June 2018 at 23:48, Alexander Graf <ag...@suse.de> wrote: > >>>>>>> Some times gcc may generate data that is then used within code that > >>>>>>> may > >>>>>>> be part of an efi runtime section. That data could be jump tables, > >>>>>>> constants or strings. > >>>>>>> > >>>>>>> In order to make sure we catch these, we need to ensure that gcc emits > >>>>>>> them into a section that we can relocate together with all the other > >>>>>>> efi runtime bits. This only works if the -ffunction-sections and > >>>>>>> -fdata-sections flags are passed and the efi runtime functions are > >>>>>>> in a section that starts with ".text". > >>>>>>> > >>>>>>> Up to now we had all efi runtime bits in sections that did not > >>>>>>> interfere with the normal section naming scheme, but this forces > >>>>>>> us to do so. Hence we need to move the efi_loader text/data/rodata > >>>>>>> sections before the global *(.text*) catch-all section. > >>>>>>> > >>>>>>> With this patch in place, we should hopefully have an easier time > >>>>>>> to extend the efi runtime functionality in the future. > >>>>>>> > >>>>>>> Signed-off-by: Alexander Graf <ag...@suse.de> > >>>>>>> --- > >>>>>>> arch/arm/config.mk | 4 ++-- > >>>>>>> arch/arm/cpu/armv8/u-boot.lds | 24 +++++++++++++-------- > >>>>>>> arch/arm/cpu/u-boot.lds | 36 > >>>>>>> ++++++++++++++++++------------- > >>>>>>> arch/arm/mach-zynq/u-boot.lds | 36 > >>>>>>> ++++++++++++++++++------------- > >>>>>>> arch/riscv/cpu/ax25/u-boot.lds | 26 +++++++++++++--------- > >>>>>>> arch/sandbox/config.mk | 3 +++ > >>>>>>> arch/sandbox/cpu/u-boot.lds | 9 ++++---- > >>>>>>> arch/x86/config.mk | 2 +- > >>>>>>> arch/x86/cpu/u-boot.lds | 32 > >>>>>>> ++++++++++++++------------- > >>>>>>> board/qualcomm/dragonboard410c/u-boot.lds | 17 +++++++++++++-- > >>>>>>> board/qualcomm/dragonboard820c/u-boot.lds | 24 +++++++++++++-------- > >>>>>>> board/ti/am335x/u-boot.lds | 36 > >>>>>>> ++++++++++++++++++------------- > >>>>>>> include/efi_loader.h | 4 ++-- > >>>>>>> 13 files changed, 154 insertions(+), 99 deletions(-) > >>>>>>> > >>>>>> I missed this at the time, probably thinking the subject made it sound > >>>>>> innocuous. There is no 'sandbox:' tag. > >>>>>> > >>>>>> This seems to break sandbox in a pretty strange way: > >>>>>> > >>>>>> gdb --args /tmp/crosfw/sandbox/u-boot -D > >>>>>> GNU gdb (Debian 7.12-6) 7.12.0.20161007-git > >>>>>> Copyright (C) 2016 Free Software Foundation, Inc. > >>>>>> License GPLv3+: GNU GPL version 3 or later > >>>>>> <http://gnu.org/licenses/gpl.html> > >>>>>> This is free software: you are free to change and redistribute it. > >>>>>> There is NO WARRANTY, to the extent permitted by law. Type "show > >>>>>> copying" > >>>>>> and "show warranty" for details. > >>>>>> This GDB was configured as "x86_64-linux-gnu". > >>>>>> Type "show configuration" for configuration details. > >>>>>> For bug reporting instructions, please see: > >>>>>> <http://www.gnu.org/software/gdb/bugs/>. > >>>>>> Find the GDB manual and other documentation resources online at: > >>>>>> <http://www.gnu.org/software/gdb/documentation/>. > >>>>>> For help, type "help". > >>>>>> Type "apropos word" to search for commands related to "word"... > >>>>>> Reading symbols from /tmp/crosfw/sandbox/u-boot...done. > >>>>>> (gdb) r > >>>>>> Starting program: /tmp/crosfw/sandbox/u-boot -D > >>>>>> [Thread debugging using libthread_db enabled] > >>>>>> Using host libthread_db library > >>>>>> "/lib/x86_64-linux-gnu/libthread_db.so.1". > >>>>>> > >>>>>> Program received signal SIGSEGV, Segmentation fault. > >>>>>> 0x0000555555571520 in open@plt () > >>>>>> (gdb) up > >>>>>> #1 0x0000555555571e9a in sandbox_read_fdt_from_file () > >>>>>> at > >>>>>> /home/sjg/c/src/third_party/u-boot/files/arch/sandbox/cpu/cpu.c:264 > >>>>>> 264 fd = os_open(fname, OS_O_RDONLY); > >>>>>> (gdb) print fname > >>>>>> $1 = 0x7ffff7ff0000 "/tmp/crosfw/sandbox/u-boot.dtb" > >>>>>> (gdb) q > >>>>>> > >>>>>> > >>>>>> Also the commit message suggests that this patch changes sandbox to > >>>>>> use --gc-sections, which is not obvious from the subject. I think that > >>>>>> should be a separate commit and in fact it should really be separate > >>>>>> commits for each arch, I think. That might help people notice it... > >>>>>> > >>>>>> I only noticed now since the EFI pull request has landed. > >>>>> > >>>>> Can you try my bss patch really quick? Maybe we're just overwriting gd. > >>>>> > >>>>> Alex > >>>>> > >>>> This patch breaks efi-x86_app_defconfig. The EFI application no longer > >>>> boots. I was testing on top of u-boot/master. > >>>> > >>>> If I do: > >>>> > >>>> diff --git a/arch/x86/config.mk b/arch/x86/config.mk > >>>> index 586e11a..fc119ec 100644 > >>>> --- a/arch/x86/config.mk > >>>> +++ b/arch/x86/config.mk > >>>> @@ -24,7 +24,6 @@ endif > >>>> ifeq ($(IS_32BIT),y) > >>>> PLATFORM_CPPFLAGS += -march=i386 -m32 > >>>> # TODO: These break on x86_64; need to debug further > >>>> -PLATFORM_RELFLAGS += -fdata-sections > >>>> else > >>>> PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -m64 > >>>> endif > >>>> > >>>> Then it boots again. Can you please take a look? > >>>> > >>>> Regards, > >>>> Bin > >>> > >>> Please can we revert the offending patch quickly for the release? I am > >>> not comfortable with the sandbox changes either (data-sections, etc.). > >> > >> I can not reproduce the sandbox breakage (and travis doesn't seem to > >> either, otherwise it would be broken for everyone, no?). Can you give me > >> some guidelines on how to reproduce the failures for you and I'll just fix > >> it? > > > > I would like to revert the sandbox changes at least. I don't want to > > enable -ffunction-sections, for example. > > Could you please explain why? In general I always thought the sandbox > target was meant as debugging aid which allows you to find and debug > bugs more easily. > > I would assume that chances for breakage are higher with function and > data sections, because the linker could remove code it considers dead? > So for a debugging target, I would think it makes sense to have it > enabled rather than disabled.
I too am puzzled. If it's discarded it wasn't in use. It also makes sandbox match all of the rest of the platforms so further aids in debugging/testing/consistency. -- Tom
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
