romdata_check: stop charging a vtable's preamble to the symbol before it (465 -> 527 verified, 0 regressions)

tools/romdata_check.py

@@ -50,13 +50,15 @@
     python tools/romdata_check.py --json build/romdata.json
 """
 import argparse
+import bisect
 import collections
 import concurrent.futures
 import io
 import json
 import os
 import pathlib
 import re
+import struct
 import subprocess
 import sys
 import tempfile
@@ -103,14 +105,65 @@ def name_index():
         return _names
 
 
+def _vtable_preamble_at(label, addr, typeinfo):
+    """True when the two words BELOW a `_ZTV` address really are mwcc's preamble.
+
+    `symbols.txt` points `_ZTV<C>` at the slot array, so the object's offset-to-top and
+    typeinfo words sit at `addr - VTABLE_PREAMBLE` under no symbol at all. They are not
+    the previous symbol's bytes, but distance-to-the-next-symbol sizing hands them to it
+    anyway, and the compare then demands eight bytes the previous object never owned.
+
+    Not every `_ZTV` in this image has a preamble -- 9 of the 414 vtable addresses
+    config names do not -- so this is
+    gated on the cartridge rather than assumed: offset-to-top must be zero and the
+    typeinfo word must be null or an address `symbols.txt` gives to some `_ZTI`, in this
+    module or in arm9. Where the evidence is absent the boundary is not inserted and the
+    old sizing stands, which can only leave an extent too long (a PARTIAL that could
+    have been VERIFIED) and never too short (a VERIFIED that should not be).
+    """
+    m = RV.mod_for(label)
+    if m is None or addr - OI.VTABLE_PREAMBLE < m["base"]:
+        return False
+    head = RV.rom_bytes(label, addr - OI.VTABLE_PREAMBLE, OI.VTABLE_PREAMBLE)
+    if head is None or len(head) < OI.VTABLE_PREAMBLE:
+        return False
+    top, info = struct.unpack("<II", head)
+    return top == 0 and (info == 0 or info in typeinfo)
+
+
+def _module_extents(entries, has_preamble):
+    """{name: extent} for one module's `(addr, name)` list, sorted or not.
+
+    Split out from `rom_data_index` so the boundary arithmetic is testable without a
+    cartridge: `has_preamble(addr)` is the only thing that reads one.
+    """
+    entries = sorted(entries)
+    bounds = sorted({a for a, _ in entries}
+                    | {a - OI.VTABLE_PREAMBLE for a, n in entries
+                       if n.startswith("_ZTV") and has_preamble(a)})
+    out = {}
+    for addr, name in entries:
+        j = bisect.bisect_right(bounds, addr)
+        if j < len(bounds):
+            out[name] = bounds[j] - addr
+    return out
+
+
 def rom_data_index():
     """{(module, name): (addr, extent)} for every symbol the ROM config names.
 
-    `extent` is the distance to the next symbol in the same module, which is the only
+    `extent` is the distance to the next BOUNDARY in the same module, which is the only
     size information available: `symbols.txt` writes data as `kind:data(any)` with no
     size, while functions carry one. It is an upper bound on the object -- a trailing
     alignment gap belongs to nobody -- so a short compare is reported PARTIAL rather
     than being silently rounded up into a pass.
