humility-core: allow enums with signed variants

Cargo.toml

@@ -17,7 +17,7 @@ name = "humility"
 #
 # Be sure to check in and push all of the files that change.  Happy versioning!
 #
-version = "0.11.1"
+version = "0.11.2"
 authors = ["Bryan Cantrill <[email protected]>"]
 edition = "2018"
 license = "MPL-2.0"

cmd/rpc/src/lib.rs

@@ -415,7 +415,10 @@ impl<'a> RpcClient<'a> {
         let buf = &self.buf[..n];
 
         if buf[0] != 0 {
-            match self.rpc_reply_type.lookup_variant_by_tag(buf[0] as u64) {
+            // TODO: assumes the discriminator is a u8. It's not clear from
+            // context whether this assumption carries through into the udprpc
+            // task.
+            match self.rpc_reply_type.lookup_variant_by_tag(Tag::from(buf[0])) {
                 Some(e) => {
                     let msg = format!("Got error from `udprpc`: {}", e.name);
                     if e.name == "BadImageId" {

cmd/validate/src/lib.rs

@@ -205,7 +205,13 @@ fn validate(context: &mut ExecutionContext) -> Result<()> {
     for (rndx, (ndx, device)) in devices.iter().enumerate() {
         let result = match &results[rndx] {
             Ok(val) => {
-                if let Some(variant) = ok.lookup_variant_by_tag(val[0].into()) {
+                // TODO: assumes discriminant is a u8. Since this is using Hiffy
+                // call results instead of looking at a Rust value in memory,
+                // it's not clear from context what changes would be required to
+                // fix this.
+                if let Some(variant) =
+                    ok.lookup_variant_by_tag(Tag::from(val[0]))
+                {
                     Ok(match variant.name.as_str() {
                         "Present" => "present".yellow(),
                         "Validated" => "validated".green(),
@@ -217,7 +223,11 @@ fn validate(context: &mut ExecutionContext) -> Result<()> {
             }
             Err(e) => {
                 if let idol::IdolError::CLike(err) = op.error {
-                    Ok(match err.lookup_variant_by_tag(*e as u64) {
+                    // TODO: assumes discriminant is a u8. Since this is using
+                    // Hiffy call results instead of looking at a Rust value in
+                    // memory, it's not clear from context what changes would be
+                    // required to fix this.
+                    Ok(match err.lookup_variant_by_tag(Tag::from(*e)) {
                         Some(variant) => match variant.name.as_str() {
                             "NotPresent" => {
                                 if device.removable {

humility-core/src/hubris.rs

@@ -15,6 +15,7 @@ use std::fmt::{self, Write};
 use std::fs::{self, OpenOptions};
 use std::io::Cursor;
 use std::mem::size_of;
+use std::num::TryFromIntError;
 use std::path::Path;
 use std::str::{self, FromStr};
 use std::time::Instant;
@@ -4756,7 +4757,11 @@ impl HubrisObjectLoader {
 
         while let Some(attr) = attrs.next()? {
             if attr.name() == gimli::constants::DW_AT_discr_value {
-                value = attr.value().udata_value();
+                value = attr.value().udata_value().map(Tag::Unsigned);
+
+                if value.is_none() {
+                    value = attr.value().sdata_value().map(Tag::Signed);
+                }
 
                 if value.is_none() {
                     bail!("bad discriminant on union {}", parent);
@@ -4844,10 +4849,19 @@ impl HubrisObjectLoader {
                 }
 
                 gimli::constants::DW_AT_const_value => {
-                    value = attr.value().udata_value();
+                    value = attr.value().udata_value().map(Tag::Unsigned);
 
                     if value.is_none() {
-                        bail!("bad discriminant on const enum {}", parent);
+                        value = attr.value().sdata_value().map(Tag::Signed);
+                    }
+
+                    if value.is_none() {
+                        bail!(
+                            "bad discriminant on const enum {}: {:?} ({:?})",
+                            parent,
+                            attr.value(),
+                            name
+                        );
                     }
                 }
 
@@ -5837,7 +5851,124 @@ pub struct HubrisEnumVariant {
     pub name: String,
     pub offset: usize,
     pub goff: Option<HubrisGoff>,
-    pub tag: Option<u64>,
+    pub tag: Option<Tag>,
+}
+
+/// Type representing an enum variant tag.
+///
+/// Tags may be signed or unsigned. Every variant of a given enum uses the same
+/// signedness. (This should likely be available on the HubrisEnum type but is
+/// not.) As a result, signed and unsigned `Tag`s never compare as equal. This
+/// can pose a problem for manually rolling enum reconstruction code using
+/// `lookup_variant_by_tag`, but `determine_variant` will do the right thing.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub enum Tag {
+    Unsigned(u64),
+    Signed(i64),
+}
+
+/// All u64s can become Tags.
+impl From<u64> for Tag {
+    fn from(u: u64) -> Self {
+        Tag::Unsigned(u)
+    }
+}
+
+/// All u32s can become Tags.
+impl From<u32> for Tag {
+    fn from(u: u32) -> Self {
+        Tag::Unsigned(u64::from(u))
+    }
+}
+
+/// All u16s can become Tags.
+impl From<u16> for Tag {
+    fn from(u: u16) -> Self {
+        Tag::Unsigned(u64::from(u))
+    }
+}
+
+/// All u8s can become Tags.
+impl From<u8> for Tag {
+    fn from(u: u8) -> Self {
+        Tag::Unsigned(u64::from(u))
+    }
+}
+
+/// All i64s can become Tags.
+impl From<i64> for Tag {
+    fn from(i: i64) -> Self {
+        Tag::Signed(i)
+    }
+}
+
+/// All i32s can become Tags.
+impl From<i32> for Tag {
+    fn from(i: i32) -> Self {
+        Tag::Signed(i64::from(i))
+    }
+}
+
+/// All i16s can become Tags.
+impl From<i16> for Tag {
+    fn from(i: i16) -> Self {
+        Tag::Signed(i64::from(i))
+    }
+}
+
+/// All i8s can become Tags.
+impl From<i8> for Tag {
+    fn from(i: i8) -> Self {
+        Tag::Signed(i64::from(i))
+    }
+}
+
+/// Some Tags can become u8s.
+impl TryFrom<Tag> for u8 {
+    type Error = TryFromIntError;
+
+    fn try_from(t: Tag) -> Result<u8, TryFromIntError> {
+        match t {
+            Tag::Unsigned(u) => u.try_into(),
+            Tag::Signed(i) => i.try_into(),
+        }
+    }
+}
+
+/// Some Tags can become u16s.
+impl TryFrom<Tag> for u16 {
+    type Error = TryFromIntError;
+
+    fn try_from(t: Tag) -> Result<u16, TryFromIntError> {
+        match t {
+            Tag::Unsigned(u) => u.try_into(),
+            Tag::Signed(i) => i.try_into(),
+        }
+    }
+}
+
+/// Some Tags can become u32s.
+impl TryFrom<Tag> for u32 {
+    type Error = TryFromIntError;
+
+    fn try_from(t: Tag) -> Result<u32, TryFromIntError> {
+        match t {
+            Tag::Unsigned(u) => u.try_into(),
+            Tag::Signed(i) => i.try_into(),
+        }
+    }
+}
+
+/// Some Tags (and all unsigned Tags) can become u64s.
+impl TryFrom<Tag> for u64 {
+    type Error = TryFromIntError;
+
+    fn try_from(t: Tag) -> Result<u64, TryFromIntError> {
+        match t {
+            Tag::Unsigned(u) => Ok(u),
+            Tag::Signed(i) => i.try_into(),
+        }
+    }
 }
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
@@ -5855,15 +5986,25 @@ pub struct HubrisEnum {
     /// only one variant.
     pub discriminant: Option<HubrisDiscriminant>,
     /// temporary to hold tag of next variant
-    pub tag: Option<u64>,
+    pub tag: Option<Tag>,
     pub variants: Vec<HubrisEnumVariant>,
     namespace: Option<NamespaceId>,
 }
 
 impl HubrisEnum {
+    /// Finds a variant of this enum type that has the discriminant `tag`.
+    ///
+    /// `tag` must have the proper signedness for this enum. Historically a lot
+    /// of code assumed that discriminants were always unsigned, and there are
+    /// still routines around that make this assumption; they will simply fail
+    /// to find variants for enums using signed discriminants.
+    ///
+    /// If you would like to avoid this pitfall, the `determine_variant`
+    /// routine will do the type lookup for you, and should almost always be
+    /// preferred where applicable!
     pub fn lookup_variant_by_tag(
         &self,
-        tag: u64,
+        tag: Tag,
     ) -> Option<&HubrisEnumVariant> {
         // We prioritize picking a variant with the matching tag
         if let Some(t) = self.variants.iter().find(|v| v.tag == Some(tag)) {
@@ -5900,36 +6041,60 @@ impl HubrisEnum {
         bail!("missing variant: {}.{}", self.name, name)
     }
 
+    /// Finds a variant of this enum given a blob of data representing one of
+    /// its variants. This handles looking up the offset of the tag in memory,
+    /// and handling signed/unsigned tags.
+    ///
+    /// It's almost always better to use this than to attempt to roll your own
+    /// using `lookup_variant_by_tag`.
     pub fn determine_variant(
         &self,
         hubris: &HubrisArchive,
         buf: &[u8],
     ) -> Result<&HubrisEnumVariant> {
-        let readval = |b: &[u8], o, sz| -> Result<u64> {
-            Ok(match sz {
-                1 => u64::from(b[o]),
-                2 => u64::from(u16::from_le_bytes(b[o..o + 2].try_into()?)),
-                4 => u64::from(u32::from_le_bytes(b[o..o + 4].try_into()?)),
-                8 => u64::from_le_bytes(b[o..o + 8].try_into()?),
+        let readtag = |b: &[u8], o, sz, enc| -> Result<Tag> {
+            Ok(match (sz, enc) {
+                (1, HubrisEncoding::Unsigned) => Tag::from(b[o]),
+                (2, HubrisEncoding::Unsigned) => {
+                    Tag::from(u16::from_le_bytes(b[o..o + 2].try_into()?))
+                }
+                (4, HubrisEncoding::Unsigned) => {
+                    Tag::from(u32::from_le_bytes(b[o..o + 4].try_into()?))
+                }
+                (8, HubrisEncoding::Unsigned) => {
+                    Tag::from(u64::from_le_bytes(b[o..o + 8].try_into()?))
+                }
+
+                (1, HubrisEncoding::Signed) => Tag::from(b[o] as i8),
+                (2, HubrisEncoding::Signed) => {
+                    Tag::from(i16::from_le_bytes(b[o..o + 2].try_into()?))
+                }
+                (4, HubrisEncoding::Signed) => {
+                    Tag::from(i32::from_le_bytes(b[o..o + 4].try_into()?))
+                }
+                (8, HubrisEncoding::Signed) => {
+                    Tag::from(i64::from_le_bytes(b[o..o + 8].try_into()?))
+                }
+
                 _ => {
                     bail!("bad variant size!");
                 }
             })
         };
 
         if let Some(HubrisDiscriminant::Value(goff, offs)) = self.discriminant {
-            let size = match hubris.basetypes.get(&goff) {
-                Some(v) => v.size,
+            let (encoding, size) = match hubris.basetypes.get(&goff) {
+                Some(v) => (v.encoding, v.size),
                 None => {
                     bail!("enum has discriminant of unknown type: {}", goff);
                 }
             };
 
-            let val = readval(buf, offs, size)?;
+            let val = readtag(buf, offs, size, encoding)?;
 
             match self.lookup_variant_by_tag(val) {
                 None => {
-                    bail!("unknown variant: 0x{:x}", val);
+                    bail!("unknown variant: {:#x?}", val);
                 }
 
                 Some(variant) => Ok(variant),

humility-hiffy/src/lib.rs

@@ -692,7 +692,12 @@ impl<'a> HiffyContext<'a> {
                 //
                 if buf[0] != 0 {
                     let rpc_reply_type = self.rpc_reply_type.unwrap();
-                    match rpc_reply_type.lookup_variant_by_tag(buf[0] as u64) {
+                    // TODO: this assumes that the reply enum can be represented
+                    // by a u8 (buf[0] is a u8) and will not work with larger
+                    // discriminants, or signed discriminants.
+                    match rpc_reply_type
+                        .lookup_variant_by_tag(Tag::from(buf[0]))
+                    {
                         Some(e) => {
                             let image_id = self.hubris.image_id().unwrap();
                             let msg = format!("RPC error: {}", e.name);
@@ -881,7 +886,12 @@ impl<'a> HiffyContext<'a> {
             }
             Err(e) => {
                 let variant = if let idol::IdolError::CLike(error) = op.error {
-                    error.lookup_variant_by_tag(*e as u64)
+                    // TODO potentially sign-extended discriminator represented
+                    // as u32 and then zero-extended to u64; won't work for
+                    // signed values. Can't use determine_variant here because
+                    // it's not laid out in memory, it's been unfolded onto the
+                    // return stack.
+                    error.lookup_variant_by_tag(Tag::from(*e as u64))
                 } else {
                     None
                 };
@@ -1296,7 +1306,13 @@ pub fn hiffy_decode(
         }
         Err(e) => match op.error {
             idol::IdolError::CLike(error) => {
-                if let Some(v) = error.lookup_variant_by_tag(e as u64) {
+                // TODO potentially sign-extended discriminator represented as
+                // u32 and then zero-extended to u64; won't work for signed
+                // values. Can't use determine_variant here because it's not
+                // laid out in memory, it's been unfolded onto the return stack.
+                if let Some(v) =
+                    error.lookup_variant_by_tag(Tag::from(e as u64))
+                {
                     Err(v.name.to_string())
                 } else {
                     Err(format!("<Unknown variant {e}>"))

humility-idol/src/lib.rs

@@ -195,7 +195,11 @@ impl<'a> IdolOperation<'a> {
 
     pub fn strerror(&self, code: u32) -> String {
         let variant = if let IdolError::CLike(error) = self.error {
-            error.lookup_variant_by_tag(code as u64)
+            // TODO: assumes discriminant is a u8. Since this is using Hiffy
+            // call results instead of looking at a Rust value in memory, it's
+            // not clear from context what changes would be required to fix
+            // this.
+            error.lookup_variant_by_tag(Tag::from(code as u64))
         } else {
             None
         };

tests/cmd/chip.trycmd

@@ -13,7 +13,7 @@ For more information try --help
 
 ```
 $ humility --chip this-can-be-anything -V
-humility 0.11.1
+humility 0.11.2
 
 ```
 
@@ -28,7 +28,7 @@ For more information try --help
 
 ```
 $ humility -c apx432 -V
-humility 0.11.1
+humility 0.11.2
 
 ```