From 6262b7e5667cd3da30ed2cd01ee7c19e9c3695c8 Mon Sep 17 00:00:00 2001
From: andrewboudreau <andrewboudreau@gmail.com>
Date: Mon, 31 Aug 2026 17:47:59 -0500
Subject: [PATCH] romdata_check: stop charging a vtable's preamble to the
 symbol before it

`symbols.txt` points `_ZTV<C>` at the slot array, so the vtable object's
offset-to-top and typeinfo words sit at `addr - 8` under no symbol at all.
`rom_data_index()` sized every data symbol as distance-to-the-next-symbol and
therefore handed those eight bytes to whatever came before, which then scored
PARTIAL by exactly eight bytes with nothing it could ever emit to close the gap.
`check_symbol` already knows about the preamble -- it applies
`OI.VTABLE_PREAMBLE` on the emitted side and corrects reloc addends by the same
8 -- so this was the one place that didn't.

522 of this ROM's 540 `_ZTV` symbols are unowned by any source, so the defect is
not rare: measured on this tree, `verified` goes 465 -> 527 and `partial` 253 ->
191, with **zero** records moving the other way and `differ` unchanged at 6.

Formulated as a boundary set rather than a subtraction, because config already
names some preamble words: a blanket "subtract 8 from whatever precedes a
vtable" drove 23 extents to <= 0, one of them to -4. Where a real symbol sits
nearer than V-8 it simply wins and gets the shorter extent.

Gated on the cartridge rather than assumed. 9 of the 414 vtable addresses config
names have something other than a preamble below them -- `_ZTV8dActor_c` reads
the tail of a string, `_ZTV8dCcPos_c` two code pointers -- and inserting a
boundary there would strip eight owned bytes off a neighbour and manufacture a
false VERIFIED, which is this same defect with the sign flipped. So the boundary
goes in only where offset-to-top is zero and the typeinfo word is null or an
address `symbols.txt` gives to some `_ZTI`. Where the evidence is absent the old
sizing stands, which can only leave an extent too long (a PARTIAL that could
have been VERIFIED) and never too short.

The boundary arithmetic is split into a pure `_module_extents(entries,
has_preamble)` so CI can test it with a stub predicate and no cartridge.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ
---
 tools/romdata_check.py      |  68 ++++++++++++++++++++---
 tools/test_romdata_check.py | 104 ++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+), 6 deletions(-)

diff --git a/tools/romdata_check.py b/tools/romdata_check.py
index 6ed50d4e69..96e1ccbafd 100644
--- a/tools/romdata_check.py
+++ b/tools/romdata_check.py
@@ -50,6 +50,7 @@
     python tools/romdata_check.py --json build/romdata.json
 """
 import argparse
+import bisect
 import collections
 import concurrent.futures
 import io
@@ -57,6 +58,7 @@
 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
 
diff --git a/tools/test_romdata_check.py b/tools/test_romdata_check.py
index 18bde87154..eb19a061f8 100644
--- a/tools/test_romdata_check.py
+++ b/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()