efi_loader: Rename sections to allow for implicit data

Message ID 20180612054837.57146-1-agraf@suse.de
State Accepted
Commit 7e21fbca26d18327cf7cabaad08df276a06a07d8
Headers show
Series
  • efi_loader: Rename sections to allow for implicit data
Related show

Commit Message

Alexander Graf June 12, 2018, 5:48 a.m.
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 <agraf@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(-)

Comments

Alexander Graf June 24, 2018, 6:47 a.m. | #1
> 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 <agraf@suse.de>

Thanks, applied to efi-next

Alex
Simon Glass Aug. 7, 2018, 5:12 p.m. | #2
Hi Alex,

On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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.

Hopefully there is no impact on all the x86 builds.

Regards,
Simon
Alexander Graf Aug. 7, 2018, 5:16 p.m. | #3
> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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

> 
> Hopefully there is no impact on all the x86 builds.
> 
> Regards,
> Simon
Bin Meng Aug. 10, 2018, 5:45 a.m. | #4
Hi Alex,

On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>>
>> Hi Alex,
>>
>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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
Simon Glass Aug. 17, 2018, 12:49 p.m. | #5
Hi,

On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Alex,
>
> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
> >
> >
> >> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
> >>
> >> Hi Alex,
> >>
> >>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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.).

Regards,
Simon
Bin Meng Aug. 20, 2018, 1:43 a.m. | #6
On Fri, Aug 17, 2018 at 8:49 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> Hi Alex,
>>
>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>> >
>> >
>> >> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>> >>
>> >> Hi Alex,
>> >>
>> >>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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 agree since it's getting close to the release date.

Regards,
Bin
Alexander Graf Aug. 20, 2018, 12:22 p.m. | #7
On 08/20/2018 03:43 AM, Bin Meng wrote:
> On Fri, Aug 17, 2018 at 8:49 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi,
>>
>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Alex,
>>>
>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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 agree since it's getting close to the release date.

I have a fix ready for you. Will send it out soon. Basically the linker 
scripts only included .bss, not .bss*.


Alex
Alexander Graf Aug. 20, 2018, 12:23 p.m. | #8
On 08/17/2018 02:49 PM, Simon Glass wrote:
> Hi,
>
> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Alex,
>>
>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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?


Alex
Simon Glass Aug. 21, 2018, 5:30 p.m. | #9
Hi Alex,

On 20 August 2018 at 06:23, Alexander Graf <agraf@suse.de> wrote:
>
> On 08/17/2018 02:49 PM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> Hi Alex,
>>>
>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>>
>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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.

Regards,
Simon
Alexander Graf Aug. 21, 2018, 7:26 p.m. | #10
On 21.08.18 19:30, Simon Glass wrote:
> Hi Alex,
> 
> On 20 August 2018 at 06:23, Alexander Graf <agraf@suse.de> wrote:
>>
>> On 08/17/2018 02:49 PM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>
>>>>>
>>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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.


Alex
Tom Rini Aug. 21, 2018, 8:31 p.m. | #11
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 <agraf@suse.de> wrote:

> >>

> >> On 08/17/2018 02:49 PM, Simon Glass wrote:

> >>>

> >>> Hi,

> >>>

> >>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:

> >>>>

> >>>> Hi Alex,

> >>>>

> >>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:

> >>>>>

> >>>>>

> >>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:

> >>>>>>

> >>>>>> Hi Alex,

> >>>>>>

> >>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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
Simon Glass Aug. 21, 2018, 10:29 p.m. | #12
Hi Alex,

On 21 August 2018 at 13:26, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 21.08.18 19:30, Simon Glass wrote:
>> Hi Alex,
>>
>> On 20 August 2018 at 06:23, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 08/17/2018 02:49 PM, Simon Glass wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>
>>>>> Hi Alex,
>>>>>
>>>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>>>
>>>>>>
>>>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>>>>>>>
>>>>>>> Hi Alex,
>>>>>>>
>>>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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
Tom Rini Aug. 21, 2018, 11:11 p.m. | #13
On Tue, Aug 21, 2018 at 04:29:49PM -0600, Simon Glass wrote:
> Hi Alex,

