ship one record per function: merge the release set, gen reads it, descriptions as doc comments, gates for what was only claimed; v3.0

src/concmd.rs

@@ -74,7 +74,17 @@ const R9: usize = 9;
 /// Caller-saved under SysV: a call destroys any constant we were tracking in these. The `this` a
 /// constructor threads through its registrations is callee-saved (rbx, r12-r15), so it survives — which
 /// is what makes the member-callback form readable at all.
-const CLOBBER: [usize; 9] = [0, RCX, RDX, RSI, RDI, R8, R9, 10, 11];
+///
+/// DERIVED from `abi::CALLER_SAVED` rather than re-listed. It is a fixed SysV fact and was spelled out
+/// three times across two readers and the ABI measurer; a register present in one list and missing from
+/// another is a tracker that either forgets a value the machine kept or keeps one it destroyed.
+fn clobbered() -> [usize; 9] {
+    let mut out = [0usize; 9];
+    for (i, &r) in crate::abi::CALLER_SAVED.iter().enumerate() {
+        out[i] = crate::abi::gp_slot(r).expect("every caller-saved register is a GPR");
+    }
+    out
+}
 
 /// Longest string accepted as a command name. Names are identifiers; anything longer is not one, so the
 /// cap doubles as a validity gate.
@@ -195,10 +205,10 @@ pub fn flag_names(flags: u64) -> Vec<&'static str> {
 }
 
 /// Index 0-15 of a GPR, after widening an 8/16/32-bit name to its 64-bit parent.
+/// The 64-bit parent register as a slot index. Thin wrapper over [`crate::abi::gp_slot`] — the mapping is a
+/// fixed SysV fact, and this file only narrows it to the `u8` its `[_; 16]` arrays index by.
 fn gpr(r: Register) -> Option<u8> {
-    let f = r.full_register();
-    f.is_gpr64()
-        .then(|| (f as usize - Register::RAX as usize) as u8)
+    crate::abi::gp_slot(r).map(|s| s as u8)
 }
 
 /// What a register provably holds. `Sym` is an offset from a value we never learned — a constructor's
@@ -233,6 +243,18 @@ impl V {
     }
 }
 
+/// End a register's current life, because something has overwritten it.
+///
+/// Called by EVERY arm that assigns to a register, not only the catch-all — which is the correction that
+/// matters. `mov`, `lea` and `xor` re-point a register just as surely as an unmodelled instruction does,
+/// so leaving their epoch alone let a base register be aimed at a second object while stores made against
+/// the FIRST still keyed to the same `(register, epoch)` pair — and a `lea rdx,[rbx+0x1c8]` for object B
+/// could then match a `mov [rbx+0x1e8],rax` that belonged to object A, attributing one constructor's
+/// handler to another's registration.
+fn end_life(epoch: &mut [u32; 16], d: u8) {
+    epoch[d as usize] = epoch[d as usize].saturating_add(1);
+}
+
 /// Address of a `this`-relative slot: base register, that register's epoch, displacement.
 type Slot = (u8, u32, i64);
 
@@ -285,13 +307,24 @@ fn inits_invalid_handle(img: &CodeImage, f: u64) -> bool {
 }
 
 /// A plausible console-command name: short, printable, no spaces or quoting.
