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.

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.

Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to