Initial code review #1

wsx-antithesis · 2024-04-23T14:53:21Z

In progress branch to track some suggested changes.
I'll put some comments alongside my changes as CR comments.

wsx-antithesis · 2024-04-23T14:56:11Z

lib/src/internal/voidstar_handler.rs

+        let payload = value.to_string();
        unsafe {
-            let json_data_func: Symbol<unsafe extern fn(s: *const c_char)> = self.voidstar_lib.get(b"fuzz_json_data").unwrap();
-            let payload = CString::new(value.to_string())?;
-            json_data_func(payload.as_ptr());
+            let json_data_func: Symbol<unsafe extern fn(s: *const c_char, l: size_t)> = self.voidstar_lib.get(b"fuzz_json_data").unwrap();
+            json_data_func(payload.as_bytes().as_ptr() as *const c_char, payload.len());
        }


fuzz_json_data in instrumentation.h accepts a data pointer and a length (in bytes), instead of a null-terminated C-style string. Update this call to pass in the correct parameters.

wsx-antithesis · 2024-04-23T14:58:48Z

lib/src/assert/mod.rs

+        let mut tracker = ASSERT_TRACKER.lock().unwrap();
+        let info = tracker.entry(self.id.clone()).or_default();
+        // Record the condition in the associated TrackingInfo entry,
+        // and emit the assertion when first seeing a condition
+        let emitting = if self.condition {
+            info.pass_count += 1;
+            info.pass_count == 1
+        } else {
+            info.fail_count += 1;
+            info.fail_count == 1
        };
-
+        drop(tracker); // release the lock asap
+        if emitting {
+            self.emit();
+        }


Minimize the critical section in which we hold the lock to ASSERT_TRACKER.

wsx-antithesis · 2024-04-23T14:59:56Z

lib/src/assert/mod.rs

-                self.emit()
-            }
+        let mut tracker = ASSERT_TRACKER.lock().unwrap();
+        let info = tracker.entry(self.id.clone()).or_default();


Uses the entry API to simplify constructing a default TrackingInfo for a vacant tracking key.

With the access to the ASSERT_TRACKER contained (is it?) entirely here, consider relocating ASSERT_TRACKER to this function.

wsx-antithesis · 2024-04-23T15:01:45Z

lib/src/assert/mod.rs

