core/mutex: clean up

[maribu]

Contribution description

This restores a pre-existing design decision to implement both blocking and non-blocking mutex locking with the same code. Those implementations have been split prior to the introduction of the core_mutex_priority_inheritance module when mutex_trylock() indeed was trivial. This decision didn't age well, so undo it.

Testing procedure

Let's have the CI run some tests.

Issues/PRs references

None

[benpicco]

Static tests has some comments, otherwise this looks good. Are you sure you can drop mutex_trylock_ffi()?

[chrysn]

I don't know of any code that used mutex_trylock_ffi directly -- but only reading this patch I saw that it was not supposed to be used directly (but through mutex_trylock which would already have picked the right implementation). If others made the same mistake, a note would be due that mutex_trylock_ffi is not needed any more, or a deprecated alias.

(This is sidestepping the question of whether trylock should or should not stay inlined down to the busy parts; about that, I don't care).

[maribu]

I also applied @fabian18 's comment in https://github.com/RIOT-OS/RIOT/pull/18620#discussion_r976448281

[maribu]

This looks like the rust bindings do care that mutex_lock() now is static inline :-/

---

Update this is the log of a random failed build:

-- running on worker breeze thread 1, build number 4090.
make: Entering directory '/tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/examples/rust-hello-world'
make: Leaving directory '/tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/examples/rust-hello-world'
make: Entering directory '/tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/examples/rust-hello-world'
Building application "hello-world" for "cc2538dk" with MCU "cc2538".

   Compiling proc-macro2 v1.0.39
   Compiling unicode-ident v1.0.0
   Compiling autocfg v1.1.0
   Compiling memchr v2.5.0
   Compiling libc v0.2.126
   Compiling syn v1.0.96
   Compiling glob v0.3.0
   Compiling cfg-if v1.0.0
   Compiling serde_derive v1.0.137
   Compiling log v0.4.17
   Compiling termcolor v1.1.3
   Compiling minimal-lexical v0.2.1
   Compiling regex-syntax v0.6.26
   Compiling os_str_bytes v6.1.0
   Compiling serde v1.0.137
   Compiling hashbrown v0.11.2
   Compiling bindgen v0.60.1
   Compiling either v1.6.1
   Compiling textwrap v0.15.0
   Compiling serde_json v1.0.81
   Compiling strsim v0.10.0
   Compiling bitflags v1.3.2
   Compiling humantime v2.1.0
   Compiling semver v1.0.9
   Compiling ryu v1.0.10
   Compiling rustc-hash v1.1.0
   Compiling peeking_take_while v0.1.2
   Compiling lazy_static v1.4.0
   Compiling itoa v1.0.2
   Compiling lazycell v1.3.0
   Compiling shlex v1.1.0
   Compiling byteorder v1.4.3
   Compiling cty v0.2.2
   Compiling shlex v0.1.1
   Compiling nb v1.0.0
   Compiling c2rust-asm-casts v0.2.0
   Compiling stable_deref_trait v1.2.0
   Compiling void v1.0.2
   Compiling mutex-trait v0.2.0
   Compiling bare-metal v1.0.0
   Compiling pin-utils v0.1.0
   Compiling hex v0.4.3
   Compiling rust_riotmodules v0.1.0 (/tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/sys/rust_riotmodules)
   Compiling libloading v0.7.3
   Compiling clap_lex v0.2.0
   Compiling nb v0.1.3
   Compiling hash32 v0.2.1
   Compiling embedded-graphics v0.6.2
   Compiling indexmap v1.8.2
   Compiling num-traits v0.2.15
   Compiling riot-wrappers v0.7.22 (https://github.com/RIOT-OS/rust-riot-wrappers#bc6183f0)
   Compiling embedded-hal v0.2.7
   Compiling clang-sys v1.3.3
   Compiling cstr_core v0.2.6
   Compiling rustc_version v0.4.0
   Compiling aho-corasick v0.7.18
   Compiling nom v7.1.1
   Compiling quote v1.0.18
   Compiling heapless v0.7.13
   Compiling atty v0.2.14
   Compiling which v4.2.5
   Compiling clap v3.1.18
   Compiling regex v1.5.6
   Compiling cexpr v0.6.0
   Compiling env_logger v0.9.0
   Compiling c2rust-bitfields-derive v0.2.1
   Compiling c2rust-bitfields v0.3.0
   Compiling riot-sys v0.7.7 (https://github.com/RIOT-OS/rust-riot-sys#c7e6e9e8)
error[E0425]: cannot find function `mutex_lock` in crate `riot_sys`
  --> /data/riotbuild/.cargo/git/checkouts/rust-riot-wrappers-155be8faf0b21fa1/bc6183f/src/mutex.rs:45:28
   |
45 |         unsafe { riot_sys::mutex_lock(crate::inline_cast_mut(self.mutex.get())) };
   |                            ^^^^^^^^^^
   |
  ::: /tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/build/target/thumbv7m-none-eabi/release/build/riot-sys-ee3ae4161c94057e/out/bindings.rs:3:699110
   |
3  | ...ust not be NULL."] pub fn rmutex_lock (rmutex : * mut rmutex_t) ; } extern "C" { # [doc = " @brief Unlocks the recursive mutex."] # [d...
   |                       -------------------------------------------- similarly named function `rmutex_lock` defined here
   |
help: a function with a similar name exists
   |
45 |         unsafe { riot_sys::rmutex_lock(crate::inline_cast_mut(self.mutex.get())) };
   |                            ~~~~~~~~~~~
help: consider importing this function
   |
8  | use riot_sys::inline::mutex_lock;
   |
help: if you import `mutex_lock`, refer to it directly
   |
45 -         unsafe { riot_sys::mutex_lock(crate::inline_cast_mut(self.mutex.get())) };
45 +         unsafe { mutex_lock(crate::inline_cast_mut(self.mutex.get())) };
   |

For more information about this error, try `rustc --explain E0425`.
error: could not compile `riot-wrappers` due to previous error
warning: build failed, waiting for other jobs to finish...
make: *** [/tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/makefiles/cargo-targets.inc.mk:44: /tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/build/target/thumbv7m-none-eabi/release/librust_hello_world.a] Error 101
make: Leaving directory '/tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/examples/rust-hello-world'
cat: /tmp/dwq.0.828799345211067/1d1268c526f23e2d84dbe3f5848970a9/build/test-input-hash.sha1: No such file or directory
{"build/": 263124}

I'll abort the CI run now, so that the other jobs have a chance to get through until tomorrow

[chrysn]

Yes; last time I checked, the functions would have kept their staticness (but maybe I missed it). It's one of these things where the bindings are stricter than C when it comes to API stability. There is a mechanism for "it's OK that this function may be static-inline or not" in the -sys crate, I'll look into updating it.

[chrysn]

I've pushed a fix, and added a branch switch to try out the fix on this branch. If this builds fine, I'll merge https://github.com/RIOT-OS/rust-riot-sys/pull/10.

[maribu]

I threw in an unrelated fix and a commit to limit the build to (one of) the app the failed building due to the API change from rust's point of view

[chrysn]

That test might have been insufficient, for the hello world Rust example doesn't build anything with mutexes (wheres the rust-gcoap does, and I'm seeing yet-unclear build errors on https://github.com/RIOT-OS/rust-riot-sys/pull/10). I'll investigate there, but in case something is afoul with the build system there, could you run this once more all Rust-ish builds (examples/rust-*, tests/rust*) active?

[maribu]

@chrysn: Done :)

[chrysn]

This should now work without the REMOVE ME, once rebased onto master. (Yes, also without rebasing, but in the spirit of bisectability a rebase would be good, and do no harm anyway).

[maribu]

Finally, all green. Technically a second ACK is still needed.

[maribu]

thx :)