oxidecomputer / humility

Debugger for Hubris
Mozilla Public License 2.0
526 stars 50 forks source link

Humility assumes enum discriminants are u64, can't parse recent output #468

Closed cbiffle closed 5 months ago

cbiffle commented 5 months ago

The HubrisEnumVariant struct assumes that any enum discriminant can fit in a u64. This isn't a valid assumption, and in particular it isn't valid for some common Rust types like Ordering:

#[repr(i8)]
pub enum Ordering {
    /// An ordering where a compared value is less than another.
    Less = -1,
    /// An ordering where a compared value is equal to another.
    Equal = 0,
    /// An ordering where a compared value is greater than another.
    Greater = 1,
}

Enum variants can also be u128s or i128s. There is no single primitive type that is capable of storing all possible enum discriminators. The tag field probably needs to become some enum that supports different representations.

This has existed for a while, but I've noticed that more recent versions of rustc have started including such things in the DWARF, and Humility is thus refusing to load archives generated by rustc 1.75.0. This is currently blocking upgrading the toolchain version in Hubris to get some other fixes.

cbiffle commented 5 months ago

Took an initial crack at this, it's slightly harder than I described.

The assumption that enum values are unsigned is embedded in quite a few places in Humility. Generally they look at the type record and use only its size field to decide how to load it. That'll need to be fixed.

(I attempted to hack around this with a clever equality comparison function, but it doesn't work, because we'd need to know the original size of the value to predict how a signed value, incorrectly read as unsigned and zero extended, would be represented. Generally the places where we compare tags for equality, the size and type are lost, all information distilled down to just a u64.)

cbiffle commented 5 months ago

Alright, I've got a compromise PR up in #469.

HubrisEnum embeds the unsigned-discriminant assumption by not actually recording the encoding of the discriminant. The determine_variant routine looks it up separately (on every use) and, as of this PR, does the right thing with signed variants.

All other codepaths that directly call lookup_variant_by_tag are responsible for determining the enum encoding themselves. These tend to be Hiffy code, and because it's not clear to me how/when things will get sign or zero extended on the way up through Hiffy, I have not attempted to fix them. So, these routines will continue to work correctly with unsigned discriminants, and will fail to handle signed.

I feel like this is better than what we had before.

cbiffle commented 5 months ago

Okay, I've confirmed that the compromise PR fixes Humility archive handling for compiler versions up through and including

rustc 1.79.0-nightly (805813650 2024-03-31)