From bdd0b63d7c1cb21745e095e0ad97c8d5f7edcf16 Mon Sep 17 00:00:00 2001 From: "Maarten \"merethan\" van Eeuwijk" Date: Mon, 23 Oct 2023 12:45:08 +0200 Subject: [PATCH 1/2] Zeroing of section .bss is now conditional on .bss start & end being defined rather than .data start & end being defined (this was likely a copy-pasta error when re-purposing code from the .data section above it to make the .bss piece). Also, the pointer jumps forward 4 bytes at a time now rather than one byte, given that pSrc and pDest are pointers to a uint32_t, meaning memory is written 4 bytes at a time. A bunch of processor architectures (especially the simple ones) can only write int32 values to a memory address aligned at a 4-byte boundary, and int16 values aligned at a 2-byte boundary. If one writes C/C++ code not taking this into account, the compiler is forced to generate all sorts of shifts and tricks to make that code work, which is very suboptimal code by every measure. (In this case suboptimal means having every byte but the first and last three written 4 or more times over and some extra space in flash memory for the tricks.) If one were to do have written the init of .data and .bss in assembly (meaning the compiler not saves your ass by inserting shifts & tricks to your doing it per-byte) this would have been a HardFault on most Cortex CPUs due to the alignment requirement. --- cores/arduino/cortex_handlers.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cores/arduino/cortex_handlers.c b/cores/arduino/cortex_handlers.c index e125b6ee8..e6ca5608d 100644 --- a/cores/arduino/cortex_handlers.c +++ b/cores/arduino/cortex_handlers.c @@ -146,13 +146,13 @@ void Reset_Handler(void) pDest = &__data_start__; if ((&__data_start__ != &__data_end__) && (pSrc != pDest)) { - for (; pDest < &__data_end__; pDest++, pSrc++) + for (; pDest < &__data_end__; pDest += sizeof(uint32_t), pSrc += sizeof(uint32_t)) *pDest = *pSrc; } /* Clear the zero section */ - if ((&__data_start__ != &__data_end__) && (pSrc != pDest)) { - for (pDest = &__bss_start__; pDest < &__bss_end__; pDest++) + if ((&__bss_start__ != &__bss_end__)) { + for (pDest = &__bss_start__; pDest < &__bss_end__; pDest += sizeof(uint32_t)) *pDest = 0; } From e80e035f82a0f1c26c08738228ed2818fd958f75 Mon Sep 17 00:00:00 2001 From: "Maarten \"merethan\" van Eeuwijk" Date: Mon, 30 Oct 2023 15:53:50 +0100 Subject: [PATCH 2/2] This patch undoes a patch submitted by Cristian Maglie on Tue Oct 11 18:36:08 2016 (ce89e7e2446ec96c8ad6052c3038bf2b823104fa). Why: Construction of global objects is something which needs be done prior running main(). It has been like this since the conception of Unix and the C language that was created to write Unix in and since then standardized in the Portable Operating System Interface (POSIX): Initialization of memory is done by entry point _start() which does stuff like loading commandline arguments from the stack and runs main() with it, and calls _exit() on return of main(). The function _exit() in the old-time days of mainframes was responsible for flushing buffers and relinquish all other resources (memory, sockets etc.) such the next user/program can use them (nowadays operating systems are smarter and got your back if programs fail to do this properly). On embedded systems (such as those running Arduino) there is no program loader or operating system, meaning these duties are to be done by what is commonly referred to as the Reset Handler, being the entry point of the program like _start() is on a POSIX-compliant machine. By moving the duties of _start() to main() itself a variety of problems may arise involving a variety of static initialization problems that are very hard to spot. And if not today there will be in the future because the current behavior is not expected. (It is anti-logical; things that are to be done prior main() should not be in main() itself.) If this breaks libraries, it means these unnamed "some libraries" are broken: Things like hardware init should be in begin(), not a class constructor. And most definitely should one class constructor never rely on another class constructor having done something already, like initializing hardware. This is because the compiler has no good control over what class is initialized first: All the compiler can do is specify an order of importance, which the linker then must be instructed to follow, which the C library then needs to run correctly in that order. None of the problems that occur due to one of those steps going not as expected are easy to spot or in any way obvious from C/C++ code. Or easy to fix for that matter. And none of this is formally specified and hence implementation-specific. Type "The Static Initialization Order Fiasco" into a search engine of your choice. Or, to cut this story down to the shortest possible way to put it: Constructors are for creating constructs in memory, nothing else. Procedural code does not belong there, it should be in begin(). Any library needing Cristian Maglie's hack is broken and should be fixed. Compromising Arduino core is not a fix. --- cores/arduino/cortex_handlers.c | 6 ++++++ cores/arduino/main.cpp | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/cores/arduino/cortex_handlers.c b/cores/arduino/cortex_handlers.c index e6ca5608d..ee2065a17 100644 --- a/cores/arduino/cortex_handlers.c +++ b/cores/arduino/cortex_handlers.c @@ -20,6 +20,9 @@ #include #include +/* libc */ +void __libc_init_array(void); + /* RTOS Hooks */ extern void svcHook(void); extern void pendSVHook(void); @@ -156,6 +159,9 @@ void Reset_Handler(void) *pDest = 0; } + /* Initialize C library */ + __libc_init_array(); + SystemInit(); main(); diff --git a/cores/arduino/main.cpp b/cores/arduino/main.cpp index a6d2cb5f5..6a771b3ad 100644 --- a/cores/arduino/main.cpp +++ b/cores/arduino/main.cpp @@ -26,9 +26,6 @@ void initVariant() { } extern USBDeviceClass USBDevice; -// Initialize C library -extern "C" void __libc_init_array(void); - /* * \brief Main entry point of Arduino application */ @@ -36,8 +33,6 @@ int main( void ) { init(); - __libc_init_array(); - initVariant(); delay(1);