+
+    A boundary is any symbol address, plus the start of each vtable OBJECT whose
+    preamble `_vtable_preamble_at` can see in the cartridge. Without that second kind
+    the symbol preceding a vtable is charged with the vtable's own two preamble words
+    and scores PARTIAL by exactly eight bytes forever. Where `symbols.txt` already names
+    one or both of those words the real symbol is the nearer boundary and wins, so an
+    already-covered preamble is left alone.
     """
     global _index
     with _index_lock:
@@ -122,14 +175,17 @@ def rom_data_index():
                 m = _SYM_RE.match(line.strip())
                 if m:
                     per_module[label].append((int(m.group(3), 16), m.group(1)))
+        arm9_typeinfo = {a for a, n in per_module.get("arm9", ())
+                         if n.startswith("_ZTI")}
         index = {}
         for label, entries in per_module.items():
             entries.sort()
-            for i, (addr, name) in enumerate(entries):
-                nxt = next((entries[j][0] for j in range(i + 1, len(entries))
-                            if entries[j][0] > addr), None)
-                if nxt is not None:
-                    index[(label, name)] = (addr, nxt - addr)
+            typeinfo = {a for a, n in entries if n.startswith("_ZTI")} | arm9_typeinfo
+            extents = _module_extents(
+                entries, lambda a: _vtable_preamble_at(label, a, typeinfo))
+            for addr, name in entries:
+                if name in extents:
+                    index[(label, name)] = (addr, extents[name])
         _index = index
         return _index
 

tools/test_romdata_check.py

@@ -70,6 +70,110 @@ def test_empty_input_is_all_zero(self):
         self.assertEqual((s["symbols"], s["verified"], s["differs"], s["totalRecords"]),
                          (0, 0, 0, 0))
 
+import unittest.mock  # noqa: E402  (used by the preamble-gate tests below)
+
+
+class ExtentStopsAtTheVtablePreamble(unittest.TestCase):
+    """`symbols.txt` points `_ZTV<C>` at the slot array, so the two words below it are
+    the vtable object's own offset-to-top and typeinfo and belong to no symbol at all.
+    Distance-to-the-next-symbol sizing charged them to whatever came before, which then
+    scored PARTIAL by exactly eight bytes with nothing it could ever emit to close the
+    gap. 522 of this ROM's 540 vtables are unowned by any source, so this was not rare.
+    """
+    ALWAYS = staticmethod(lambda addr: True)
+    NEVER = staticmethod(lambda addr: False)
+
+    def test_previous_symbol_is_not_charged_for_the_preamble(self):
+        e = RDC._module_extents([(0x100, "data_x"), (0x120, "_ZTV3Foo")], self.ALWAYS)
+        self.assertEqual(e["data_x"], 0x18)
+
+    def test_without_the_fix_that_symbol_is_eight_bytes_short_forever(self):
+        e = RDC._module_extents([(0x100, "data_x"), (0x120, "_ZTV3Foo")], self.NEVER)
+        self.assertEqual(e["data_x"], 0x20)
+
+    def test_a_vtable_does_not_pay_for_the_next_vtables_preamble(self):
+        e = RDC._module_extents([(0x100, "_ZTV3Foo"), (0x190, "_ZTV3Bar")], self.ALWAYS)
+        self.assertEqual(e["_ZTV3Foo"], 0x88)
+
+    def test_a_named_preamble_word_keeps_only_the_bytes_it_owns(self):
+        """Where config already names one of the two words, that symbol is a boundary
+        too and simply gets the shorter extent. Seven symbols in this ROM sit exactly
+        four bytes below a vtable and eight sit exactly at V-8; the blanket
+        'subtract 8 from whatever precedes a vtable' formulation sized 23 of them <= 0,
+        one of them -4, which is why this is a boundary set and not a subtraction."""
+        e = RDC._module_extents(
+            [(0x100, "data_x"), (0x11c, "data_typeinfo_word"), (0x120, "_ZTV3Foo")],
+            self.ALWAYS)
+        self.assertEqual(e["data_x"], 0x18)
+        self.assertEqual(e["data_typeinfo_word"], 4)
+        self.assertTrue(all(v > 0 for v in e.values()))
+
+    def test_a_symbol_exactly_at_the_preamble_is_unchanged(self):
+        e = RDC._module_extents(
+            [(0x118, "data_preamble"), (0x120, "_ZTV3Foo")], self.ALWAYS)
+        self.assertEqual(e["data_preamble"], 8)
+
+    def test_the_last_symbol_still_has_no_extent(self):
+        e = RDC._module_extents([(0x100, "data_x"), (0x120, "_ZTV3Foo")], self.ALWAYS)
+        self.assertNotIn("_ZTV3Foo", e)
+
+
+class PreambleIsGatedOnTheCartridge(unittest.TestCase):
+    """9 of this ROM's 414 vtable addresses show something other than a preamble below
+    them -- strings, code, a non-zero first word. The boundary is inserted only where
+    the cartridge shows one, so an absent preamble leaves an extent too LONG (a PARTIAL
+    that might have been VERIFIED) and never too short (a VERIFIED that should not be).
+    """
+    def _rv(self, words):
+        import struct as _s
+
+        class FakeRV:
+            @staticmethod
+            def mod_for(label):
+                return {"base": 0x100}
+
+            @staticmethod
+            def rom_bytes(label, addr, size):
+                return _s.pack("<II", *words)[:size]
+        return FakeRV
+
+    def check(self, words, typeinfo=frozenset()):
+        with unittest.mock.patch.object(RDC, "RV", self._rv(words)):
+            return RDC._vtable_preamble_at("ov006", 0x200, typeinfo)
+
+    def test_zero_and_a_known_typeinfo_is_a_preamble(self):
+        self.assertTrue(self.check((0, 0x1234), {0x1234}))
+
+    def test_zero_and_null_typeinfo_is_a_preamble(self):
+        """A vtable whose class carries no RTTI still gets the two words."""
+        self.assertTrue(self.check((0, 0)))
+
+    def test_a_nonzero_offset_to_top_is_not_a_preamble(self):
+        self.assertFalse(self.check((0x2086f58, 0x1234), {0x1234}))
+
+    def test_an_unrecognized_typeinfo_word_is_not_a_preamble(self):
+        """`_ZTV8dActor_c` reads 'Play' 'Roo' below it -- the tail of a string."""
+        self.assertFalse(self.check((0x79616c50, 0x6f6f5220)))
+
+    def test_a_vtable_at_the_module_base_has_nothing_below_it(self):
+        class FakeRV:
+            @staticmethod
+            def mod_for(label):
+                return {"base": 0x200}
+
+            @staticmethod
+            def rom_bytes(label, addr, size):  # pragma: no cover - never reached
+                raise AssertionError("read below the module base")
+        with unittest.mock.patch.object(RDC, "RV", FakeRV):
+            self.assertFalse(RDC._vtable_preamble_at("ov006", 0x200, frozenset()))
+
+    def test_a_module_with_no_image_is_not_a_preamble(self):
+        class FakeRV:
+            @staticmethod
+            def mod_for(label):
+                return None
+        with unittest.mock.patch.object(RDC, "RV", FakeRV):
+            self.assertFalse(RDC._vtable_preamble_at("ov006", 0x200, frozenset()))
 
 if __name__ == "__main__":
     unittest.main()