rework colors and display code

CHANGELOG

@@ -1,8 +1,80 @@
-## 0.3.0
+## TODO
 
-TODO: Reader::next_n should return the number of items read as Err(ReadError::Incomplete(n)) if the buffer is exhausted
+~~TODO: Reader::next_n should return the number of items read as Err(ReadError::Incomplete(n)) if the buffer is exhausted~~
+* a reader's `.offset()` should reflect the amount of items that were consumed, if any. if a reader can quickly determine
+  there is not enough input, should it return Incomplete(0) or ExhaustedInput? Incomplete(0) vs ExhaustedInput may still
+  imply that some state was changed (an access mode, for example). this needs more thought.
 TODO: Reader::offset should return an AddressDiff<Address>, not a bare Address
+* quick look seems reasonable enough, should be changed in concert with
+  yaxpeax-core though and that's more than i'm signing up for today
 TODO: impls of `fn one` and `fn zero` so downstream users don't have to import num_traits directly
+* seems nice at first but this means that there are conflicting functions when Zero or One are in scope
+  ... assuming that the idea at the time was to add `fn one` and `fn zero` to `AddressBase`.
+TODO: 0.4.0 or later:
+  * remove `mod colors`, crossterm dependency, related feature flags
+
+## 0.3.0
+
+added a new crate feature flag, `alloc`.
+  this flag is for any features that do not require std, but do require
+  containers from `liballoc`. good examples are `alloc::string::String` or
+  `alloc::vec::Vec`.
+
+added `yaxpeax_arch::display::DisplaySink` after revisiting output colorization.
+  `DisplaySink` is better suited for general markup, rather than being focused
+  specifically on ANSI/console text coloring. `YaxColors` also simply does not
+  style text in some unfortunate circumstances, such as when the console that
+  needs to be styled is only written to after intermediate buffering.
+
+  `DisplaySink` also includes specializable functions for writing text to an
+  output, and the implementation for `alloc::string::String` takes advantage of
+  this: writing through `impl DisplaySink for String` will often be substantially
+  more performant than writing through `fmt::Write`.
+
+added `mod color_new`:
+  this includes an alternate vision for `YaxColors` and better fits with the
+  new `DisplaySink` machinery; ANSI-style text markup can be done through the
+  new `yaxpeax_arch::color_new::ansi::AnsiDisplaySink`.
+
+  this provides more flexibility than i'd initially expected! yours truly will
+  be using this to render instructions with HTML spans (rather than ANSI
+  sequences) to colorize dis.yaxpeax.net.
+
+  in the future, `mod colored` will be removed, `mod color_new` will be renamed
+  to `mod color`.
+
+deprecated `mod colored`:
+  generally, colorization of text is a presentation issue; `trait Colorize`
+  mixed formatting of data to text with how that text is presented, but that is
+  at odds with the same text being presented in different ways for which
+  colorization is not generic. for example, rendering an instruction as marked
+  up HTML involves coloring in an entirely different way than rendering an
+  instruction with ANSI sequences for a VT100-like terminal.
+
+added `yaxpeax_arch::safer_unchecked` to aid in testing use of unchecked methods
+  these were originally added to improve yaxpeax-x86 testing:
+  https://github.com/iximeow/yaxpeax-x86/pull/17, but are being pulled into
+  yaxpeax-arch as they're generally applicable and overall wonderful tools.
+  thank you again 522!
+
+added `mod testkit`:
+  this module contains tools to validate the correctness of crates implementing
+  `yaxpeax-arch` traits. these initial tools are focused on validating the
+  correctness of functions that write to `DisplaySink`, especially that span
+  management is correct.
+
+  `yaxpeax-x86`, for example, will imminently have fuzz targets to use these
+  types for its own validation.
+
+made VecSink's `records` private. instead of extracting records from the struct
+  by accessing this field directly, call `VecSink::into_inner()`.
+
+made VecSink is now available through the `alloc` feature flag as well as `std`.
+
+meta: the major omission in this release is an architecture-agnostic way to
+format an instruction into a `DisplaySink`. i haven't been able to figure out
+quite the right shape for that! it is fully expected in the future, and will
+probably end up somehow referenced through `yaxpeax_arch::Arch`.
 
 ## 0.2.8
 

Cargo.toml

@@ -23,14 +23,24 @@ thiserror = "1.0.26"
 lto = true
 
 [features]
-default = ["std", "use-serde", "colors", "address-parse"]
+default = ["std", "alloc", "use-serde", "color-new", "address-parse"]
 
-std = []
+std = ["alloc"]
+
+alloc = []
 
 # enables the (optional) use of Serde for bounds on
 # Arch and Arch::Address
 use-serde = ["serde", "serde_derive"]
 
+# feature flag for the existing but misfeature'd initial support for output
+# coloring.  the module this gates will be removed in 0.4.0, which includes
+# removing `trait Colorize`, and requires a major version bump for any
+# dependency that moves forward.
 colors = ["crossterm"]
 
