Hi Tom. On 23 August 2018 at 07:58, Tom Rini <tr...@konsulko.com> wrote: > On Thu, Aug 23, 2018 at 04:45:29AM -0600, Simon Glass wrote: >> Hi Tom, >> >> On 21 August 2018 at 17:11, Tom Rini <tr...@konsulko.com> wrote: >> > On Tue, Aug 21, 2018 at 04:29:49PM -0600, Simon Glass wrote: >> >> Hi Alex, >> >> >> >> On 21 August 2018 at 13:26, Alexander Graf <ag...@suse.de> 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. >> >> >> >> Yes I think removing dead could could cause problems. But so could not >> >> garbage-collecting sections, so it is not a great argument. Sandbox is >> >> targeted at building as much code as possible. Ideally every piece of >> >> non-arch-specific code should be built with sandbox. >> > >> > I feel like this is the argument we had about enabling gc on other >> > platforms. gc'ing dead code cannot cause problems that aren't real >> > bugs. In fact, if we build something and it results in dead code that >> > we don't expect to be dead that is a problem. We have everything else >> > doing this collection so I think we do want sandbox to match. >> >> Fair enough, this is the consistency argument that Alex made also. I >> do understand that. Sandbox does potentially have a wider toolchain >> target, since it uses whatever is installed on the machine, and >> apparently runs on non-Linux devices. I suppose toolchains that people >> use handle this reliably now. > > Yes, this is something that toolchains get right and have been getting > right for a long while now. Honestly, I didn't realize that sandbox > wasn't doing this already or I would have brought it up. > >> >> Maybe I am being conservative, but I see no reason to enable it for >> >> sandbox. I'll try to think of some better reasons and reply if I can. >> >> I also feel that it slipped in under the radar with no review. >> > >> > This is something I want to be sensitive to. If you really want this >> > change to go in for v2018.11, OK, it's your arch. But I really think we >> > should move sandbox in this direction for consistency with all the >> > hardware platforms. >> >> At the least I would like a revert followed by a patch to change >> sandbox over. The commit that changed this is titled: >> >> efi_loader: Rename sections to allow for implicit data >> >> It does not have a sandbox: tag, nor does it mention the compiler flag >> changes. No one would guess that this major change is happening. It >> broke x86 and I am not yet confident that sandbox still functions >> correctly. >> >> I would be happier if the second patch went in for the next release, >> but if we really are not seeing problems, then OK. > > OK, now I'm trying to see what we need to do next. Regardless of Alex's > current EFI PR, we have 7e21fbca26d1 ("efi_loader: Rename sections to > allow for implicit data") in master. Looking at the PR, I see that it > does turn on -fdata-sections on x86, and that Bin reviewed/tested it. > So I'm going to push the PR there shortly. > > If, for sandbox you want to backout --gc-sections now (the commit > message does spell out that we need -ffunction/data-sections and what > for) and re-introduce that for the next release please post the patches > and I'll grab the backout one and apply it (or post it then PR it if > you'd rather, that's fine too).
I'd like to back out all of the compilel/link flag changes: Here is my patch: http://patchwork.ozlabs.org/patch/954875/ Then I would like to get the sandbox EFI stuff reviewed by Alex, and get that applied when the merge window opens. I can bring it in via sandbox but I do want Alex to look at it. It has been waiting for two releases and I want to avoid any more barriers to this work. Herinrich's testing work is excellent, but having sandbox able to actually run apps is a good step forward I think. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
