search

adafruit / Adafruit_nRF52_Bootloader

USB-enabled bootloaders for the nRF52 BLE SoC chips
MIT License
438 stars 393 forks source link

GCC 11 Build fixes #211

Closed DuMaM closed 3 years ago

DuMaM commented 3 years ago

Fixed build errors for GCC11

src/main.c:456:5: error: 'soc_evt' may be used uninitialized [-Werror=maybe-uninitialized]
  456 |     pstorage_sys_event_handler(soc_evt);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/main.c: In function 'proc_ble':
src/main.c:425:24: error: '*(ble_evt_t *)(&ev_buf[0]).header.evt_id' may be used uninitialized [-Werror=maybe-uninitialized]
  425 |     switch (evt->header.evt_id)
      |             ~~~~~~~~~~~^~~~~~~

lib/sdk11/components/ble/ble_services/ble_dfu/ble_dfu.c: In function 'ble_dfu_init':
lib/sdk11/components/ble/ble_services/ble_dfu/ble_dfu.c:487:36: error: 'service_uuid.type' may be used uninitialized [-Werror=maybe-uninitialized]
  487 |     p_dfu->uuid_type = service_uuid.type;
      |                        ~~~~~~~~~~~~^~~~~
DuMaM commented 3 years ago

I also fixed build for one submodule https://github.com/hathach/tinyusb/pull/923

DuMaM commented 3 years ago

Anyway i noticed that something is wrong with bootloader itself when i complie it with gcc11. I dont know why I'm unable to flash any image and boot to them. In both uf2 and after flashing with JTAG...

One I took from Github Releases works without issues

hathach commented 3 years ago

Anyway i noticed that something is wrong with bootloader itself when i complie it with gcc11. I dont know why I'm unable to flash any image and boot to them. In both uf2 and after flashing with JTAG...

One I took from Github Releases works without issues

which command you use for flashing, also which board and setup you are using ? The release is built with gcc 9.3.1 https://github.com/adafruit/Adafruit_nRF52_Bootloader/blob/master/.github/workflows/githubci.yml#L71 Since I run into an issue with one of later gcc and haven't got time to address it since. It may have something with optimization etc with latest gcc, feel free to open an separate issue with more info. We can easier address it there, since this PR will be closed when merged.

DuMaM commented 3 years ago

Anyway i noticed that something is wrong with bootloader itself when i complie it with gcc11. I dont know why I'm unable to flash any image and boot to them. In both uf2 and after flashing with JTAG... One I took from Github Releases works without issues

which command you use for flashing, also which board and setup you are using ? The release is built with gcc 9.3.1 https://github.com/adafruit/Adafruit_nRF52_Bootloader/blob/master/.github/workflows/githubci.yml#L71 Since I run into an issue with one of later gcc and haven't got time to address it since. It may have something with optimization etc with latest gcc, feel free to open an separate issue with more info. We can easier address it there, since this PR will be closed when merged.

So the same command works for release version and do not work for GCC11

Build

make BOARD=particle_xenon all
...
...
...
LD particle_xenon_bootloader-0.6.0-2-g6e33825-dirty.out
   text    data     bss     dec     hex filename
  31992    1576   22578   56146    db52 _build/build-particle_xenon/particle_xenon_bootloader-0.6.0-2-g6e33825-dirty.out
Create particle_xenon_bootloader-0.6.0-2-g6e33825-dirty.hex
Create particle_xenon_bootloader-0.6.0-2-g6e33825-dirty_nosd.hex
Create update-particle_xenon_bootloader-0.6.0-2-g6e33825-dirty_nosd.uf2
Converting to uf2, output size: 73728, start address: 0x0
Wrote 73728 bytes to _build/build-particle_xenon/update-particle_xenon_bootloader-0.6.0-2-g6e33825-dirty_nosd.uf2
Create particle_xenon_bootloader-0.6.0-2-g6e33825-dirty_s140_6.1.1.hex
Zip created at _build/build-particle_xenon/particle_xenon_bootloader-0.6.0-2-g6e33825-dirty_s140_6.1.1.zip

Flashing

#!/bin/bash

# https://docs.particle.io/tutorials/learn-more/xenon-circuit-python/

FIRMWARE=$(find . -name "particle_xenon*s140_*.hex")
echo "Flashing $FIRMWARE"

openocd -f interface/ftdi/jtag-lock-pick_tiny_2.cfg \
        -c "transport select swd" \
        -f target/nrf52.cfg \
        -c "init; reset; halt" \
        -c "nrf5 mass_erase" \
        -c "program $FIRMWARE 0x00000000 verify reset" \
        -c "exit"

Tools

Using built-in specs.
COLLECT_GCC=arm-none-eabi-gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/arm-none-eabi/11.1.0/lto-wrapper
Target: arm-none-eabi
Configured with: /build/arm-none-eabi-gcc/src/gcc-11.1.0/configure --target=arm-none-eabi --prefix=/usr --with-sysroot=/usr/arm-none-eabi --with-native-system-header-dir=/include --libexecdir=/usr/lib --enable-languages=c,c++ --enable-plugins --disable-decimal-float --disable-libffi --disable-libgomp --disable-libmudflap --disable-libquadmath --disable-libssp --disable-libstdcxx-pch --disable-nls --disable-shared --disable-threads --disable-tls --with-gnu-as --with-gnu-ld --with-system-zlib --with-newlib --with-headers=/usr/arm-none-eabi/include --with-python-dir=share/gcc-arm-none-eabi --with-gmp --with-mpfr --with-mpc --with-isl --with-libelf --enable-gnu-indirect-function --with-host-libstdcxx='-static-libgcc -Wl,-Bstatic,-lstdc++,-Bdynamic -lm' --with-pkgversion='Arch Repository' --with-bugurl=https://bugs.archlinux.org/ --with-multilib-list=rmprofile
Thread model: single
Supported LTO compression algorithms: zlib zstd
gcc version 11.1.0 (Arch Repository) 

