Merge #564

564: Make order of bitflags values deterministic r=Urhengulas a=jonas-schievink Fixes #563 Since each bitflags value is stored in its own symbol, their order in the final executable is up to rustc/LLVM, or even the linker. This causes unreliable output that depends on toolchain details. This PR fixes that by storing an index in the symbol for bitflags values, which indicates their order in the source code. The decoder sorts by this index to put the values back in definition order. We now exactly match the output of the `bitflags` crate. Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
knurling-rs · Aug 25, 2021 · 45beb42 · 45beb42
2 parents 3b4af2f + 87aa5e4
commit 45beb42
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 13 deletions.
diff --git a/decoder/src/elf2table/mod.rs b/decoder/src/elf2table/mod.rs
@@ -161,8 +161,10 @@ pub fn parse_impl(elf: &[u8], check_version: bool) -> Result<Option<Table>, anyh
                     log::debug!("bitflags value `{}` has value {:#x}", sym.data(), value);
 
                     let segments = sym.data().split("::").collect::<Vec<_>>();
-                    let (bitflags_name, value_name) = match &*segments {
-                        [bitflags_name, value_name] => (*bitflags_name, *value_name),
+                    let (bitflags_name, value_idx, value_name) = match &*segments {
+                        [bitflags_name, value_idx, value_name] => {
+                            (*bitflags_name, value_idx.parse::<u128>()?, *value_name)
+                        }
                         _ => bail!("malformed bitflags value string '{}'", sym.data()),
                     };
 
@@ -172,10 +174,11 @@ pub fn parse_impl(elf: &[u8], check_version: bool) -> Result<Option<Table>, anyh
                         disambig: sym.disambiguator().into(),
                     };
 
-                    bitflags_map
-                        .entry(key)
-                        .or_insert_with(Vec::new)
-                        .push((value_name.into(), value));
+                    bitflags_map.entry(key).or_insert_with(Vec::new).push((
+                        value_name.into(),
+                        value_idx,
+                        value,
+                    ));
                 }
                 symbol::SymbolTag::Defmt(tag) => {
                     map.insert(
@@ -191,10 +194,26 @@ pub fn parse_impl(elf: &[u8], check_version: bool) -> Result<Option<Table>, anyh
         }
     }
 
+    // Sort bitflags values by the value's index in definition order. Since all values get their own
+    // symbol and section, their order in the final binary is unspecified and can't be relied on, so
+    // we put them back in the original order here.
+    let bitflags = bitflags_map
+        .into_iter()
+        .map(|(k, mut values)| {
+            values.sort_by_key(|(_, index, _)| *index);
+            let values = values
+                .into_iter()
+                .map(|(name, _index, value)| (name, value))
+                .collect();
+
+            (k, values)
+        })
+        .collect();
+
     Ok(Some(Table {
         entries: map,
         timestamp,
-        bitflags: bitflags_map,
+        bitflags,
         encoding,
     }))
 }

diff --git a/decoder/src/frame.rs b/decoder/src/frame.rs
@@ -226,8 +226,6 @@ impl<'t> Frame<'t> {
                 };
                 match self.table.bitflags.get(&key) {
                     Some(flags) => {
-                        // FIXME flag print order does not match definition order
-                        // (does some part of the toolchain alphabetically sort them?)
                         let set_flags = flags
                             .iter()
                             .filter(|(_, value)| {

diff --git a/firmware/qemu/src/bin/bitflags.out b/firmware/qemu/src/bin/bitflags.out
@@ -6,7 +6,7 @@ INFO Flags::FLAG_1: FLAG_1
 INFO Flags::FLAG_1: FLAG_1 (fmt::Debug)
 INFO Flags::FLAG_7: FLAG_7
 INFO Flags::FLAG_7: FLAG_7 (fmt::Debug)
-INFO LargeFlags::ALL: ALL | MSB | NON_LITERAL
+INFO LargeFlags::ALL: MSB | ALL | NON_LITERAL
 INFO LargeFlags::ALL: MSB | ALL | NON_LITERAL (fmt::Debug)
 INFO LargeFlags::empty(): (empty)
 INFO LargeFlags::empty(): (empty) (fmt::Debug)
diff --git a/firmware/qemu/src/bin/bitflags.release.out b/firmware/qemu/src/bin/bitflags.release.out
@@ -6,7 +6,7 @@ INFO Flags::FLAG_1: FLAG_1
 INFO Flags::FLAG_1: FLAG_1 (fmt::Debug)
 INFO Flags::FLAG_7: FLAG_7
 INFO Flags::FLAG_7: FLAG_7 (fmt::Debug)
-INFO LargeFlags::ALL: ALL | MSB | NON_LITERAL
+INFO LargeFlags::ALL: MSB | ALL | NON_LITERAL
 INFO LargeFlags::ALL: MSB | ALL | NON_LITERAL (fmt::Debug)
 INFO LargeFlags::empty(): (empty)
 INFO LargeFlags::empty(): (empty) (fmt::Debug)
diff --git a/macros/src/items/bitflags.rs b/macros/src/items/bitflags.rs
@@ -61,15 +61,16 @@ pub(crate) fn expand(input: TokenStream) -> TokenStream {
 fn codegen_flag_statics(input: &Input) -> Vec<TokenStream2> {
     input
         .flags()
-        .map(|flag| {
+        .enumerate()
+        .map(|(i, flag)| {
             let cfg_attrs = flag.cfg_attrs();
             let var_name = &flag.ident();
             let value = &flag.value();
             let repr_ty = &input.ty();
 
             let sym_name = construct::mangled_symbol_name(
                 "bitflags_value",
-                &format!("{}::{}", input.ident(), flag.ident()),
+                &format!("{}::{}::{}", input.ident(), i, flag.ident()),
             );
 
             quote! {