From 8922227dde11f838effd39e5490233da00ff254e Mon Sep 17 00:00:00 2001 From: Alrik Vidstrom Date: Fri, 3 Nov 2023 11:19:01 +0100 Subject: [PATCH 1/2] Fix MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL redefine bug The redefines break paragraph 3.2.4 of the C++14 specification (a subcategory of the ODR - One Definition Rule) in an insidious way. There's no diagnostic required in these cases, which means no warnings, no errors required by the compiler. It's also undefined behavior. There's a conditional compilation of the function call_fn() in the Callback implementation, and in practice, it's inlined in the call() function in the same object file. Depending on the MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL setting you get an extra "ldr r3,[r3, #0]" instruction or not inlined in call(). Without that indirection, calling a nontrivial callback leads to the execution of "bx r3", where r3 contains the address of the ops structure (dynamically dispatched operations), instead of the address contained in the first member of that structure, which would lead to the type-erased function pointer. And because the structure is at an even address, there's a UsageFault when the CPU tries to enter Arm mode, which isn't supported on Cortex-M. When there's a single definition of MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL everything is fine, because there's only one definition of call_fn() and call() - the last function of the two is the only one that exists in practice, in the binary. But when we redefine MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL we get two definitions of the call() function at the same time, breaking paragraph 3.2.4. And remember, no diagnostic required, and this is undefined behavior. So anything goes. On macOS, USBDevice::endpoint_add() is then linked against call() from USBHost.o, which contains the extra "ldr r3,[r3, #0]" instruction. On Windows, it's instead linked against the call() function from USBHostSerial.o, which contains no indirection, and it leads to a crash. These two translation units are compiled with different settings because the redefines are applied to some parts of the library but not all of them. Remember, breaking 3.2.4 is undefined behavior, so it's ok to link this at random. But keeping them equal wouldn't be ok anyway because 3.2.4 would still be broken by the library in combination with the core. --- src/USBHost/USBDeviceConnected.h | 2 -- src/USBHost/USBEndpoint.h | 2 -- src/USBHost/USBHostConf.h | 2 -- 3 files changed, 6 deletions(-) diff --git a/src/USBHost/USBDeviceConnected.h b/src/USBHost/USBDeviceConnected.h index 30d3362..8cbae7d 100644 --- a/src/USBHost/USBDeviceConnected.h +++ b/src/USBHost/USBDeviceConnected.h @@ -21,8 +21,6 @@ #include "USBHost/USBEndpoint.h" #include "USBHost/USBHostConf.h" #include "rtos.h" -#undef MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL -#define MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL 0 #include "Callback.h" class USBHostHub; diff --git a/src/USBHost/USBEndpoint.h b/src/USBHost/USBEndpoint.h index d254d0d..aa77902 100644 --- a/src/USBHost/USBEndpoint.h +++ b/src/USBHost/USBEndpoint.h @@ -17,8 +17,6 @@ #ifndef USBENDPOINT_H #define USBENDPOINT_H -#undef MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL -#define MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL 0 #include "Callback.h" #include "USBHostTypes.h" #include "rtos.h" diff --git a/src/USBHost/USBHostConf.h b/src/USBHost/USBHostConf.h index e28640a..d94963f 100644 --- a/src/USBHost/USBHostConf.h +++ b/src/USBHost/USBHostConf.h @@ -17,8 +17,6 @@ #ifndef USBHOST_CONF_H #define USBHOST_CONF_H -#undef MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL -#define MBED_CONF_PLATFORM_CALLBACK_NONTRIVIAL 0 #include "Callback.h" #include "Arduino.h" From c409e1fbbb2f1e9f8230e6aa6a2fe3324597244d Mon Sep 17 00:00:00 2001 From: Alrik Vidstrom Date: Fri, 3 Nov 2023 20:23:27 +0100 Subject: [PATCH 2/2] Fix missing default Mbed configuration Parts of the library don't know about the default Mbed configuration. If only the redefines are removed, Callback.h is included already in USBHostConf.h, but without any default Mbed configuration, so we get the trivial version of call(). Next, Arduino.h is included, which includes the Mbed configuration indirectly, but the damage has already been done. Over in USBHostSerial.h, USBHostConf.h is included. After that, Callback.h is included in USBHostSerial.h, and we have the correct Mbed configuration in place, but since it's already been included once, the correct version of call() is never actually used. So we get two versions of call() again, which is undefined behavior, and a crash. --- src/USBHost/USBDeviceConnected.h | 1 + src/USBHost/USBEndpoint.h | 1 + src/USBHost/USBHostConf.h | 1 + src/USBHostSerial/USBHostSerial.h | 1 + 4 files changed, 4 insertions(+) diff --git a/src/USBHost/USBDeviceConnected.h b/src/USBHost/USBDeviceConnected.h index 8cbae7d..8d317ca 100644 --- a/src/USBHost/USBDeviceConnected.h +++ b/src/USBHost/USBDeviceConnected.h @@ -21,6 +21,7 @@ #include "USBHost/USBEndpoint.h" #include "USBHost/USBHostConf.h" #include "rtos.h" +#include "mbed_config.h" #include "Callback.h" class USBHostHub; diff --git a/src/USBHost/USBEndpoint.h b/src/USBHost/USBEndpoint.h index aa77902..c85004a 100644 --- a/src/USBHost/USBEndpoint.h +++ b/src/USBHost/USBEndpoint.h @@ -17,6 +17,7 @@ #ifndef USBENDPOINT_H #define USBENDPOINT_H +#include "mbed_config.h" #include "Callback.h" #include "USBHostTypes.h" #include "rtos.h" diff --git a/src/USBHost/USBHostConf.h b/src/USBHost/USBHostConf.h index d94963f..7145219 100644 --- a/src/USBHost/USBHostConf.h +++ b/src/USBHost/USBHostConf.h @@ -17,6 +17,7 @@ #ifndef USBHOST_CONF_H #define USBHOST_CONF_H +#include "mbed_config.h" #include "Callback.h" #include "Arduino.h" diff --git a/src/USBHostSerial/USBHostSerial.h b/src/USBHostSerial/USBHostSerial.h index eef193e..e663cfa 100644 --- a/src/USBHostSerial/USBHostSerial.h +++ b/src/USBHostSerial/USBHostSerial.h @@ -24,6 +24,7 @@ #include "USBHost/USBHost.h" #include "Stream.h" #include "MtxCircBuffer.h" +#include "mbed_config.h" #include "Callback.h" /**