@@ -260,7 +243,7 @@ pub fn assert_impl(


    let assertion = AssertionInfo::new(assert_type, display_type, condition, message, class, function, file, begin_line, begin_column, hit, must_hit, id, details);
-    let _ = &assertion.track_entry(ASSERT_TRACKER.lock().as_deref_mut().unwrap());
+    let _ = &assertion.track_entry();


Let track_entry obtain the lock to ASSERT_TRACKER internally (see above) instead of having the caller locking it and passing in as a parameter. This helps with minimizing the critical section.

No need to ignore the return value from .track_entry() - there is no longer a return value defined

wsx-antithesis · 2024-04-23T19:11:28Z

lib/src/internal/mod.rs

-static PROTOCOL_VERSION: &str = "1.0.0";
+const PROTOCOL_VERSION: &str = "1.0.0";

 // Tracks SDK releases
-static SDK_VERSION: &str = "0.1.1";
+const SDK_VERSION: &str = "0.1.1";


const is preferred over static in these cases, as suggested in https://doc.rust-lang.org/reference/items/static-items.html#using-statics-or-consts.

wsx-antithesis · 2024-04-23T19:19:05Z

lib/Cargo.toml

@@ -9,7 +9,7 @@ libloading = "0.8.3"
 libc = "0.2.153"
 rand = "0.8.5"
 rustc_version_runtime = "0.3.0"
-lazy_static = "1.4.0"
+once_cell = "1"


lazy_static is effectively unmaintained and would be deprecated (rust-lang-nursery/lazy-static.rs#214). once_cell is the recommended migration path.
If we decide to only support rust 1.70+ (which is higher than our current internal toolchain version 1.69), we can implement the functionality from once_cell using OnceLock and cutoff the once_cell dependencies.
A more direct std substitute for once_cell::sync::Lazy is tracked in the unstable lazy_cell feature.

wsx-antithesis · 2024-04-23T19:24:24Z

lib/src/internal/local_handler.rs

@@ -27,7 +26,7 @@ impl LocalHandler {
            // Seems like LocalHandler gets bound to a reference with 
            // a 'static lifetime.
            LocalHandler{
-                maybe_writer: Some(BufWriter::with_capacity(0, Box::new(f)))
+                maybe_writer: Some(f)


Not sure about the point of using a zero-capacity BufWriter. Since BufWriter is a wrapper over some underlying writer W, and writing to BufWriter only potentially writes (but not flush) to W, writing to a zero-capacity BufWriter should be equivalent to directly writing to W.

wsx-antithesis · 2024-04-23T19:27:05Z

lib/src/internal/local_handler.rs

 pub struct LocalHandler {
-    maybe_writer: Option<BufWriter<Box<dyn Write + Send>>>
+    maybe_writer: Option<File>


I hard coded the inner writer to File for now as it avoids a dynamic dispatch redirection introduced by Box<dyn Write>. Might have to revisit when we ever have to support dynamically switching the write sink for some reason?

wsx-antithesis · 2024-04-23T19:34:24Z

lib/src/internal/mod.rs

-// static mut LIB_HANDLER: Option<Box<dyn LibHandler>> = None;
-static LIB_HANDLER: Mutex<Option<Box<dyn LibHandler + Send>>> = Mutex::new(None);
+static LIB_HANDLER: Lazy<Box<dyn LibHandler + Sync + Send>> = Lazy::new(|| {
+    match VoidstarHandler::try_load() {
+        Ok(handler) => Box::new(handler),
+        Err(_) => Box::new(LocalHandler::new()),
+    }
+});


Switch to using a Lazy cell for the global handler. Now the initialization logic for LIB_HANDLER would be called when it's content is access for the first time. So we can also remove manual calls to instantiate_handler() further down.
Lazy should have lower overhead compared to a Mutex, and we can remove the Mutex to guard against concurrent calls on LIB_HANDLER as we changed how LibHandler is implemented below.

wsx-antithesis · 2024-04-23T19:40:24Z

lib/src/internal/mod.rs

 trait LibHandler {
-    fn output(&mut self, value: &Value) -> Result<(), Error>;
+    fn output(&self, value: &Value) -> Result<(), Error>;
    fn random(&self) -> u64;
 }


Ensure all LibHandler methods only takes &self. This has the benefit that all calls to those methods on LIB_HANDLER would be thread-safe without an extra Mutex.
The original user of the &mut self parameter is the implementation of LocalHandler::output, but we can avoid that since now LocalHandler stores an inner File, and &File implements Write already.

wsx-antithesis · 2024-04-23T19:45:31Z

lib/src/internal/voidstar_handler.rs

-    pub fn new() -> Self {
-        load_voidstar();
-        let lib = LIB_VOIDSTAR.lock().unwrap().as_ref().unwrap().clone();
-        VoidstarHandler{
-            voidstar_lib: lib,
-        }
+    pub fn try_load() -> Result<Self, libloading::Error> {
+        let lib = unsafe { Library::new(LIB_NAME) }?;
+        Ok(VoidstarHandler { voidstar_lib: lib })


We should be able to avoid maintaining a local static for initializing the voidstar library here if the initialization logic at internal/mod.rs is enough?

wsx-antithesis · 2024-04-23T20:48:22Z

lib/src/internal/voidstar_handler.rs

-    voidstar_lib: Library,
+    // Not used directly but exists to ensure the library is loaded
+    // and all the following function pointers points to valid memory.
+    _lib: Library,
+    // SAFETY: The memory pointed by `s` must be valid up to `l` bytes.
+    fuzz_json_data: unsafe fn(s: *const c_char, l: size_t),
+    fuzz_get_random: fn() -> u64,


Switch to storing the function pointers of the symbols inline in the struct, so we can avoid repeated get for symbols in the output and random calls. See the SAFETY comments for all the unsafe blocks.

Really should verify/enforce that calls to fuzz_json_data() and fuzz_get_random() are thread-safe.

To enforce safe use, mark fuzz_get_random() as unsafe (and handle this constraint in callers), and use a mutex when calling either one of these ffi symbols. Check if there are any guarantees in libvoidstar() for thread safe operation.

wsx-antithesis · 2024-04-23T20:49:55Z

lib/src/internal/voidstar_handler.rs

-#[derive(Debug)]
 pub struct VoidstarHandler {


Is the Debug implementation used anywhere?

wsx-antithesis · 2024-04-23T20:53:08Z

lib/src/random.rs

-pub fn random_choice<T: Copy>(slice: &[T]) -> Option<T> {
-    let lx: usize = slice.len();
-    match lx {
-        0 => None,
-        1 => Some(slice[0]),
+pub fn random_choice<T>(slice: &[T]) -> Option<&T> {
+    match slice {
+        [] => None,
+        [x] => Some(x),
        _ => {
-            let idx: usize = (get_random() as usize) % lx;
-            Some(slice[idx])
+            let idx: usize = (get_random() as usize) % slice.len();
+            Some(&slice[idx])
        }
    }
 }


Requiring T: Copy is a bit too restrictive, returns a borrow to an item in the slice instead (similar to how the first method on slice is declared: https://doc.rust-lang.org/std/primitive.slice.html#method.first). Also switch to using slice pattern in the match.

wsx-antithesis · 2024-04-23T21:18:43Z

Some general comments/notes:

I haven't look too much into assertion macros, since it is still WIP. Seems like we are in the progress of using linkme?
Do we need to expose something like antithesis_init (reporting back the assertion catalog etc) and the user should call it as early as possible in their main?
Do we want a compile time flag to switch most of the APIs into true no-ops?

herzogp · 2024-04-25T15:55:03Z

lib/src/assert/mod.rs

+            info.pass_count == 1
+        } else {
+            info.fail_count += 1;
+            info.fail_count == 1
        };


Perhaps comment out the setting of emitting here as whether or not, pass_count/fail_count is exactly '1' at this point, and emit is required.

herzogp · 2024-04-25T16:11:27Z

lib/src/internal/local_handler.rs

-                let mut text_line = value.to_string();
-                text_line.push('\n');
-                b2w.write_all(text_line.as_bytes())?;
+                let mut b2w = b2w;


Consider using a separate name for the mutable b2w, rather than shadowing the Some(b2w)

wsx-antithesis added 2 commits April 22, 2024 16:18

Fix the binding for fuzz_json_data.

ed5f36a

Simplify assertion entry tracking.

7219dcc

wsx-antithesis commented Apr 23, 2024

View reviewed changes

wsx-antithesis added 2 commits April 23, 2024 14:54

Prefer const over static for small immutable strings.

191f94f

Switch to once_cell and reduce indirections.

1f56606

This comment was marked as duplicate.

Sign in to view

wsx-antithesis commented Apr 23, 2024

View reviewed changes

wsx-antithesis added 2 commits April 23, 2024 16:40

Cache symbols at library load time.

f20e220

Make random_choice more general.

f36e994

wsx-antithesis commented Apr 23, 2024

View reviewed changes

herzogp reviewed Apr 25, 2024

View reviewed changes

herzogp approved these changes Apr 25, 2024

View reviewed changes

herzogp marked this pull request as ready for review April 25, 2024 16:54

herzogp merged commit 6217f9d into main Apr 25, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial code review #1

Initial code review #1

wsx-antithesis commented Apr 23, 2024 •

edited

Loading

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024

herzogp Apr 25, 2024

wsx-antithesis Apr 23, 2024

herzogp Apr 25, 2024

This comment was marked as duplicate.

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024 •

edited

Loading

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024

herzogp Apr 25, 2024

herzogp Apr 25, 2024

wsx-antithesis Apr 23, 2024

wsx-antithesis Apr 23, 2024 •

edited

Loading

wsx-antithesis commented Apr 23, 2024

herzogp Apr 25, 2024

herzogp Apr 25, 2024

Initial code review #1

Initial code review #1

Conversation

wsx-antithesis commented Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsx-antithesis Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsx-antithesis Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

wsx-antithesis commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsx-antithesis commented Apr 23, 2024 •

edited

Loading

wsx-antithesis Apr 23, 2024 •

edited

Loading

wsx-antithesis Apr 23, 2024 •

edited

Loading