Merge #299

299: Update `CommandReturn` in preparation for the Command implementation. r=jrvanwhy a=jrvanwhy

This makes the following change to `CommandReturn`, which are needed by my implementation of the Command system call and its tests:

1. Implements `Debug`. This is not a replacement for `ufmt`-based debugging, but rather to support unit tests (e.g. `assert_eq!`).
2. Changes the register values from `usize` to `u32`, as `RawSyscalls` no longer returns its registers as `usize`s. I discovered that code that interacts with `CommandReturn` is cleaner if we store `u32`s in `CommandReturn` rather than `Register`s.
3. `new` is now `pub` rather than `pub(crate)`, as `libtock_unittest` needs to create `CommandReturn` values.
4. I added `CommandReturn::raw_values`, which will be used by `libtock_unittest` to transform `CommandReturn`s from fake drivers and expected syscalls into register values for `RawSyscalls`.
5. I added safe `CommandReturn` constructors to `libtock_unittest`. `libtock_platform` intentionally does not expose safe constructors, as a process binary should only create a `CommandReturn` by calling the Command system call. However, `libtock_unittest` and unit tests based on it need to create `CommandReturn` values as well, so I added safe constructors.

The combination of 1 and 5 required me to add `Debug` to `ErrorCode` and `ReturnVariant`, so I did that as well.

Co-authored-by: Johnathan Van Why <jrvanwhy@google.com>

platform/src/command_return.rs

@@ -4,33 +4,21 @@ use core::mem::transmute;
 
 /// The response type from `command`. Can represent a successful value or a
 /// failure.
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug)]
 pub struct CommandReturn {
     return_variant: ReturnVariant,
-    // r1, r2, and r3 should only contain 32-bit values. However, these are
-    // converted directly from usizes returned by RawSyscalls::four_arg_syscall.
-    // To avoid casting twice (both when converting to a Command Return and when
-    // calling a get_*() function), we store the usizes directly. Then using the
-    // CommandReturn only involves one conversion for each of r1, r2, and r3,
-    // performed in the get_*() functions.
 
     // Safety invariant on r1: If return_variant is failure variant, r1 must be
     // a valid ErrorCode.
-    r1: usize,
-    r2: usize,
-    r3: usize,
+    r1: u32,
+    r2: u32,
+    r3: u32,
 }
 
 impl CommandReturn {
     /// # Safety
     /// If return_variant is a failure variant, r1 must be a valid ErrorCode.
-    #[cfg(test)] // Will be removed when command() is implemented.
-    pub(crate) unsafe fn new(
-        return_variant: ReturnVariant,
-        r1: usize,
-        r2: usize,
-        r3: usize,
-    ) -> Self {
+    pub unsafe fn new(return_variant: ReturnVariant, r1: u32, r2: u32, r3: u32) -> Self {
         CommandReturn {
             return_variant,
             r1,
@@ -115,7 +103,7 @@ impl CommandReturn {
         if !self.is_failure_u32() {
             return None;
         }
-        Some((unsafe { transmute(self.r1 as u16) }, self.r2 as u32))
+        Some((unsafe { transmute(self.r1 as u16) }, self.r2))
     }
 
     /// Returns the error code and return values if this CommandReturn is of
@@ -124,11 +112,7 @@ impl CommandReturn {
         if !self.is_failure_2_u32() {
             return None;
         }
-        Some((
-            unsafe { transmute(self.r1 as u16) },
-            self.r2 as u32,
-            self.r3 as u32,
-        ))
+        Some((unsafe { transmute(self.r1 as u16) }, self.r2, self.r3))
     }
 
     /// Returns the error code and return value if this CommandReturn is of type
@@ -148,15 +132,15 @@ impl CommandReturn {
         if !self.is_success_u32() {
             return None;
         }
-        Some(self.r1 as u32)
+        Some(self.r1)
     }
 
     /// Returns the values if this CommandReturn is of type Success with 2 u32.
     pub fn get_success_2_u32(&self) -> Option<(u32, u32)> {
         if !self.is_success_2_u32() {
             return None;
         }
-        Some((self.r1 as u32, self.r2 as u32))
+        Some((self.r1, self.r2))
     }
 
     /// Returns the value if this CommandReturn is of type Success with u64.
@@ -172,7 +156,7 @@ impl CommandReturn {
         if !self.is_success_3_u32() {
             return None;
         }
-        Some((self.r1 as u32, self.r2 as u32, self.r3 as u32))
+        Some((self.r1, self.r2, self.r3))
     }
 
     /// Returns the values if this CommandReturn is of type Success with u32 and
@@ -181,7 +165,12 @@ impl CommandReturn {
         if !self.is_success_u32_u64() {
             return None;
         }
-        Some((self.r1 as u32, self.r2 as u64 + ((self.r3 as u64) << 32)))
+        Some((self.r1, self.r2 as u64 + ((self.r3 as u64) << 32)))
+    }
+
+    /// Returns the register values used to create this command.
+    pub fn raw_values(&self) -> (ReturnVariant, u32, u32, u32) {
+        (self.return_variant, self.r1, self.r2, self.r3)
     }
 
     /// Returns the return variant of this command.

platform/src/command_return_tests.rs

@@ -5,7 +5,7 @@ fn failure() {
     let command_return = unsafe {
         CommandReturn::new(
             return_variant::FAILURE,
-            ErrorCode::Reserve as usize,
+            ErrorCode::Reserve as u32,
             1002,
             1003,
         )
@@ -29,6 +29,10 @@ fn failure() {
     assert_eq!(command_return.get_success_u64(), None);
     assert_eq!(command_return.get_success_3_u32(), None);
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::FAILURE, 5, 1002, 1003)
+    );
     assert_eq!(command_return.return_variant(), return_variant::FAILURE);
 }
 
@@ -37,7 +41,7 @@ fn failure_u32() {
     let command_return = unsafe {
         CommandReturn::new(
             return_variant::FAILURE_U32,
-            ErrorCode::Off as usize,
+            ErrorCode::Off as u32,
             1002,
             1003,
         )
@@ -64,6 +68,10 @@ fn failure_u32() {
     assert_eq!(command_return.get_success_u64(), None);
     assert_eq!(command_return.get_success_3_u32(), None);
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::FAILURE_U32, 4, 1002, 1003)
+    );
     assert_eq!(command_return.return_variant(), return_variant::FAILURE_U32);
 }
 
@@ -72,7 +80,7 @@ fn failure_2_u32() {
     let command_return = unsafe {
         CommandReturn::new(
             return_variant::FAILURE_2_U32,
-            ErrorCode::Already as usize,
+            ErrorCode::Already as u32,
             1002,
             1003,
         )
@@ -99,6 +107,10 @@ fn failure_2_u32() {
     assert_eq!(command_return.get_success_u64(), None);
     assert_eq!(command_return.get_success_3_u32(), None);
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::FAILURE_2_U32, 3, 1002, 1003)
+    );
     assert_eq!(
         command_return.return_variant(),
         return_variant::FAILURE_2_U32
@@ -110,7 +122,7 @@ fn failure_u64() {
     let command_return = unsafe {
         CommandReturn::new(
             return_variant::FAILURE_U64,
-            ErrorCode::Busy as usize,
+            ErrorCode::Busy as u32,
             0x1002,
             0x1003,
         )
@@ -137,6 +149,10 @@ fn failure_u64() {
     assert_eq!(command_return.get_success_u64(), None);
     assert_eq!(command_return.get_success_3_u32(), None);
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::FAILURE_U64, 2, 0x1002, 0x1003)
+    );
     assert_eq!(command_return.return_variant(), return_variant::FAILURE_U64);
 }
 
@@ -162,6 +178,10 @@ fn success() {
     assert_eq!(command_return.get_success_u64(), None);
     assert_eq!(command_return.get_success_3_u32(), None);
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::SUCCESS, 1001, 1002, 1003)
+    );
     assert_eq!(command_return.return_variant(), return_variant::SUCCESS);
 }
 
@@ -188,6 +208,10 @@ fn success_u32() {
     assert_eq!(command_return.get_success_u64(), None);
     assert_eq!(command_return.get_success_3_u32(), None);
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::SUCCESS_U32, 1001, 1002, 1003)
+    );
     assert_eq!(command_return.return_variant(), return_variant::SUCCESS_U32);
 }
 
@@ -214,6 +238,10 @@ fn success_2_u32() {
     assert_eq!(command_return.get_success_u64(), None);
     assert_eq!(command_return.get_success_3_u32(), None);
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::SUCCESS_2_U32, 1001, 1002, 1003)
+    );
     assert_eq!(
         command_return.return_variant(),
         return_variant::SUCCESS_2_U32
@@ -246,6 +274,10 @@ fn success_u64() {
     );
     assert_eq!(command_return.get_success_3_u32(), None);
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::SUCCESS_U64, 0x1001, 0x1002, 1003)
+    );
     assert_eq!(command_return.return_variant(), return_variant::SUCCESS_U64);
 }
 
