Fix implementation of smp_atomic_compare_exchange_weak on Pico

Fix a bug in the implementation where the spinlock was allocated and released within smp_atomic_compare_exchange_weak, thus implementation was effectively only disabling interrupts on the executing core. Also update Pico tests to actually test without SMP enabled. Multicore fork of rp2040js doesn't work with spinlocks, and upstream version 0.19.1 has cyw43 emulation, paving the way for network tests. Signed-off-by: Paul Guyot <[email protected]>
pguyot · Oct 15, 2023 · 7bd109f · 7bd109f
1 parent 49a655c
commit 7bd109f
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 16 deletions.
diff --git a/.github/workflows/pico-build.yaml b/.github/workflows/pico-build.yaml
@@ -62,17 +62,25 @@ jobs:
         source $HOME/.nvm/nvm.sh
         nvm install 20
 
+    - name: Build tests (without SMP)
+      shell: bash
+      working-directory: ./src/platforms/rp2040/
+      run: |
+        set -euo pipefail
+        mkdir build.nosmp
+        cd build.nosmp
+        cmake .. -G Ninja -DPICO_BOARD=${{ matrix.board }} -DAVM_DISABLE_SMP=1
+        cmake --build . --target=rp2040_tests
+
     - name: Run tests with rp2040js
       shell: bash
       working-directory: ./src/platforms/rp2040/tests
-      # Unfortunately, rp2040js doesn't support cyw43 and cyw43_arch_init panics
-      if: ${{ success() && matrix.board != 'pico_w' }}
       run: |
         set -euo pipefail
         source $HOME/.nvm/nvm.sh
         nvm use node
         npm install
-        npx tsx run-tests.ts ../build/tests/rp2040_tests.uf2 ../build/tests/test_erl_sources/rp2040_test_modules.uf2
+        npx tsx run-tests.ts ../build.nosmp/tests/rp2040_tests.uf2 ../build.nosmp/tests/test_erl_sources/rp2040_test_modules.uf2
 
     - name: Build atomvmlib.uf2
       if: startsWith(github.ref, 'refs/tags/')

diff --git a/src/platforms/rp2040/src/lib/platform_smp.h b/src/platforms/rp2040/src/lib/platform_smp.h
@@ -35,7 +35,7 @@
 #define ATOMIC
 
 #define ATOMIC_COMPARE_EXCHANGE_WEAK(object, expected, desired) \
-    smp_atomic_compare_exchange_weak(object, expected, desired, sizeof(desired))
+    smp_atomic_compare_exchange_weak(object, expected, (uint64_t) desired, sizeof(desired))
 
 #ifndef TYPEDEF_SPINLOCK
 #define TYPEDEF_SPINLOCK
@@ -77,11 +77,27 @@ static inline void smp_spinlock_unlock(SpinLock *lock)
     mutex_exit(&lock->mutex);
 }
 
+static critical_section_t atomic_cas_section;
+
+/**
+ * @brief Initialize structures of SMP functions
+ */
+static inline void smp_init()
+{
+    critical_section_init(&atomic_cas_section);
+}
+
+/**
+ * @brief Free structures for SMP functions
+ */
+static inline void smp_free()
+{
+    critical_section_deinit(&atomic_cas_section);
+}
+
 static inline bool smp_atomic_compare_exchange_weak(void *object, void *expected, uint64_t desired, size_t desired_len)
 {
-    critical_section_t crit_sec;
-    critical_section_init(&crit_sec);
-    critical_section_enter_blocking(&crit_sec);
+    critical_section_enter_blocking(&atomic_cas_section);
 
     bool result;
     switch (desired_len) {
@@ -131,8 +147,7 @@ static inline bool smp_atomic_compare_exchange_weak(void *object, void *expected
         }
     }
 
-    critical_section_exit(&crit_sec);
-    critical_section_deinit(&crit_sec);
+    critical_section_exit(&atomic_cas_section);
     return result;
 }
 

diff --git a/src/platforms/rp2040/src/lib/rp2040_sys.h b/src/platforms/rp2040/src/lib/rp2040_sys.h
@@ -67,8 +67,10 @@
 
 struct RP2040PlatformData
 {
+#ifndef AVM_NO_SMP
     mutex_t event_poll_mutex;
     cond_t event_poll_cond;
+#endif
 };
 
 typedef void (*port_driver_init_t)(GlobalContext *global);

diff --git a/src/platforms/rp2040/src/lib/sys.c b/src/platforms/rp2040/src/lib/sys.c
@@ -52,8 +52,11 @@ void sys_init_platform(GlobalContext *glb)
 {
     struct RP2040PlatformData *platform = malloc(sizeof(struct RP2040PlatformData));
     glb->platform_data = platform;
+#ifndef AVM_NO_SMP
     mutex_init(&platform->event_poll_mutex);
     cond_init(&platform->event_poll_cond);
+    smp_init();
+#endif
 
 #ifdef LIB_PICO_CYW43_ARCH
     cyw43_arch_init();
@@ -68,6 +71,10 @@ void sys_free_platform(GlobalContext *glb)
 
     struct RP2040PlatformData *platform = glb->platform_data;
     free(platform);
+
+#ifndef AVM_NO_SMP
+    smp_free();
+#endif
 }
 
 #ifndef AVM_NO_SMP

diff --git a/src/platforms/rp2040/tests/package-lock.json b/src/platforms/rp2040/tests/package-lock.json
diff --git a/src/platforms/rp2040/tests/package.json b/src/platforms/rp2040/tests/package.json
@@ -3,7 +3,7 @@
   "description": "Tests using rp2040js emulator",
   "dependencies": {
     "@tsconfig/node20": "^20.1.2",
-    "rp2040js": "github:mingpepe/rp2040js#core1",
+    "rp2040js": "^0.19.1",
     "sync-fetch": "^0.5.2",
     "tsx": "^3.12.8",
     "typescript": "^5.2.2",

diff --git a/src/platforms/rp2040/tests/run-tests.ts b/src/platforms/rp2040/tests/run-tests.ts
@@ -96,6 +96,5 @@ mcu.uart[0].onByte = (value) => {
   }
 };
 
-mcu.core0.PC = 0x10000000;
-mcu.core1.PC = 0x10000000;
+mcu.core.PC = 0x10000000;
 mcu.execute();
diff --git a/src/platforms/rp2040/tests/test_erl_sources/CMakeLists.txt b/src/platforms/rp2040/tests/test_erl_sources/CMakeLists.txt
@@ -21,6 +21,7 @@
 ExternalProject_Add(HostAtomVM
     SOURCE_DIR ../../../../../../
     INSTALL_COMMAND cmake -E echo "Skipping install step."
+    BUILD_COMMAND cmake --build . --target=atomvmlib --target=PackBEAM
 )
 
 function(compile_erlang module_name)