Enum support and ASAN/MSAN C roundtrip tests by fredszaq · Pull Request #58 · sonos/ffi-convert-rs

fredszaq · 2026-04-11T17:11:23Z

This adds support for unit enums. Fixes #57

I also expanded the roundtrip test we have; this will now generate a C header and compile C \o/

fredszaq · 2026-04-11T17:25:08Z

@anthonyray @Deluvi 👋 care for a review ?

Deluvi

The unit enum support looks great ! 👍

The rest is a bit of nitpicks and such for the ASan / MSan tests, but it's good to have that covered ! (though through a very parkoury interpretation of a Rust test)

Feel free to dig into it. If that's irrelevant, ship it

I don't know how much is relevant here, but maybe mind that cross-language ASan and MSan might require extra caution because there might two conflicting ASan/MSan Clang runtimes, see: https://doc.rust-lang.org/beta/unstable-book/compiler-flags/sanitizer.html#working-with-other-languages

Deluvi · 2026-04-11T19:15:00Z

+        printf("Triggering ASan canary (should crash):\n");
+        test_asan_canary();
+        printf("ERROR: ASan did not catch use-after-free!\n");
+        return 1;


When you run the ASan canary, you expect the binary to return a non-zero status:

assert!( !canary.status.success(), "Sanitizer canary ({}) should have crashed but didn't", canary_flag );

That's because ASan will make exit the program with a non-zero status.

By doing return 1; here, you won't actually signal to the test program that ASan did not catch the use-after-free.

I think you should be doing a return 0; here

#58 (comment)

Did that ! I also made sure the test won't actually segfault when doing the violation so we can actually get there if asan/msan is not enabled

Deluvi · 2026-04-11T19:15:19Z

+        printf("Triggering MSan canary (should crash):\n");
+        test_msan_canary();
+        printf("ERROR: MSan did not catch uninitialized read!\n");
+        return 1;


Same here, I think you should return 0; here

#58 (comment)

Deluvi · 2026-04-11T19:19:14Z

+
+    /// Helper: set the env vars that the cc crate expects outside of build.rs.
+    fn setup_cc_env(host_target: &str) {
+        std::env::set_var("TARGET", host_target);


Note: std::env::set_var is not thread-safe:

This function is safe to call in a single-threaded program.
...
In multi-threaded programs on other operating systems, the only safe option is to not use set_var or remove_var at all.

cargo test will make several tests run in parallel in the same process but in different threads

Thankfully, for now, your tests are mutually exclusives because of #[cfg(feature = "msan")], but that's something to note

I got rid of that, there are actually proper functions to configure CC

Deluvi · 2026-04-11T19:20:24Z

+        compile_c_test(
+            tmp_dir.path(),
+            &target_dir,
+            "-fsanitize=address",


We build the C binary with ASan, but we don't build the Rust cdylib with ASan

Is there a reason why not?

oversight, fixed, we now have the normal C round trip test that won't do asan/msan and will run on stable rust. We also have 2 nightly runs of the tests, enabling asan/msan on the rust code (at two levels, once for the full rust tests, and once when building the test cdylib

Deluvi · 2026-04-11T19:37:44Z

+        let tmp_dir = tempfile::tempdir().expect("Failed to create temp dir");
+
+        // Are we calling cargo from inside the test binary to make sure the cdylib is built ?
+        // yes, watch me.


Yeah that feels wrong, that's probably something that should be done outside of a cargo test, especially because tinkering with the build flags will probably render the cache of the cargo test invalid when running cargo build with such things as -Zsanitizer=memory, making the iteration loop a bit cumbersome

But hey, at least the CI and the developer will run them with a simple cargo test, so I guess it's not that so wrong 😂 (until it becomes annoying for the developer I guess)

I made sure to change the cargo target dir when we do that now. This is actually quite fast once you have built it once (and first build is ok) this will prevent having weird things with the cargo lock on the build dir, this did work previously but was not really solid

Deluvi · 2026-04-11T19:40:00Z

+        let stderr = String::from_utf8_lossy(&canary.stderr);
+        assert!(
+            stderr.contains(expected_stderr),
+            "Canary ({}) crashed but not from expected sanitizer: {}",


Ah right, you check for AddressSanitizer / MemorySanitizer in the logs. I don't know how much checking for such log is reliable and will not result in false positives

Maybe checking for return 0; is less finicky ?

configured asan/msan to return 42 on violation and checking that, should be more robust

…e test program

anthonyray · 2026-04-13T07:46:39Z

    pub flattened_range: Range<i64>,
    pub field_with_specific_rust_name: String,
    pub pancake_data: Option<Vec<u8>>,
+    pub extra_ice_cream_flavor: Flavor,


it's getting yummier and yummier

fredszaq added 5 commits April 11, 2026 18:44

add suppport for unit enums

b092dc8

add round trip test compiling C

149ee32

Changelog

bad217c

fmt

f580149

clippy

6c0a148

fredszaq mentioned this pull request Apr 11, 2026

feat: enum support #57

Closed

fredszaq added 5 commits April 11, 2026 19:50

add asan to the c test

4a612b4

memory sanitizer on roundrip tests

18ee45d

changelog

9bf0052

fmt

6b793f7

don't try to run doctests with msan

03ed9ae

fredszaq changed the title ~~Enum support~~ Enum support and MSAN C roundtrip tests Apr 11, 2026

Deluvi approved these changes Apr 11, 2026

View reviewed changes

fredszaq added 5 commits April 12, 2026 16:22

full asan test + refacto of c roundtrips

fb3b09c

do not set env vars in the test process to configure cc

e659ef8

better detection of asan/msan, make sure the violation don't crash th…

3d30db4

…e test program

fmt

a6d4596

fix standard roundrip test when running with asan flags

256d6cb

fredszaq changed the title ~~Enum support and MSAN C roundtrip tests~~ Enum support and ASAN/MSAN C roundtrip tests Apr 12, 2026

fredszaq added 2 commits April 12, 2026 18:44

fmt

743e7c9

typo

56d1a78

anthonyray reviewed Apr 13, 2026

View reviewed changes

fredszaq merged commit 4f3ae6b into main Apr 13, 2026
1 check passed

fredszaq deleted the enum-support branch April 13, 2026 17:48

Conversation

fredszaq commented Apr 11, 2026

Uh oh!

fredszaq commented Apr 11, 2026

Uh oh!

Deluvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredszaq Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fredszaq Apr 12, 2026 •

edited

Loading