arm-none-eabi-newlib-4.1.0
arm-none-eabi-binutils-2.36.1

I assume there are collisions with latest tinyusb because I also have to use a master version for it, due to gcc fixes. Ok I will create an issue with all this description.

hathach commented 3 years ago

@DuMaM latest tinyusb could have its own breaking changes. It is best for you to stick to the version of the sub-modules. You could cherry-pick or have a modification to suppress the warning instead of bump it up to master. Once after a while, I will do bump up tinyusb when I think is needed since not all changes is needed for this particular bootloader.

PS: yeah, please open separated issue with detail information for your issue, this PR is merged and will be considered as closed.

daniel-thompson commented 2 years ago

Sorry for poking an old thread but was the bootloader tested after being compiled with gcc-11 or was it just build tested?

The reason I ask is that I've been looking into some broken codegen with gcc-11 and concluded that the data is not actually uninitialized meaning the changes to suppress the warnings end up hiding a real codegen problem.

The Nordic Softdevice SDK headers contain broken inline assembler contraints and, when combined with inter-proceedural optimizations, the code gen breaks as "dead" code (that is not actually dead) is striped from the resulting binaries. The result of these broken assembler contraints is that the compiler incorrectly concludes some of the arguments to functions calls that used the SVCALL() macro are unused. This doesn't just results in the uninitialized data warnings that this PR papers over. It also causes the call sites not to bother setting up the arguments in the first place. The result of this is that the bootloader makes svc calls to the SoftDevice without initializing the arguments.

At a codegen level it looks like this:

Dump of assembler code for function softdev_mbr_init:
   0x000f4788 <+0>:     sub     sp, #16
   0x000f478a <+2>:     mov     r0, sp
   0x000f478c <+4>:     add     sp, #16
   0x000f478e <+6>:     b.w     0xf4758 <sd_mbr_command>

Naturally the best fix is to fix the SVCALL() macro but this is pretty complex (because I can't find a way to fix it without changing all the sites that use the macro). However you can turn off one of the inter-procedural optimization paths. That has the effect of fixing the warnings (without any extra initialization) and keeping the codegen the same as gcc-10.

In other words, with -fno-ipa-modref, then the codegen is fixed:

(gdb) disass softdev_mbr_init 
Dump of assembler code for function softdev_mbr_init:
   0x000f4788 <+0>:     push    {r0, r1, r2, r3, r4, lr}
   0x000f478a <+2>:     movs    r3, #0
   0x000f478c <+4>:     strd    r3, r3, [sp, #4]
   0x000f4790 <+8>:     str     r3, [sp, #12]
   0x000f4792 <+10>:    mov     r0, sp
   0x000f4794 <+12>:    movs    r3, #2
   0x000f4796 <+14>:    str     r3, [sp, #0]
   0x000f4798 <+16>:    bl      0xf4758 <sd_mbr_command>
   0x000f479c <+20>:    add     sp, #20
   0x000f479e <+22>:    ldr.w   pc, [sp], #4

Obviously a proper fix would fix the inline assembler contraints in the SVCALL() macro. Unfortunately this requires restrucuting all the call sites (because to get the contraints correct we must know the number and names of the function arguments). Thus for now I have been making a simpler change (with also fixes all the uninitialized variable warnings):

diff --git a/Makefile b/Makefile
index 238dbb9..e2d5fc4 100644
--- a/Makefile
+++ b/Makefile
@@ -270,6 +270,16 @@ CFLAGS += -Wno-unused-parameter -Wno-expansion-to-defined
 # TinyUSB tusb_hal_nrf_power_event
 CFLAGS += -Wno-cast-function-type

+# Nordic Softdevice SDK header files contain inline assembler that has^M
+# broken contraints. As a result the IPA-modref pass is able to "prove"^M
+# that arguments to wrapper functions generated with the SVCALL() macro^M
+# are unused and, as a result, the optimizer removes the code in the callers^M
+# that sets up these arguments (which result in broken syscalls). Since^M
+# the broken headers come from Nordic-supplied zip files and are not^M
+# easily patched then, for now, we'll simply disable some of the^M
+# gcc-11 inter-procedural optimizations.^M
+CFLAGS += -fno-ipa-modref^M
+^M
 # Defined Symbol (MACROS)
 CFLAGS += -D__HEAP_SIZE=0
 CFLAGS += -DCONFIG_GPIO_AS_PINRESET

I'm happy to submit a PR if this approach to fixing the codegen is acceptable as a temporary workaround.

hathach commented 2 years ago

@daniel-thompson Accepted !! it is great workaround if we could get SVCALL to work with latest gcc. It has been a puzzle to me and couldn't find time to investigate it. The gcc is fixed with 9.3.1 for ci testing and generating binaries https://github.com/adafruit/Adafruit_nRF52_Bootloader/blob/master/.github/workflows/githubci.yml#L74

Please make an PR, I will try to test it out with my actual hardware