wasp-os / wasp-os

A MicroPython based development environment for smart watches (including Pine64 PineTime)
https://wasp-os.readthedocs.io/
GNU General Public License v3.0
837 stars 228 forks source link

bootloader does not compile with gcc-11 (-Werror=maybe-uninitialized) #209

Closed Liorst4 closed 3 years ago

Liorst4 commented 3 years ago
make BOARD=pinetime all
rm -f bootloader/_build-pinetime_nrf52832//pinetime_nrf52832_bootloader-*-nosd.hex
make -C bootloader/ BOARD=pinetime_nrf52832 all genhex
make[1]: Entering directory 'wasp-os/bootloader'
CC main.c
src/main.c: In function 'proc_ble':
src/main.c:421:24: error: '*(ble_evt_t *)(&ev_buf[0]).header.evt_id' may be used uninitialized [-Werror=maybe-uninitialized]
  421 |     switch (evt->header.evt_id)
      |             ~~~~~~~~~~~^~~~~~~
src/main.c:410:22: note: 'ev_buf' declared here
  410 |   __ALIGN(4) uint8_t ev_buf[ BLE_EVT_LEN_MAX(BLEGATT_ATT_MTU_MAX) ];
      |                      ^~~~~~
src/main.c: In function 'proc_soc':
src/main.c:452:5: error: 'soc_evt' may be used uninitialized [-Werror=maybe-uninitialized]
  452 |     pstorage_sys_event_handler(soc_evt);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/main.c:447:12: note: 'soc_evt' declared here
  447 |   uint32_t soc_evt;
      |            ^~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:355: _build-pinetime_nrf52832/main.o] Error 1
make[1]: Leaving directory 'wasp-os/bootloader'
make: *** [Makefile:38: bootloader] Error 2
Liorst4 commented 3 years ago

BTW im using gcc-11

sethmiller commented 3 years ago

I get the same issue. You can workaround by patching the bootlader makefile to ignore the error (ignoring errors is probably not a good permanent fix though):

diff --git a/Makefile b/Makefile
index bca5edf..e0c0de8 100644
--- a/Makefile
+++ b/Makefile
@@ -231,6 +231,7 @@ CFLAGS += -mcpu=cortex-m4
 CFLAGS += -mthumb -mabi=aapcs --std=gnu99
 CFLAGS += -Wall -Werror -Os -g3
 CFLAGS += -mfloat-abi=hard -mfpu=fpv4-sp-d16
+CFLAGS += -Wno-maybe-uninitialized

 # keep every function in separate section. This will allow linker to dump unused functions
 CFLAGS += -ffunction-sections -fdata-sections -fno-strict-aliasing
daniel-thompson commented 3 years ago

Does the bootloader actualy work after these changes? There are reports that the gcc-11 from ArchLinux compiles ok but results in a bootloader that boot loops continually (which is enough to break sealed units).. and I was able to reproduce when i tried the same thing

sethmiller commented 3 years ago

That's what happened to me. I had assumed I had done something else wrong and have not had time to try to get it back to a good state (thankfully it was an open unit so I should be able to).

Any pointers on getting it up and running again? Is there I guide I can run through for flashing via a physical connection?

daniel-thompson commented 3 years ago

Any pointers on getting it up and running again? Is there I guide I can run through for flashing via a physical connection?

See https://wiki.pine64.org/wiki/Reprogramming_the_PineTime

daniel-thompson commented 3 years ago

So I think I have found the cause of the broken bootloader with gcc-11.

I have found what I think it is a broken bit of macro and inline assembler magic in the Nordic Softdevice header files. The inline assembler code does not currently describe its input, output or clobbers. Historically that didn't matter because the compiler wasn't smart enough to determine that the macro generated assembler functions were ignoring their arguments and therefore "acidentally" generated correct code. Newer compilers are able to study this and conclude they can discard redundant paramter setup which (acording to the inline assembler contraints) is not used.

This diff causes correct code generation... but I haven't tested it on real hardware yet:

diff --git a/lib/softdevice/s132_nrf52_6.1.1/s132_nrf52_6.1.1_API/include/nrf_svc.h b/lib/softdevice/s132_nrf52_6.1.1/s132_nrf52_6.1.1_API/include/nrf_svc.h
index 292c692..590fdee 100644
--- a/lib/softdevice/s132_nrf52_6.1.1/s132_nrf52_6.1.1_API/include/nrf_svc.h
+++ b/lib/softdevice/s132_nrf52_6.1.1/s132_nrf52_6.1.1_API/include/nrf_svc.h
@@ -58,19 +58,38 @@ extern "C" {
 #else
 #define GCC_CAST_CPP
 #endif
+
+__attribute__((weak))
+long wrap_svc_for_svcall(long l0, long l1, long l2, long l3)
+{
+   /*
+    * These register allocations match the AArch32 proceedure
+    * call standard and should not result in any additional code
+    * generation.
+    */
+        register long r0 asm("r0") = l0;
+        register long r1 asm("r1") = l1;
+        register long r2 asm("r2") = l2;
+        register long r3 asm("r3") = l3;
+
+        asm volatile ("svc %1\n"
+                : "+r" (r0)
+                : "I" (0x18), "r" (r1), "r" (r2), "r" (r3)
+                : "memory");
+
+        return r0;
+}
+
+/*
+ * This cannot be handled with a push/pop inside the SVCALL macro
+ * since it appears to be generated at a different point during
+ * parseing. Thus we are forced to disable it unilaterally.
+ */
+#pragma GCC diagnostic ignored "-Wattribute-alias"
+
 #define SVCALL(number, return_type, signature)          \
-  _Pragma("GCC diagnostic push")                        \
-  _Pragma("GCC diagnostic ignored \"-Wreturn-type\"")   \
-  __attribute__((naked))                                \
-  __attribute__((unused))                               \
-  static return_type signature                          \
-  {                                                     \
-    __asm(                                              \
-        "svc %0\n"                                      \
-        "bx r14" : : "I" (GCC_CAST_CPP number) : "r0"   \
-    );                                                  \
-  }                                                     \
-  _Pragma("GCC diagnostic pop")
+  __attribute__((alias("wrap_svc_for_svcall")))         \
+  static return_type signature;

 #elif defined (__ICCARM__)
 #define PRAGMA(x) _Pragma(#x)               
daniel-thompson commented 3 years ago

So... I think I was right about the cause of the probelm but the above workaround doesn't quite work so, for now, I have added a different one.

As a side effect this also fixes the warnings without code changes.