> 

> On 21 August 2018 at 13:26, Alexander Graf <agraf@suse.de> wrote:

> >

> >

> > On 21.08.18 19:30, Simon Glass wrote:

> >> Hi Alex,

> >>

> >> On 20 August 2018 at 06:23, Alexander Graf <agraf@suse.de> wrote:

> >>>

> >>> On 08/17/2018 02:49 PM, Simon Glass wrote:

> >>>>

> >>>> Hi,

> >>>>

> >>>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:

> >>>>>

> >>>>> Hi Alex,

> >>>>>

> >>>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:

> >>>>>>

> >>>>>>

> >>>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:

> >>>>>>>

> >>>>>>> Hi Alex,

> >>>>>>>

> >>>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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.

> 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.

-- 
Tom
Simon Glass Aug. 23, 2018, 10:45 a.m. | #14
Hi Tom,

On 21 August 2018 at 17:11, Tom Rini <trini@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 <agraf@suse.de> wrote:
>> >
>> >
>> > On 21.08.18 19:30, Simon Glass wrote:
>> >> Hi Alex,
>> >>
>> >> On 20 August 2018 at 06:23, Alexander Graf <agraf@suse.de> wrote:
>> >>>
>> >>> On 08/17/2018 02:49 PM, Simon Glass wrote:
>> >>>>
>> >>>> Hi,
>> >>>>
>> >>>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>> >>>>>
>> >>>>> Hi Alex,
>> >>>>>
>> >>>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>> >>>>>>
>> >>>>>>
>> >>>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>> >>>>>>>
>> >>>>>>> Hi Alex,
>> >>>>>>>
>> >>>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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
Tom Rini Aug. 23, 2018, 1:58 p.m. | #15
On Thu, Aug 23, 2018 at 04:45:29AM -0600, Simon Glass wrote:
> Hi Tom,

> 

> On 21 August 2018 at 17:11, Tom Rini <trini@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 <agraf@suse.de> wrote:

> >> >

> >> >

> >> > On 21.08.18 19:30, Simon Glass wrote:

> >> >> Hi Alex,

> >> >>

> >> >> On 20 August 2018 at 06:23, Alexander Graf <agraf@suse.de> wrote:

> >> >>>

> >> >>> On 08/17/2018 02:49 PM, Simon Glass wrote:

> >> >>>>

> >> >>>> Hi,

> >> >>>>

> >> >>>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:

> >> >>>>>

> >> >>>>> Hi Alex,

> >> >>>>>

> >> >>>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:

> >> >>>>>>

> >> >>>>>>

> >> >>>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:

> >> >>>>>>>

> >> >>>>>>> Hi Alex,

> >> >>>>>>>

> >> >>>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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).

Thanks all!

-- 
Tom
Simon Glass Aug. 23, 2018, 4:31 p.m. | #16
Hi Tom.

On 23 August 2018 at 07:58, Tom Rini <trini@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 <trini@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 <agraf@suse.de> wrote:
>> >> >
>> >> >
>> >> > On 21.08.18 19:30, Simon Glass wrote:
>> >> >> Hi Alex,
>> >> >>
>> >> >> On 20 August 2018 at 06:23, Alexander Graf <agraf@suse.de> wrote:
>> >> >>>
>> >> >>> On 08/17/2018 02:49 PM, Simon Glass wrote:
>> >> >>>>
>> >> >>>> Hi,
>> >> >>>>
>> >> >>>> On 9 August 2018 at 23:45, Bin Meng <bmeng.cn@gmail.com> wrote:
>> >> >>>>>
>> >> >>>>> Hi Alex,
>> >> >>>>>
>> >> >>>>> On Wed, Aug 8, 2018 at 1:16 AM, Alexander Graf <agraf@suse.de> wrote:
>> >> >>>>>>
>> >> >>>>>>
>> >> >>>>>>> Am 07.08.2018 um 18:12 schrieb Simon Glass <sjg@chromium.org>:
>> >> >>>>>>>
>> >> >>>>>>> Hi Alex,
>> >> >>>>>>>
>> >> >>>>>>>> On 11 June 2018 at 23:48, Alexander Graf <agraf@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 <agraf@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
Andreas Färber Aug. 23, 2018, 4:41 p.m. | #17
Am 23.08.2018 um 18:31 schrieb Simon Glass:
> On 23 August 2018 at 07:58, Tom Rini <trini@konsulko.com> wrote:
>> 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/