-fn cmd_name(img: &CodeImage, va: u64) -> Option<String> {
-    let s = img.read_c_string(va)?;
-    let ok = !s.is_empty()
+/// Whether `s` is shaped like a console COMMAND name.
+///
+/// Split from the read so a test can call the rule instead of restating it — restating it is how the
+/// convar test came to assert this rule while claiming to pin the other one, and would have passed with
+/// the two gates swapped.
+///
+/// Deliberately looser than [`is_convar_name`]: a command name may lead with punctuation, because the
+/// `+bugvoice` / `-bugvoice` on/off pairs are real commands and a convar can never be spelled that way.
+fn is_cmd_name(s: &str) -> bool {
+    !s.is_empty()
         && s.len() <= MAX_NAME
         && s.bytes()
-            .all(|c| c.is_ascii_graphic() && c != b'"' && c != b'%');
-    ok.then_some(s)
+            .all(|c| c.is_ascii_graphic() && c != b'"' && c != b'%')
+}
+
+fn cmd_name(img: &CodeImage, va: u64) -> Option<String> {
+    let s = img.read_c_string(va)?;
+    is_cmd_name(&s).then_some(s)
 }
 
 /// Every console command `img` registers.
@@ -316,10 +349,7 @@ fn collect_sites(
     img: &CodeImage,
     mut accept: impl FnMut(&CodeImage, u64, &[V; 16]) -> bool,
 ) -> Vec<Site> {
-    let mut entries = crate::locate::candidate_entries(img);
-    entries.extend(img.eh_frame_functions().into_iter().map(|(s, _)| s));
-    entries.sort_unstable();
-    entries.dedup();
+    let entries = crate::locate::function_entries(img);
 
     let mut sites: Vec<Site> = Vec::new();
     let mut factory = InstructionInfoFactory::new();
@@ -351,7 +381,13 @@ fn collect_sites(
                 if accept(img, t, &val) {
                     found.push((t, val));
                 }
-                for c in CLOBBER {
+                // A call assigns to NINE registers at once, so it ends nine lives — the arm that most
+                // needs the epoch bump and the one that was missing it. The Lea arm below mints a fresh
+                // symbolic base for any unknown-valued register, and the store arm keys that base as
+                // `(reg, epoch, disp)`: without the bump, `rax` after two successive calls is ONE key
+                // space shared by two objects, where same-displacement stores overwrite each other.
+                for c in clobbered() {
+                    end_life(&mut epoch, c as u8);
                     val[c] = V::Unknown;
                 }
                 continue;
@@ -385,6 +421,7 @@ fn collect_sites(
                 // `lea r,[rip+d]` is a string/global/function address; `lea r,[base+d]` walks to a member.
                 Mnemonic::Lea => {
                     if let Some(d) = gpr(insn.op0_register()) {
+                        end_life(&mut epoch, d);
                         val[d as usize] = if insn.is_ip_rel_memory_operand() {
                             V::Const(insn.ip_rel_memory_address())
                         } else if insn.memory_index() == Register::None {
@@ -401,6 +438,7 @@ fn collect_sites(
                 }
                 Mnemonic::Mov => {
                     if let Some(d) = gpr(insn.op0_register()) {
+                        end_life(&mut epoch, d);
                         val[d as usize] = match insn.op1_kind() {
                             OpKind::Immediate8to64
                             | OpKind::Immediate32to64
@@ -419,6 +457,7 @@ fn collect_sites(
                 Mnemonic::Xor => {
                     if let (Some(d), Some(s)) = (gpr(insn.op0_register()), gpr(insn.op1_register()))
                     {
+                        end_life(&mut epoch, d);
                         val[d as usize] = if d == s { V::Const(0) } else { V::Unknown };
                     }
                 }
@@ -432,8 +471,8 @@ fn collect_sites(
                             OpAccess::Write | OpAccess::ReadWrite | OpAccess::CondWrite
                         ) && let Some(d) = gpr(ur.register())
                         {
+                            end_life(&mut epoch, d);
                             val[d as usize] = V::Unknown;
-                            epoch[d as usize] = epoch[d as usize].saturating_add(1);
                         }
                     }
                 }
@@ -508,62 +547,6 @@ fn interpret_commands(img: &CodeImage, sites: &[Site]) -> Vec<ConsoleCommand> {
     out
 }
 
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn only_measured_flag_bits_are_named() {
-        // bot_add ships 0x80004 = bits 2 and 19. Bit 19 is `release`; bit 2 stays unnamed because no
-        // name in Valve's dump matches it, and naming it anyway is the whole mistake to avoid.
-        assert_eq!(flag_names(0x80004), vec!["release"]);
-        // bot_place ships 0x4004 = bits 2 and 14 — bit 14 is `cheat`.
-        assert_eq!(flag_names(0x4004), vec!["cheat"]);
-        // A command with no flags names none, rather than falling back to a default.
-        assert!(flag_names(0).is_empty());
-        // Every listed bit is distinct and in range.
-        let mut seen: Vec<u32> = FLAG_BITS.iter().map(|&(b, _)| b).collect();
-        seen.sort_unstable();
-        seen.dedup();
-        assert_eq!(seen.len(), FLAG_BITS.len());
-        assert!(FLAG_BITS.iter().all(|&(b, _)| b < 64));
-    }
-
-    #[test]
-    fn symbolic_offsets_keep_the_base_and_track_the_epoch() {
-        // A `this`-relative walk composes, so `lea rax,[rbx+0x1c8]` then `lea rdx,[rax+0x40]` addresses
-        // the same object the constructor stored into.
-        assert_eq!(V::Sym(3, 0, 0x1c8).offset(0x40), V::Sym(3, 0, 0x208));
-        // A constant walk stays constant.
-        assert_eq!(V::Const(0x1000).offset(8), V::Const(0x1008));
-        // Nothing is invented from nothing.
-        assert_eq!(V::Unknown.offset(8), V::Unknown);
-        // Two runs of the same register never address each other's slots.
-        assert_ne!(V::Sym(3, 0, 0x1c8), V::Sym(3, 1, 0x1c8));
-        // Only a constant is a usable address.
-        assert_eq!(V::Const(7).konst(), Some(7));
-        assert_eq!(V::Sym(3, 0, 7).konst(), None);
-    }
-
-    #[test]
-    fn a_command_name_is_an_identifier_not_prose() {
-        // The gate is applied to a resolved string, so exercise it through the same predicate the
-        // reader uses by checking the shape rules it encodes.
-        let ok = |s: &str| {
-            !s.is_empty()
-                && s.len() <= MAX_NAME
-                && s.bytes()
-                    .all(|c| c.is_ascii_graphic() && c != b'"' && c != b'%')
-        };
-        assert!(ok("bot_add"));
-        assert!(ok("+bugvoice")); // an on/off pair is a real command name
-        assert!(!ok("")); // an empty string is not a name
-        assert!(!ok("Adds a bot matching the given criteria.")); // a description
-        assert!(!ok("%s: no varname specified\n")); // a format string
-        assert!(!ok(&"x".repeat(MAX_NAME + 1)));
-    }
-}
-
 /// How many call sites must present the convar argument shape before a target counts as a registrar.
 ///
 /// This is the whole safety margin for identifying convar registration by shape rather than by a semantic
@@ -597,16 +580,24 @@ pub struct ConVar {
 /// Stricter than [`cmd_name`], which admits any printable run because commands like `+bugvoice` exist.
 /// A convar name is always an identifier, and the tighter gate is what keeps prose out of the name slot
 /// when the shape test is the only thing standing between a call site and a record.
-fn convar_name(img: &CodeImage, va: u64) -> Option<String> {
-    let s = img.read_c_string(va)?;
-    let ok = !s.is_empty()
+/// Whether `s` is shaped like a CONVAR name — stricter than [`is_cmd_name`] in both directions: it must
+/// LEAD with a letter or underscore, and its body admits only `[A-Za-z0-9_.]`.
+///
+/// This gate is the only shape check between a call site and an emitted ConVar record, so it is what keeps
+/// prose out of the name slot.
+fn is_convar_name(s: &str) -> bool {
+    !s.is_empty()
         && s.len() <= MAX_NAME
         && s.chars()
             .next()
             .is_some_and(|c| c.is_ascii_alphabetic() || c == '_')
         && s.bytes()
-            .all(|c| c.is_ascii_alphanumeric() || c == b'_' || c == b'.');
-    ok.then_some(s)
+            .all(|c| c.is_ascii_alphanumeric() || c == b'_' || c == b'.')
+}
+
+fn convar_name(img: &CodeImage, va: u64) -> Option<String> {
+    let s = img.read_c_string(va)?;
+    is_convar_name(&s).then_some(s)
 }
 
 /// Help text: prose, or nothing. Deliberately permissive about content and strict about being a real
@@ -844,3 +835,110 @@ pub fn convars(img: &CodeImage, library: &str) -> Vec<ConVar> {
     out.dedup_by(|a, b| a.name == b.name && a.addr == b.addr);
     out
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // ---- command reader ----
+
+    #[test]
+    fn only_measured_flag_bits_are_named() {
+        // bot_add ships 0x80004 = bits 2 and 19. Bit 19 is `release`; bit 2 stays unnamed because no
+        // name in Valve's dump matches it, and naming it anyway is the whole mistake to avoid.
+        assert_eq!(flag_names(0x80004), vec!["release"]);
+        // bot_place ships 0x4004 = bits 2 and 14 — bit 14 is `cheat`.
+        assert_eq!(flag_names(0x4004), vec!["cheat"]);
+        // A command with no flags names none, rather than falling back to a default.
+        assert!(flag_names(0).is_empty());
+        // Every listed bit is distinct and in range.
+        let mut seen: Vec<u32> = FLAG_BITS.iter().map(|&(b, _)| b).collect();
+        seen.sort_unstable();
+        seen.dedup();
+        assert_eq!(seen.len(), FLAG_BITS.len());
+        assert!(FLAG_BITS.iter().all(|&(b, _)| b < 64));
+    }
+
+    #[test]
+    fn symbolic_offsets_keep_the_base_and_track_the_epoch() {
+        // A `this`-relative walk composes, so `lea rax,[rbx+0x1c8]` then `lea rdx,[rax+0x40]` addresses
+        // the same object the constructor stored into.
+        assert_eq!(V::Sym(3, 0, 0x1c8).offset(0x40), V::Sym(3, 0, 0x208));
+        // A constant walk stays constant.
+        assert_eq!(V::Const(0x1000).offset(8), V::Const(0x1008));
+        // Nothing is invented from nothing.
+        assert_eq!(V::Unknown.offset(8), V::Unknown);
+        // Two runs of the same register never address each other's slots.
+        assert_ne!(V::Sym(3, 0, 0x1c8), V::Sym(3, 1, 0x1c8));
+        // Only a constant is a usable address.
+        assert_eq!(V::Const(7).konst(), Some(7));
+        assert_eq!(V::Sym(3, 0, 7).konst(), None);
+    }
+
+    #[test]
+    fn every_arm_that_clobbers_a_register_ends_its_life() {
+        // The invariant `end_life` documents, checked against the arm that breaks it most cheaply. A
+        // CALL assigns to nine caller-saved registers at once; leaving their epochs alone made `rax`
+        // after two successive calls one key space shared by two objects, so a store to `[rax+0x18]`
+        // made through the FIRST could be read back as a slot of the SECOND — and the member-callback
+        // recovery ships whatever executable pointer that merged window holds.
+        let mut epoch = [0u32; 16];
+        let clobber = clobbered();
+        let before: Vec<u32> = clobber.iter().map(|&c| epoch[c]).collect();
+        for c in clobbered() {
+            end_life(&mut epoch, c as u8);
+        }
+        for (i, &c) in clobber.iter().enumerate() {
+            assert_eq!(
+                epoch[c],
+                before[i] + 1,
+                "register {c} kept its epoch across a call"
+            );
+            // …and the two runs are therefore distinguishable keys, which is the point.
+            assert_ne!(
+                V::Sym(c as u8, before[i], 0x18),
+                V::Sym(c as u8, epoch[c], 0x18)
+            );
+        }
+        // A callee-saved register is NOT clobbered: the registration `this` a constructor threads
+        // through survives the call, which is what the epoch widening was for in the first place.
+        for saved in [3u8 /* rbx */, 12, 13, 14, 15] {
+            assert!(
+                !clobber.contains(&(saved as usize)),
+                "r{saved} is callee-saved and must survive a call"
+            );
+        }
+    }
+
+    #[test]
+    fn a_command_name_is_an_identifier_not_prose() {
+        // Calls the shipped rule, for the same reason as the convar test below it.
+        assert!(is_cmd_name("bot_add"));
+        assert!(is_cmd_name("+bugvoice")); // an on/off pair is a real command name
+        assert!(!is_cmd_name("")); // an empty string is not a name
+        assert!(!is_cmd_name("Adds a bot matching the given criteria.")); // a description (spaces)
+        assert!(!is_cmd_name("%s: no varname specified\n")); // a format string
+        assert!(!is_cmd_name(&"x".repeat(MAX_NAME + 1)));
+    }
+
+    // ---- convar reader ----
+
+    #[test]
+    fn a_convar_name_is_stricter_than_a_command_name() {
+        // CALLS the gate rather than restating it. The previous version of this test re-implemented the
+        // COMMAND rule and asserted only inputs both rules agree on, so it would have passed with the two
+        // gates swapped — the exact regression it is named for.
+        assert!(is_convar_name("sv_cheats"));
+        assert!(is_convar_name("mp_roundtime_defuse"));
+        assert!(is_convar_name("_internal.thing")); // leading underscore and a dot are both legal
+        assert!(!is_convar_name(""));
+        assert!(!is_convar_name("Set to 1 to enable cheats")); // help text, not a name
+        assert!(!is_convar_name(&"x".repeat(MAX_NAME + 1)));
+
+        // The DISCRIMINATING cases — the ones that fail if the two gates are confused. A command may lead
+        // with punctuation (the `+`/`-` on/off pairs); a convar may not, and admits no other punctuation.
+        assert!(is_cmd_name("+bugvoice") && !is_convar_name("+bugvoice"));
+        assert!(is_cmd_name("1st_arg") && !is_convar_name("1st_arg")); // digit-led
+        assert!(is_cmd_name("say/all") && !is_convar_name("say/all"));
+    }
+}