@@ -272,6 +304,10 @@ fn success_3_u32() {
     assert_eq!(command_return.get_success_u64(), None);
     assert_eq!(command_return.get_success_3_u32(), Some((1001, 1002, 1003)));
     assert_eq!(command_return.get_success_u32_u64(), None);
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::SUCCESS_3_U32, 1001, 1002, 1003)
+    );
     assert_eq!(
         command_return.return_variant(),
         return_variant::SUCCESS_3_U32
@@ -304,6 +340,10 @@ fn success_u32_u64() {
         command_return.get_success_u32_u64(),
         Some((1001, 0x0000_1003_0000_1002))
     );
+    assert_eq!(
+        command_return.raw_values(),
+        (return_variant::SUCCESS_U32_U64, 1001, 0x1002, 0x1003)
+    );
     assert_eq!(
         command_return.return_variant(),
         return_variant::SUCCESS_U32_U64

platform/src/error_code.rs

@@ -1,10 +1,6 @@
 /// An error code returned by the kernel.
-// TODO: derive(Debug) is currently only enabled for test builds, which is
-// necessary so it can be used in assert_eq!. We should develop a lighter-weight
-// Debug implementation and see if it is small enough to enable on non-Debug
-// builds.
-#[cfg_attr(test, derive(Debug))]
-#[derive(Clone, Copy, PartialEq, Eq)]
+// TODO: Add a ufmt debug implementation for process binaries to use.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 #[repr(u16)]  // To facilitate use with transmute() in CommandReturn
 #[rustfmt::skip]
 pub enum ErrorCode {

platform/src/return_variant.rs

@@ -1,11 +1,7 @@
 /// `ReturnVariant` describes what value type the kernel has returned.
 // ReturnVariant is not an enum so that it can be converted from a u32 for free.
-// TODO: derive(Debug) is currently only enabled for test builds, which is
-// necessary so it can be used in assert_eq!. We should develop a lighter-weight
-// Debug implementation and see if it is small enough to enable on non-Debug
-// builds.
-#[cfg_attr(test, derive(Debug))]
-#[derive(Clone, Copy, PartialEq, Eq)]
+// TODO: Add a ufmt debug implementation for use by process binaries.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct ReturnVariant(u32);
 
 impl From<u32> for ReturnVariant {