The guest's assembly conversion keeps string literals byte for byte

The arm64 and Android guests are compiled for arm64_32-apple-watchos and
their Darwin assembly converted to ELF (tools/android_asm_convert.py). The
converter took a quoted .ascii/.asciz operand for a quoted symbol name and
dropped a leading "_" from it, as it does from symbols; and its quoted-name
pattern ended at an escaped quote (\"), so the "_" of a word after one was
dropped too. Clang writes byte tables as .ascii strings, so a table holding
95 ("_") lost that byte and everything after it moved down by one.

In Opus that table was CELT's pulse cache (modes.c, cache_bits50): the bits
a band may have are read off it, and shifted by a byte it lets a band have
more pulses than the PVQ tables hold. Decoding a frame from another player
then read past CELT_PVQ_U_ROW in decode_pulses (n 32, k 240) and crashed: a
client on the Steam Frame, in a CTF game on the internet. The guest's own
encoder could crash likewise in encode_pulses. The same build of Opus,
driven with the game's settings and with clean, corrupt, truncated and
random packets and voice_audio_play's loss handling, crashed within a few
thousand frames (and identically at -O0); with the fix it ran 4.8 million
decodes clean, and its encoded bytes and decoded samples are identical to a
native 64-bit build's.

The same loss hit other strings and tables of the guest: musl's iswalpha,
iswpunct and towctrans tables and iconv's charmaps; tomlc17's "_0123..."
digit sets (TOML numbers with "_"); the attract mode's "_de"/"_fr" names,
"_lan_", "_lit" and "_%s"; the progress bar's compressed monk tone and the
DirectSound effects image; and assertion and error texts. All are now
assembled as compiled.

String directives now go through unchanged, and quoted names honour
escapes. tools/test_android_asm_convert.py checks both, and a table of
every byte value through clang, the conversion and the assembler.

.github/workflows/build.yml

@@ -91,7 +91,7 @@ jobs:
 
       - name: Test
         run: |
-          python -m pytest -q tools/test_linux_port.py tools/test_host_watch_hash.py tools/test_touch_menu.py tools/harness tools/test_profile.py
+          python -m pytest -q tools/test_linux_port.py tools/test_android_asm_convert.py tools/test_host_watch_hash.py tools/test_touch_menu.py tools/harness tools/test_profile.py
           python tools/test_light_storage.py
 
       - name: Compile cache

port/android/README.md

@@ -307,7 +307,9 @@ The guest is the game, the platform layer and a small runtime:
    AArch64 target of clang. The options `-U__APPLE__` and
    `-fno-define-target-os-macros` hide the Darwin environment.
 2. `tools/android_asm_convert.py` changes the Mach-O assembly to ELF
-   assembly.
+   assembly. It renames symbols, but keeps string literals (`.ascii`,
+   `.asciz`) byte for byte: they hold tables as well as text
+   (`tools/test_android_asm_convert.py`).
 3. The AArch64 assembler makes the objects.
 4. `ld.lld` links the objects with `guest/guest.ld` to a static image,
    `build/android/halo_guest.elf`, at a fixed address above the Xbox memory

tools/android_asm_convert.py

@@ -15,7 +15,13 @@ Usage: android_asm_convert.py input.s output.s
 import re
 import sys
 
-IDENT = re.compile(r'"[^"]*"|[A-Za-z_.$][\w.$]*')
+# a quoted name (its escapes kept: \" does not end it) or a plain one
+IDENT = re.compile(r'"(?:[^"\\]|\\.)*"|[A-Za-z_.$][\w.$]*')
+
+# directives whose operands are string literals, the same in both
+# syntaxes: they go through as they are, never renamed as symbols (data
+# starting with "_" would lose that byte)
+STRING_DIRECTIVES = {".ascii", ".asciz", ".string"}
 
 DROP_DIRECTIVES = {
     ".subsections_via_symbols", ".build_version", ".watchos_version_min",
@@ -217,6 +223,9 @@ class Converter:
         if op == ".align":
             self.emit(f"\t.p2align {args}")
             return
+        if op in STRING_DIRECTIVES:
+            self.emit(f"\t{op} {args}")
+            return
         if op.startswith("."):
             self.emit(f"\t{op} {self.operands(args)}" if args else f"\t{op}")
             return

tools/embed_assets.py

@@ -22,7 +22,7 @@ no files beside its guest image.
 
 The data are 32-bit words, not bytes: the Android build passes the guest's
 assembly through tools/android_asm_convert.py, which rewrites identifiers in
-operands and would garble a long .ascii string of binary data.
+operands (it keeps .ascii strings as they are, but once garbled them).
 """
 
 import json

tools/test_android_asm_convert.py

@@ -0,0 +1,76 @@
+"""The Darwin to ELF assembly conversion of the ILP32 guest
+(tools/android_asm_convert.py): string literals keep every byte. It once took
+a quoted .ascii operand for a quoted symbol name and dropped its leading "_"
+(and, after an escaped quote, the "_" of any word after it), which shifted
+tables such as Opus's pulse cache by a byte: voice chat crashed in
+decode_pulses on the arm64 and Android builds."""
+import shutil
+import struct
+import subprocess
+import sys
+from pathlib import Path
+
+import pytest
+
+from tools.android_asm_convert import Converter
+
+ROOT = Path(__file__).resolve().parent.parent
+
+
+def convert(*lines):
+    return Converter(list(lines)).run().splitlines()
+
+
+@pytest.mark.parametrize("directive", [
+    '.ascii\t"_c[}]L{s{"',
+    '.asciz\t"_transport_error_none"',
+    '.ascii\t"v\\177\\"O7v\\177_\\037\\374"',
+    '.ascii\t"=`\'\\\\_\\026"',
+    '.asciz\t"ltmp0 _x @PAGE ; not a comment"',
+])
+def test_string_literals_are_kept(directive):
+    op, args = directive.split("\t", 1)
+    assert convert("l_.str:", directive) == [".Ll_.str:", f"\t{op} {args}"]
+
+
+def test_symbols_are_still_renamed():
+    assert convert("\t.globl\t_table", "_table:", "\tadrp\tx8, _table@PAGE",
+                   "\tadd\tx8, x8, _table@PAGEOFF", '\t.long\t"_odd name"') == [
+        "\t.globl table", "table:", "\tadrp x8, table", "\tadd x8, x8, :lo12:table", '\t.long "odd name"']
+
+
+def rodata(elf: bytes) -> bytes:
+    """the .rodata section of a 64-bit little-endian ELF object"""
+    shoff, = struct.unpack_from("<Q", elf, 0x28)
+    shentsize, shnum, shstrndx = struct.unpack_from("<HHH", elf, 0x3a)
+    sections = [struct.unpack_from("<IIQQQQ", elf, shoff + index * shentsize) for index in range(shnum)]
+    names = sections[shstrndx][4]
+    for name, _, _, _, offset, size in sections:
+        if elf[names + name:elf.index(b"\0", names + name)] == b".rodata":
+            return elf[offset:offset + size]
+    raise AssertionError("no .rodata")
+
+
+def test_tables_survive_the_build(tmp_path):
+    """every byte value, and the patterns that were lost, through the
+    guest's pipeline (clang for arm64_32, the conversion, the assembler)"""
+    clang = shutil.which("clang")
+    if not clang:
+        pytest.skip("no clang")
+    table = bytes(range(256)) + b'_a"_b\\_c' + bytes(reversed(range(256)))
+    source = tmp_path / "table.c"
+    source.write_text(
+        "const unsigned char table[] = {" + ",".join(str(b) for b in table) + "};\n"
+        'const char text[] = "_starts with an underscore";\n')
+    darwin, converted, obj = tmp_path / "table.darwin.s", tmp_path / "table.s", tmp_path / "table.o"
+    try:
+        subprocess.run([clang, "--target=arm64_32-apple-watchos", "-O2", "-S", str(source), "-o", str(darwin)],
+                       check=True, capture_output=True)
+    except subprocess.CalledProcessError:
+        pytest.skip("clang has no arm64_32 target")
+    subprocess.run([sys.executable, str(ROOT / "tools" / "android_asm_convert.py"), str(darwin), str(converted)],
+                   check=True)
+    subprocess.run([clang, "--target=aarch64-linux-gnu", "-c", str(converted), "-o", str(obj)], check=True)
+    data = rodata(obj.read_bytes())
+    assert table in data
+    assert b"_starts with an underscore\0" in data