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.
>
>> 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.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