If you go down that path, please align the subject of that commit with
what it does, e.g., s/Revert/Partially revert/. Otherwise it is terribly
confusing for users that don't care about sandbox.

Regards,
Andreas
Tom Rini Aug. 23, 2018, 5:55 p.m. | #18
On Thu, Aug 23, 2018 at 06:41:56PM +0200, Andreas Färber wrote:
> Am 23.08.2018 um 18:31 schrieb Simon Glass:

> > On 23 August 2018 at 07:58, Tom Rini <trini@konsulko.com> wrote:

> >> 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/

> 

> If you go down that path, please align the subject of that commit with

> what it does, e.g., s/Revert/Partially revert/. Otherwise it is terribly

> confusing for users that don't care about sandbox.


With a slight subject reword, should I grab this patch now then?

-- 
Tom
Simon Glass Aug. 23, 2018, 8:01 p.m. | #19
Hi Tom,

On 23 August 2018 at 11:55, Tom Rini <trini@konsulko.com> wrote:
> On Thu, Aug 23, 2018 at 06:41:56PM +0200, Andreas Färber wrote:
>> Am 23.08.2018 um 18:31 schrieb Simon Glass:
>> > On 23 August 2018 at 07:58, Tom Rini <trini@konsulko.com> wrote:
>> >> 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/
>>
>> If you go down that path, please align the subject of that commit with
>> what it does, e.g., s/Revert/Partially revert/. Otherwise it is terribly
>> confusing for users that don't care about sandbox.

Yes I agree, sorry. I did a full revert and then changed it because I
figured the only real problem I had was with sandbox.

>
> With a slight subject reword, should I grab this patch now then?

That's fine with me.

Regards,
Simon

Patch

diff --git a/arch/arm/config.mk b/arch/arm/config.mk
index efafc69d1b..f25603109e 100644
--- a/arch/arm/config.mk
+++ b/arch/arm/config.mk
@@ -134,11 +134,11 @@  endif
 ifdef CONFIG_ARM64
 OBJCOPYFLAGS += -j .text -j .secure_text -j .secure_data -j .rodata -j .data \
 		-j .u_boot_list -j .rela.dyn -j .got -j .got.plt \
-		-j .binman_sym_table
+		-j .binman_sym_table -j .text_rest
 else
 OBJCOPYFLAGS += -j .text -j .secure_text -j .secure_data -j .rodata -j .hash \
 		-j .data -j .got -j .got.plt -j .u_boot_list -j .rel.dyn \
-		-j .binman_sym_table
+		-j .binman_sym_table -j .text_rest
 endif
 
 # if a dtb section exists we always have to include it
diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index eb926b3c14..53de80f745 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -25,6 +25,19 @@  SECTIONS
 	{
 		*(.__image_copy_start)
 		CPUDIR/start.o (.text*)
+	}
+
+	/* This needs to come before *(.text*) */
+	.efi_runtime : {
+                __efi_runtime_start = .;
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
+                __efi_runtime_stop = .;
+	}
+
+	.text_rest :
+	{
 		*(.text*)
 	}
 
@@ -98,17 +111,10 @@  SECTIONS
 
 	. = ALIGN(8);
 
-	.efi_runtime : {
-                __efi_runtime_start = .;
-		*(efi_runtime_text)
-		*(efi_runtime_data)
-                __efi_runtime_stop = .;
-	}
-
 	.efi_runtime_rel : {
                 __efi_runtime_rel_start = .;
-		*(.relaefi_runtime_text)
-		*(.relaefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
                 __efi_runtime_rel_stop = .;
 	}
 
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 4157374d21..834dc99554 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -43,6 +43,25 @@  SECTIONS
 		*(.__image_copy_start)
 		*(.vectors)
 		CPUDIR/start.o (.text*)
+	}
+
+	/* This needs to come before *(.text*) */
+	.__efi_runtime_start : {
+		*(.__efi_runtime_start)
+	}
+
+	.efi_runtime : {
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
+	}
+
+	.__efi_runtime_stop : {
+		*(.__efi_runtime_stop)
+	}
+
+	.text_rest :
+	{
 		*(.text*)
 	}
 
