efi_loader: Rename sections to allow for implicit data

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 <[email protected]>

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 <[email protected]>

---
 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(-)

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 85 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 1 line

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

Hi Alex,

Quoted 85 lines · Alexander Graf

Quoted 85 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Hi,

Quoted 114 lines · Bin Meng

Hi Alex,

Quoted 85 lines · Alexander Graf

Quoted 85 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 112 lines · Simon Glass

Hi,

Quoted 109 lines · Bin Meng

Hi Alex,

Quoted 83 lines · Alexander Graf

Quoted 83 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 5 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 7 lines

Quoted 4 lines

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.

Hi,

Quoted 109 lines · Bin Meng

Hi Alex,

Quoted 83 lines · Alexander Graf

Quoted 83 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 5 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 118 lines · Simon Glass

Hi,

Quoted 114 lines · Bin Meng

Hi Alex,

Quoted 84 lines · Alexander Graf

Quoted 84 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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?

Quoted 5 lines

Quoted 2 lines

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?

Hi Alex,

Quoted 126 lines · Alexander Graf

Quoted 123 lines · Simon Glass

Hi,

Quoted 112 lines · Bin Meng

Hi Alex,

Quoted 84 lines · Alexander Graf

Quoted 84 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 1 line

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.

Quoted 129 lines · Simon Glass

Hi Alex,

Quoted 126 lines · Alexander Graf

Quoted 123 lines · Simon Glass

Hi,

Quoted 112 lines · Bin Meng

Hi Alex,

Quoted 84 lines · Alexander Graf

Quoted 84 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 129 lines · Simon Glass

Hi Alex,

Quoted 126 lines · Alexander Graf

Quoted 123 lines · Simon Glass

Hi,

Quoted 112 lines · Bin Meng

Hi Alex,

Quoted 84 lines · Alexander Graf

Quoted 84 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 4 lines

Quoted 1 line

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.

Hi Alex,

Quoted 137 lines · Alexander Graf

Quoted 129 lines · Simon Glass

Hi Alex,

Quoted 126 lines · Alexander Graf

Quoted 123 lines · Simon Glass

Hi,

Quoted 112 lines · Bin Meng

Hi Alex,

Quoted 84 lines · Alexander Graf

Quoted 84 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

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.

Quoted 149 lines · Simon Glass

Hi Alex,

Quoted 137 lines · Alexander Graf

Quoted 129 lines · Simon Glass

Hi Alex,

Quoted 126 lines · Alexander Graf

Quoted 123 lines · Simon Glass

Hi,

Quoted 112 lines · Bin Meng

Hi Alex,

Quoted 84 lines · Alexander Graf

Quoted 84 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Hi Tom,

Quoted 155 lines · Tom Rini

Quoted 149 lines · Simon Glass

Hi Alex,

Quoted 137 lines · Alexander Graf

Quoted 129 lines · Simon Glass

Hi Alex,

Quoted 126 lines · Alexander Graf

Quoted 123 lines · Simon Glass

Hi,

Quoted 112 lines · Bin Meng

Hi Alex,

Quoted 84 lines · Alexander Graf

Quoted 84 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 164 lines · Simon Glass

Hi Tom,

Quoted 155 lines · Tom Rini

Quoted 149 lines · Simon Glass

Hi Alex,

Quoted 137 lines · Alexander Graf

Quoted 129 lines · Simon Glass

Hi Alex,

Quoted 126 lines · Alexander Graf

Quoted 123 lines · Simon Glass

Hi,

Quoted 112 lines · Bin Meng

Hi Alex,

Quoted 84 lines · Alexander Graf

Quoted 84 lines

Am 07.08.2018 um 18:12 schrieb Simon Glass <[email protected]>:

Hi Alex,

Quoted 21 lines · Alexander Graf

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 <[email protected]>

---

Quoted 15 lines

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

Quoted 6 lines

Quoted 1 line

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:

arch/x86/config.mk +0 -1

index 586e11a..fc119ec 100644
@@ -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

Quoted 4 lines

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 14 lines

Quoted 8 lines

Quoted 3 lines

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

Quoted 7 lines

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

Quoted 11 lines

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/

Am 23.08.2018 um 18:31 schrieb Simon Glass:

Quoted 15 lines · Tom Rini

Quoted 11 lines

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:

Quoted 3 lines

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.

Quoted 16 lines · Andreas Färber

Am 23.08.2018 um 18:31 schrieb Simon Glass:

Quoted 15 lines · Tom Rini

Quoted 11 lines

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:

Quoted 7 lines

Quoted 3 lines

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?