+# feature flag for revised output colorizing support, which will replace the
+# existing `colors` feature in 0.4.0.
+color-new = []
+
 address-parse = []

Makefile

@@ -0,0 +1,14 @@
+test: test-std test-no-std test-serde-no-std test-colors-no-std test-color-new-no-std test-alloc-no-std
+
+test-std:
+	cargo test
+test-no-std:
+	cargo test --no-default-features
+test-serde-no-std:
+	cargo test --no-default-features --features "serde"
+test-colors-no-std:
+	cargo test --no-default-features --features "colors"
+test-color-new-no-std:
+	cargo test --no-default-features --features "color-new"
+test-alloc-no-std:
+	cargo test --no-default-features --features "alloc"

fuzz/.gitignore

@@ -0,0 +1,3 @@
+target
+corpus
+artifacts

fuzz/Cargo.toml

@@ -0,0 +1,25 @@
+[package]
+name = "yaxpeax-arch-fuzz"
+version = "0.0.0"
+authors = ["Automatically generated"]
+publish = false
+edition = "2018"
+
+[package.metadata]
+cargo-fuzz = true
+
+[dependencies]
+libfuzzer-sys = "0.4"
+
+[dependencies.yaxpeax-arch]
+path = ".."
+
+# Prevent this from interfering with workspaces
+[workspace]
+members = ["."]
+
+[[bin]]
+name = "write_helpers_are_correct"
+path = "fuzz_targets/write_helpers_are_correct.rs"
+test = false
+doc = false

fuzz/fuzz_targets/write_helpers_are_correct.rs

@@ -0,0 +1,96 @@
+#![no_main]
+use libfuzzer_sys::fuzz_target;
+use yaxpeax_arch::display::DisplaySink;
+
+use std::convert::TryInto;
+
+fuzz_target!(|data: &[u8]| {
+    let mut buf = String::new();
+    match data.len() {
+        1 => {
+            let i = data[0];
+
+            buf.clear();
+            buf.write_u8(i).expect("write succeeds");
+            assert_eq!(buf, format!("{:x}", i));
+
+            buf.clear();
+            buf.write_prefixed_u8(i).expect("write succeeds");
+            assert_eq!(buf, format!("0x{:x}", i));
+
+            let expected = if (i as i8) < 0 {
+                format!("-0x{:x}", (i as i8).unsigned_abs())
+            } else {
+                format!("0x{:x}", i)
+            };
+
+            buf.clear();
+            buf.write_prefixed_i8(i as i8).expect("write succeeds");
+            assert_eq!(buf, expected);
+        },
+        2 => {
+            let i: u16 = u16::from_le_bytes(data.try_into().expect("checked the size is right"));
+
+            buf.clear();
+            buf.write_u16(i).expect("write succeeds");
+            assert_eq!(buf, format!("{:x}", i));
+
+            buf.clear();
+            buf.write_prefixed_u16(i).expect("write succeeds");
+            assert_eq!(buf, format!("0x{:x}", i));
+
+            let expected = if (i as i16) < 0 {
+                format!("-0x{:x}", (i as i16).unsigned_abs())
+            } else {
+                format!("0x{:x}", i)
+            };
+
+            buf.clear();
+            buf.write_prefixed_i16(i as i16).expect("write succeeds");
+            assert_eq!(buf, expected);
+        }
+        4 => {
+            let i: u32 = u32::from_le_bytes(data.try_into().expect("checked the size is right"));
+
+            buf.clear();
+            buf.write_u32(i).expect("write succeeds");
+            assert_eq!(buf, format!("{:x}", i));
+
+            buf.clear();
+            buf.write_prefixed_u32(i).expect("write succeeds");
+            assert_eq!(buf, format!("0x{:x}", i));
+
+            let expected = if (i as i32) < 0 {
+                format!("-0x{:x}", (i as i32).unsigned_abs())
+            } else {
+                format!("0x{:x}", i)
+            };
+
+            buf.clear();
+            buf.write_prefixed_i32(i as i32).expect("write succeeds");
+            assert_eq!(buf, expected);
+        },
+        8 => {
+            let i: u64 = u64::from_le_bytes(data.try_into().expect("checked the size is right"));
+
+            buf.clear();
+            buf.write_u64(i).expect("write succeeds");
+            assert_eq!(buf, format!("{:x}", i));
+
+            buf.clear();
+            buf.write_prefixed_u64(i).expect("write succeeds");
+            assert_eq!(buf, format!("0x{:x}", i));
+
+            let expected = if (i as i64) < 0 {
+                format!("-0x{:x}", (i as i64).unsigned_abs())
+            } else {
+                format!("0x{:x}", i)
+            };
+
+            buf.clear();
+            buf.write_prefixed_i64(i as i64).expect("write succeeds");
+            assert_eq!(buf, expected);
+        },
+        _ => {}
+    }
+});

goodfile

@@ -5,6 +5,7 @@ Step.push("build")
 Build.run({"cargo", "build"})
 
 Step.advance("test")