@@ -136,27 +155,14 @@  SECTIONS
 
 	. = ALIGN(4);
 
-	.__efi_runtime_start : {
-		*(.__efi_runtime_start)
-	}
-
-	.efi_runtime : {
-		*(efi_runtime_text)
-		*(efi_runtime_data)
-	}
-
-	.__efi_runtime_stop : {
-		*(.__efi_runtime_stop)
-	}
-
 	.efi_runtime_rel_start :
 	{
 		*(.__efi_runtime_rel_start)
 	}
 
 	.efi_runtime_rel : {
-		*(.relefi_runtime_text)
-		*(.relefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
 	}
 
 	.efi_runtime_rel_stop :
diff --git a/arch/arm/mach-zynq/u-boot.lds b/arch/arm/mach-zynq/u-boot.lds
index ec9a0a0161..91c32e89e8 100644
--- a/arch/arm/mach-zynq/u-boot.lds
+++ b/arch/arm/mach-zynq/u-boot.lds
@@ -19,6 +19,25 @@  SECTIONS
 		*(.__image_copy_start)
 		*(.vectors)
 		CPUDIR/start.o (.text*)
+	}
+
+	/* This needs to come before *(.text*) */
+	.__efi_runtime_start : {
+		*(.__efi_runtime_start)
+	}
+
+	.efi_runtime : {
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
+	}
+
+	.__efi_runtime_stop : {
+		*(.__efi_runtime_stop)
+	}
+
+	.text_rest :
+	{
 		*(.text*)
 	}
 
@@ -41,27 +60,14 @@  SECTIONS
 
 	. = ALIGN(4);
 
-	.__efi_runtime_start : {
-		*(.__efi_runtime_start)
-	}
-
-	.efi_runtime : {
-		*(efi_runtime_text)
-		*(efi_runtime_data)
-	}
-
-	.__efi_runtime_stop : {
-		*(.__efi_runtime_stop)
-	}
-
 	.efi_runtime_rel_start :
 	{
 		*(.__efi_runtime_rel_start)
 	}
 
 	.efi_runtime_rel : {
-		*(.relefi_runtime_text)
-		*(.relefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
 	}
 
 	.efi_runtime_rel_stop :
diff --git a/arch/riscv/cpu/ax25/u-boot.lds b/arch/riscv/cpu/ax25/u-boot.lds
index 1589babf6c..3cc89746b1 100644
--- a/arch/riscv/cpu/ax25/u-boot.lds
+++ b/arch/riscv/cpu/ax25/u-boot.lds
@@ -12,7 +12,20 @@  SECTIONS
 	.text :
 	{
 		arch/riscv/cpu/ax25/start.o	(.text)
-		*(.text)
+	}
+
+	/* This needs to come before *(.text*) */
+	.efi_runtime : {
+                __efi_runtime_start = .;
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
+                __efi_runtime_stop = .;
+	}
+
+	.text_rest :
+	{
+		*(.text*)
 	}
 
 	. = ALIGN(4);
@@ -39,17 +52,10 @@  SECTIONS
 
     . = ALIGN(4);
 
-	.efi_runtime : {
-                __efi_runtime_start = .;
-		*(efi_runtime_text)
-		*(efi_runtime_data)
-                __efi_runtime_stop = .;
-	}
-
 	.efi_runtime_rel : {
                 __efi_runtime_rel_start = .;
-		*(.relaefi_runtime_text)
-		*(.relaefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
                 __efi_runtime_rel_stop = .;
 	}
 
diff --git a/arch/sandbox/config.mk b/arch/sandbox/config.mk
index 2babcde881..5e7077bfe7 100644
--- a/arch/sandbox/config.mk
+++ b/arch/sandbox/config.mk
@@ -5,6 +5,9 @@  PLATFORM_CPPFLAGS += -D__SANDBOX__ -U_FORTIFY_SOURCE
 PLATFORM_CPPFLAGS += -DCONFIG_ARCH_MAP_SYSMEM
 PLATFORM_LIBS += -lrt
 
+LDFLAGS_FINAL += --gc-sections
+PLATFORM_RELFLAGS += -ffunction-sections -fdata-sections
+
 # Define this to avoid linking with SDL, which requires SDL libraries
 # This can solve 'sdl-config: Command not found' errors
 ifneq ($(NO_SDL),)
diff --git a/arch/sandbox/cpu/u-boot.lds b/arch/sandbox/cpu/u-boot.lds
index 3a6cf55eb9..727bcc3598 100644
--- a/arch/sandbox/cpu/u-boot.lds
+++ b/arch/sandbox/cpu/u-boot.lds
@@ -24,8 +24,9 @@  SECTIONS
 	}
 
 	.efi_runtime : {
-		*(efi_runtime_text)
-		*(efi_runtime_data)
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
 	}
 
 	.__efi_runtime_stop : {
@@ -38,8 +39,8 @@  SECTIONS
 	}
 
 	.efi_runtime_rel : {
-		*(.relefi_runtime_text)
-		*(.relefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
 	}
 
 	.efi_runtime_rel_stop :
diff --git a/arch/x86/config.mk b/arch/x86/config.mk
index 5f77f98e60..3759d466ad 100644
--- a/arch/x86/config.mk
+++ b/arch/x86/config.mk
@@ -27,7 +27,7 @@  else
 PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common -m64
 endif
 
-PLATFORM_RELFLAGS += -ffunction-sections -fvisibility=hidden
+PLATFORM_RELFLAGS += -ffunction-sections -fdata-sections -fvisibility=hidden
 
 PLATFORM_LDFLAGS += -Bsymbolic -Bsymbolic-functions
 PLATFORM_LDFLAGS += -m $(if $(IS_32BIT),elf_i386,elf_x86_64)
diff --git a/arch/x86/cpu/u-boot.lds b/arch/x86/cpu/u-boot.lds
index f0719368ba..fed006deb7 100644
--- a/arch/x86/cpu/u-boot.lds
+++ b/arch/x86/cpu/u-boot.lds
@@ -17,6 +17,21 @@  SECTIONS
 
 	. = CONFIG_SYS_TEXT_BASE;	/* Location of bootcode in flash */
 	__text_start = .;
+
+	.__efi_runtime_start : {
+		*(.__efi_runtime_start)
+	}
+
+	.efi_runtime : {
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
+	}
+
+	.__efi_runtime_stop : {
+		*(.__efi_runtime_stop)
+	}
+
 	.text  : { *(.text*); }
 
 	. = ALIGN(4);
@@ -43,27 +58,14 @@  SECTIONS
 
 	. = ALIGN(4);
 
-	.__efi_runtime_start : {
-		*(.__efi_runtime_start)
-	}
-
-	.efi_runtime : {
-		*(efi_runtime_text)
-		*(efi_runtime_data)
-	}
-
-	.__efi_runtime_stop : {
-		*(.__efi_runtime_stop)
-	}
-
 	.efi_runtime_rel_start :
 	{
 		*(.__efi_runtime_rel_start)
 	}
 
 	.efi_runtime_rel : {
-		*(.relefi_runtime_text)
-		*(.relefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
 	}
 
 	.efi_runtime_rel_stop :
diff --git a/board/qualcomm/dragonboard410c/u-boot.lds b/board/qualcomm/dragonboard410c/u-boot.lds
index dc3f718b05..fc1bba8cf0 100644
--- a/board/qualcomm/dragonboard410c/u-boot.lds
+++ b/board/qualcomm/dragonboard410c/u-boot.lds
@@ -20,6 +20,19 @@  SECTIONS
 		*(.__image_copy_start)
 		board/qualcomm/dragonboard410c/head.o (.text*)
 		CPUDIR/start.o (.text*)
+	}
+
+	/* This needs to come before *(.text*) */
+	.efi_runtime : {
+                __efi_runtime_start = .;
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
+                __efi_runtime_stop = .;
+	}
+
+	.text_rest :
+	{
 		*(.text*)
 	}
 
@@ -51,8 +64,8 @@  SECTIONS
 
 	.efi_runtime_rel : {
                 __efi_runtime_rel_start = .;
-		*(.relaefi_runtime_text)
-		*(.relaefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
                 __efi_runtime_rel_stop = .;
 	}
 
diff --git a/board/qualcomm/dragonboard820c/u-boot.lds b/board/qualcomm/dragonboard820c/u-boot.lds
index bcf5738d38..dcf8256cec 100644
--- a/board/qualcomm/dragonboard820c/u-boot.lds
+++ b/board/qualcomm/dragonboard820c/u-boot.lds
@@ -20,6 +20,19 @@  SECTIONS
 		*(.__image_copy_start)
 		board/qualcomm/dragonboard820c/head.o (.text*)
 		CPUDIR/start.o (.text*)
+	}
+
+	/* This needs to come before *(.text*) */
+	.efi_runtime : {
+                __efi_runtime_start = .;
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
+                __efi_runtime_stop = .;
+	}
+
+	.text_rest :
+	{
 		*(.text*)
 	}
 
@@ -42,17 +55,10 @@  SECTIONS
 
 	. = ALIGN(8);
 
-	.efi_runtime : {
-                __efi_runtime_start = .;
-		*(efi_runtime_text)
-		*(efi_runtime_data)
-                __efi_runtime_stop = .;
-	}
-
 	.efi_runtime_rel : {
                 __efi_runtime_rel_start = .;
-		*(.relaefi_runtime_text)
-		*(.relaefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
                 __efi_runtime_rel_stop = .;
 	}
 
diff --git a/board/ti/am335x/u-boot.lds b/board/ti/am335x/u-boot.lds
index a56cc8216b..03c1d5f73b 100644
--- a/board/ti/am335x/u-boot.lds
+++ b/board/ti/am335x/u-boot.lds
@@ -37,6 +37,25 @@  SECTIONS
 		*(.vectors)
 		CPUDIR/start.o (.text*)
 		board/ti/am335x/built-in.o (.text*)
+	}
+
+	/* This needs to come before *(.text*) */
+	.__efi_runtime_start : {
+		*(.__efi_runtime_start)
+	}
+
+	.efi_runtime : {
+		*(.text.efi_runtime*)
+		*(.rodata.efi_runtime*)
+		*(.data.efi_runtime*)
+	}
+
+	.__efi_runtime_stop : {
+		*(.__efi_runtime_stop)
+	}
+
+	.text_rest :
+	{
 		*(.text*)
 	}
 
@@ -59,27 +78,14 @@  SECTIONS
 
 	. = ALIGN(4);
 
-	.__efi_runtime_start : {
-		*(.__efi_runtime_start)
-	}
-
-	.efi_runtime : {
-		*(efi_runtime_text)
-		*(efi_runtime_data)
-	}
-
-	.__efi_runtime_stop : {
-		*(.__efi_runtime_stop)
-	}
-
 	.efi_runtime_rel_start :
 	{
 		*(.__efi_runtime_rel_start)
 	}
 
 	.efi_runtime_rel : {
-		*(.relefi_runtime_text)
-		*(.relefi_runtime_data)
+		*(.rel*.efi_runtime)
+		*(.rel*.efi_runtime.*)
 	}
 
 	.efi_runtime_rel_stop :
diff --git a/include/efi_loader.h b/include/efi_loader.h
index c66252a7dd..d6e1f50e22 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -398,8 +398,8 @@  static inline int guidcmp(const efi_guid_t *g1, const efi_guid_t *g2)
  * Use these to indicate that your code / data should go into the EFI runtime
  * section and thus still be available when the OS is running
  */
-#define __efi_runtime_data __attribute__ ((section ("efi_runtime_data")))
-#define __efi_runtime __attribute__ ((section ("efi_runtime_text")))
+#define __efi_runtime_data __attribute__ ((section (".data.efi_runtime")))
+#define __efi_runtime __attribute__ ((section (".text.efi_runtime")))
 
 /* Call this with mmio_ptr as the _pointer_ to a pointer to an MMIO region
  * to make it available at runtime */