+-- TODO: set `-D warnings` here and below...
 Build.run({"cargo", "test"}, {name="test default features"})
 
 -- `cargo test` ends up running doc tests. great! but yaxpeax-arch's docs reference items in std only.
@@ -15,9 +16,16 @@ Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "s
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "colors"}, {name="test feature combinations"})
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "use-serde"}, {name="test feature combinations"})
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "address-parse"}, {name="test feature combinations"})
+Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "alloc"}, {name="test feature combinations"})
+Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "color-new"}, {name="test feature combinations"})
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "std,colors"}, {name="test feature combinations"})
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "std,use-serde"}, {name="test feature combinations"})
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "std,address-parse"}, {name="test feature combinations"})
+Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "std,address-parse,alloc"}, {name="test feature combinations"})
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "use-serde,colors,address-parse"}, {name="test feature combinations"})
+Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "use-serde,colors,address-parse,alloc"}, {name="test feature combinations"})
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "std,colors,address-parse"}, {name="test feature combinations"})
+Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "std,colors,address-parse,alloc"}, {name="test feature combinations"})
 Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "std,use-serde,colors"}, {name="test feature combinations"})
+Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "std,use-serde,colors,alloc"}, {name="test feature combinations"})
+Build.run({"cargo", "test", "--no-default-features", "--tests", "--features", "color-new,alloc"}, {name="test feature combinations"})

src/annotation/mod.rs

@@ -19,6 +19,8 @@
 //! in a generic setting, there isn't much to do with a `FieldDescription` other than display it. a
 //! typical use might look something like:
 //! ```
+//! #[cfg(feature="std")]
+//! # {
 //! use core::fmt;
 //!
 //! use yaxpeax_arch::annotation::{AnnotatingDecoder, VecSink};
@@ -40,6 +42,7 @@
 //!         println!("  bits [{}, {}]: {}", start, end, desc);
 //!     }
 //! }
+//! # }
 //! ```
 //!
 //! note that the range `[start, end]` for a reported span is _inclusive_. the `end`-th bit of a
@@ -73,7 +76,7 @@ use crate::{Arch, Reader};
 
 use core::fmt::Display;
 
-/// implementors of `DescriptionSink` receive descriptions of an instruction's disassembly process
+/// implementers of `DescriptionSink` receive descriptions of an instruction's disassembly process
 /// and relevant offsets in the bitstream being decoded. descriptions are archtecture-specific, and
 /// architectures are expected to be able to turn the bit-level `start` and `width` values into a
 /// meaningful description of bits in the original instruction stream.
@@ -91,24 +94,34 @@ impl<T> DescriptionSink<T> for NullSink {
     fn record(&mut self, _start: u32, _end: u32, _description: T) { }
 }
 
-#[cfg(feature = "std")]
-pub struct VecSink<T: Clone + Display> {
-    pub records: std::vec::Vec<(u32, u32, T)>
-}
+#[cfg(feature = "alloc")]
+mod vec_sink {
+    use alloc::vec::Vec;
+    use core::fmt::Display;
+    use crate::annotation::DescriptionSink;
 
-#[cfg(feature = "std")]
-impl<T: Clone + Display> VecSink<T> {
-    pub fn new() -> Self {
-        VecSink { records: std::vec::Vec::new() }
+    pub struct VecSink<T: Clone + Display> {
+        pub records: Vec<(u32, u32, T)>
+    }
+
+    impl<T: Clone + Display> VecSink<T> {
+        pub fn new() -> Self {
+            VecSink { records: Vec::new() }
+        }
+
+        pub fn into_inner(self) -> Vec<(u32, u32, T)> {
+            self.records
+        }
     }
-}
 
-#[cfg(feature = "std")]
-impl<T: Clone + Display> DescriptionSink<T> for VecSink<T> {
-    fn record(&mut self, start: u32, end: u32, description: T) {
-        self.records.push((start, end, description));
+    impl<T: Clone + Display> DescriptionSink<T> for VecSink<T> {
+        fn record(&mut self, start: u32, end: u32, description: T) {
+            self.records.push((start, end, description));
+        }
     }
 }
+#[cfg(feature = "alloc")]
+pub use vec_sink::VecSink;
 
 pub trait FieldDescription {
     fn id(&self) -> u32;
@@ -118,7 +131,7 @@ pub trait FieldDescription {
 /// an interface to decode [`Arch::Instruction`] words from a reader of [`Arch::Word`]s, with the
 /// decoder able to report descriptions of bits or fields in the instruction to a sink implementing
 /// [`DescriptionSink`]. the sink may be [`NullSink`] to discard provided data. decoding with a
-/// `NullSink` should behave identically to `Decoder::decode_into`. implementors are recommended to
+/// `NullSink` should behave identically to `Decoder::decode_into`. implementers are recommended to
 /// implement `Decoder::decode_into` as a call to `AnnotatingDecoder::decode_with_annotation` if
 /// implementing both traits.
 pub trait AnnotatingDecoder<A: Arch + ?Sized> {