Merge pull request #9150 from gdols/ci/alignas-limit

ci(conventions): reject alignas above the 8192 bytes MinGW allows

CMakeLists.txt

@@ -264,6 +264,8 @@ if(BUILD_TESTING)
             ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_check_conventions_blob_reads.py)
         add_test(NAME conventions_agent_files COMMAND ${Python3_EXECUTABLE}
             ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_check_conventions_agent_files.py)
+        add_test(NAME conventions_alignment COMMAND ${Python3_EXECUTABLE}
+            ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_check_conventions_alignment.py)
         add_test(NAME conventions_pr_template COMMAND ${Python3_EXECUTABLE}
             ${CMAKE_CURRENT_SOURCE_DIR}/tools/tests/test_check_conventions_pr_template.py)
         add_test(NAME check_compile COMMAND ${Python3_EXECUTABLE}

CONTRIBUTING.md

@@ -12,6 +12,7 @@
 - Implement the general behaviour of a function, not what one title happens to need.
 - Don't add replacements for modules that titles ship themselves in `sce_module/`, `sce_modules/` or `prx/`: engine or middleware modules (Cohtml, FMOD, GOG Galaxy) and SDK libraries that only wrap other system libraries. The relinker converts and loads the title's own module, and a host library with the same name is left out of `DT_NEEDED`. Only system libraries, which reach the kernel or the hardware, are reimplemented in [core/libs/prx](core/libs/prx). A host copy may still exist for a wrapped library that some dumps lack entirely, like libSceNpCppWebApi: the title's own module wins whenever one is shipped, so it is only ever consulted by titles that ship nothing.
 - Avoid non-standard extensions (`__attribute__`, etc.) where standard C++ is enough. Helper symbols that must not become NIDs use the `_nid_no_patch` or `_nid_no_patch_cut` suffix.
+- `alignas` takes at most 8192: MinGW GCC, the Windows compiler, rejects static objects aligned above it. A larger block is aligned at run time inside a bigger buffer.
 - The relinker uses only the C++20 standard library.
 - Third-party code is added as a submodule under `3rdparty/` and built from source, not found on the system. With `ANYPS5_USE_SYSTEM_DEPS=ON` the packages packaged by the distributions (SDL2, freetype, Vulkan-Headers, SPIRV-Headers, glslang and SPIRV-Tools) come from the system instead; that resolution lives in [cmake/SystemDeps.cmake](cmake/SystemDeps.cmake) and the default build never reads the system.
 - Don't add tests that only check that a symbol is exported: a missing export already fails at startup.

tools/check_conventions.py

@@ -1,5 +1,6 @@
 import argparse
 import io
+import itertools
 import json
 import os
 import posixpath
@@ -24,6 +25,7 @@ RULES = {
     "system-dependency": ("error", "CONTRIBUTING.md#code", "Third-party code is a submodule under 3rdparty/ built from source, not found on the system"),
     "title-specific": ("error", "CONTRIBUTING.md#code", "Implement the general behaviour, not what one title needs; title-specific code belongs in its sce_module"),
     "extension": ("error", "CONTRIBUTING.md#code", "Avoid non-standard extensions where standard C++ is enough"),
+    "alignment": ("error", "CONTRIBUTING.md#code", "MinGW GCC rejects static objects aligned above 8192 bytes; align a larger block at run time inside a bigger buffer"),
     "notes-file": ("error", "CONTRIBUTING.md#branches-and-pull-requests", "Notes, investigation and agent files go in the pull request, not in the repository; only the root AGENTS.md is allowed"),
     "binary": ("error", "docs/dev/CONVENTIONS.md", "Only UTF-8 text files in the repository; images go in the gist comments"),
     "ci-gate": ("error", "CONTRIBUTING.md#build-and-test", "A job that runs for pull requests needs a file filter: paths on the pull_request trigger, or needs eligibility and an if on its outputs"),
@@ -59,6 +61,12 @@ STUB_BODY = re.compile(r"^(?:\(void\)\s*\w+\s*;|static_cast<void>\(\s*\w+\s*\)\s
 SYSTEM_DEPENDENCY = re.compile(r"\b(find_package\s*\((?!\s*(Python3|Threads|Git)\b)|pkg_check_modules\b|pkg_search_module\b|find_(library|path|program|file)\b)", re.I)
 TITLE = re.compile(r"\b(PPSA|CUSA)\d{5}\b")
 EXTENSION = re.compile(r"__attribute__|__declspec|#\s*pragma\s+(?!once\b)")
+INTEGER = r"(0[xX][0-9a-fA-F']+|0[bB][01']+|[0-9][0-9']*)[uUlLzZ]*"
+ALIGNAS = re.compile(rf"\balignas\s*\(\s*(?:{INTEGER}|([A-Za-z_][A-Za-z0-9_]*))\s*\)")
+CONSTANT = re.compile(rf"\bconstexpr\b[^;=]*?\b([A-Za-z_][A-Za-z0-9_]*)\s*(?:=\s*|\{{\s*){INTEGER}\s*[;}}]")
+NAMESPACE = re.compile(r"\bnamespace\s+((?:[A-Za-z_][A-Za-z0-9_]*\s*::\s*(?:inline\s+)?)*[A-Za-z_][A-Za-z0-9_]*)\s*$|\bnamespace\s*$")
+LINKAGE = re.compile(r'\bextern\s*""\s*$')
+ALIGNMENT_LIMIT = 8192
 LINK = re.compile(r"\[[^\]]+\]\(([^)\s]+)")
 FENCE = re.compile(r"^ {0,3}(```|~~~).*?^ {0,3}\1[ \t]*\r?$|^ {0,3}(?:```|~~~).*\Z", re.S | re.M)
 INLINE = re.compile(r"`[^`\n]*`")
@@ -172,12 +180,20 @@ class Check:
             source = self.show(path).split("\n")
             masked = cpp_text("\n".join(source), preserve_comments=True).split("\n")
             assembly = asm_exports("\n".join(source)) if core and path.startswith("core/libs/prx/") else {}
+            constants, starts = None, None
             for number, text in lines:
                 bare = masked[number - 1]
                 if ("//" in bare or "/*" in bare) and not ALLOWED_COMMENT.match(bare):
                     self.report("comment", path, number)
                 if EXTENSION.search(bare):
                     self.report("extension", path, number, EXTENSION.search(bare).group(0))
+                if "alignas" in bare:
+                    if constants is None:
+                        constants = named_constants(cpp_text("\n".join(source)))
+                        starts = list(itertools.accumulate((len(line) + 1 for line in masked), initial=0))
+                    alignment = over_aligned(bare, constants, starts[number - 1])
+                    if alignment:
+                        self.report("alignment", path, number, alignment)
                 if core and TITLE.search(text):
                     self.report("title-specific", path, number, TITLE.search(text).group(0))
                 if core and path.startswith("core/libs/prx/"):
@@ -580,6 +596,68 @@ def cpp_text(text, preserve_comments=False):
     return CPP_TOKEN.sub(mask, text)
 
 
+def integer_literal(text):
+    digits = text.replace("'", "").lower()
+    if digits.startswith(("0x", "0b")):
+        return int(digits, 0)
+    return int(digits, 8 if len(digits) > 1 and digits[0] == "0" else 10)
+
+
+def brace_scopes(text):
+    stack, scopes, boundary = [], [], -1
+    for index, character in enumerate(text):
+        if character not in "{};":
+            continue
+        if character == "{":
+            head = text[boundary + 1:index]
+            outer = next((key[1] for _, key in reversed(stack) if key and key[0] == "namespace"), "")
+            match = NAMESPACE.search(head)
+            if match and re.search(r"\binline\s+namespace\b", head):
+                key = ("namespace", outer)
+            elif match:
+                key = ("namespace", outer + "::" + re.sub(r"\binline\b|\s+", "", match.group(1) or "{anonymous}"))
+            else:
+                key = None if LINKAGE.search(head) else ("block", index)
+            stack.append((index, key))
+        elif character == "}" and stack:
+            start, key = stack.pop()
+            scopes.append((start, index, key))
+        boundary = index
+    return scopes
+
+
+def scope_chain(scopes, position):
+    chain = []
+    for _, _, key in sorted((scope for scope in scopes if scope[0] < position < scope[1]), reverse=True):
+        if key and key[0] == "namespace":
+            parts = key[1].split("::")
+            chain.extend(prefix for prefix in (("namespace", "::".join(parts[:count])) for count in range(len(parts), 1, -1)) if prefix not in chain)
+        elif key:
+            chain.append(key)
+    return chain + [("namespace", "")]
+
+
+def named_constants(text):
+    scopes = brace_scopes(text)
+    definitions = [(match.group(1), integer_literal(match.group(2)), match.start(), scope_chain(scopes, match.start())[0]) for match in CONSTANT.finditer(text)]
+    return definitions, scopes
+
+
+def constant_value(constants, name, position):
+    definitions, scopes = constants
+    chain = scope_chain(scopes, position)
+    visible = [(-chain.index(scope), start, value) for found, value, start, scope in definitions if found == name and start < position and scope in chain]
+    return max(visible)[2] if visible else 0
+
+
+def over_aligned(text, constants=((), ()), offset=0):
+    for match in ALIGNAS.finditer(text):
+        value = integer_literal(match.group(1)) if match.group(1) else constant_value(constants, match.group(2), offset + match.start())
+        if value > ALIGNMENT_LIMIT:
+            return match.group(0)
+    return None
+
+
 def function_body(lines, start):
     text = cpp_text("\n".join(lines[start:start + 400]))
     brace, semicolon = text.find("{"), text.find(";")

tools/tests/test_check_conventions_alignment.py

@@ -0,0 +1,153 @@
+import itertools
+import os
+import subprocess
+import sys
+import tempfile
+import unittest
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
+from check_conventions import Check, cpp_text, named_constants, over_aligned
+
+SOURCE = "core/libs/prx/libSceExample/tests/Example.cpp"
+
+SMALL = "void Small() {\n    constexpr std::size_t Alignment = 64;\n    alignas(Alignment) static char small[4];\n}\n"
+LARGE = "void Large() {\n    constexpr std::size_t Alignment = 16384;\n    alignas(Alignment) static char large[4];\n}\n"
+
+
+def reported(text):
+    constants = named_constants(cpp_text(text))
+    lines = text.split("\n")
+    starts = list(itertools.accumulate((len(line) + 1 for line in lines), initial=0))
+    return [number for number, line in enumerate(lines, 1) if over_aligned(line, constants, starts[number - 1])]
+
+
+class OverAlignedTests(unittest.TestCase):
+    def test_alignments_above_the_limit(self):
+        for line, match in (
+            ("alignas(65536) std::array<std::uint8_t, 4u << 20u> Texels{};", "alignas(65536)"),
+            ("alignas(16384) static char block[16];", "alignas(16384)"),
+            ("alignas( 0x10000 ) char block[16];", "alignas( 0x10000 )"),
+            ("alignas(0X4000u) char block[16];", "alignas(0X4000u)"),
+            ("alignas(65'536) char block[16];", "alignas(65'536)"),
+            ("alignas(040000) char block[16];", "alignas(040000)"),
+            ("alignas(0b100000000000000) char block[16];", "alignas(0b100000000000000)"),
+            ("alignas(256) char small[4]; alignas(32768) char large[4];", "alignas(32768)"),
+        ):
+            with self.subTest(line=line):
+                self.assertEqual(over_aligned(line), match)
+
+    def test_alignments_within_the_limit(self):
+        for line in (
+            "alignas(8192) char block[16];",
+            "alignas(0x2000) char block[16];",
+            "alignas(256) std::array<std::uint32_t, 128> Input{};",
+            "alignas(4096) char page[4096];",
+            "alignas(std::max_align_t) char storage[16];",
+            "alignas(Shader) char storage[sizeof(Shader)];",
+            "realignas(65536)",
+            "constexpr std::size_t TexelAlignment = 65536;",
+        ):
+            with self.subTest(line=line):
+                self.assertIsNone(over_aligned(line))
+
+    def test_named_constants_of_the_file(self):
+        self.assertEqual(reported("constexpr std::size_t BlockBytes = 65536;\n"
+                                  "static constexpr std::uint32_t PageBytes{0x1000u};\n"
+                                  "constexpr auto Large = 0x10000;\n"
+                                  "constexpr std::size_t Texels = 4u << 20u;\n"
+                                  "alignas(BlockBytes) std::array<std::uint8_t, BlockBytes> Block{};\n"
+                                  "alignas( Large ) char block[16];\n"
+                                  "alignas(PageBytes) char page[4096];\n"
+                                  "alignas(Texels) char texels[16];\n"
+                                  "alignas(inotify_event) char buffer[16384];\n"), [5, 6])
+        self.assertEqual(reported("alignas(BlockBytes) char block[16];\n"
+                                  "constexpr std::size_t BlockBytes = 65536;\n"), [])
+
+    def test_constants_in_separate_scopes(self):
+        self.assertEqual(reported(SMALL + LARGE), [7])
+        self.assertEqual(reported(LARGE + SMALL), [3])
+
+    def test_constants_in_nested_scopes(self):
+        for outer, inner, expected in ((64, 16384, [6]), (16384, 64, [3, 8])):
+            text = ("void Nested() {\n"
+                    f"    constexpr std::size_t Alignment = {outer};\n"
+                    "    alignas(Alignment) static char before[4];\n"
+                    "    {\n"
+                    f"        constexpr std::size_t Alignment = {inner};\n"
+                    "        alignas(Alignment) static char inside[4];\n"
+                    "    }\n"
+                    "    alignas(Alignment) static char after[4];\n"
+                    "}\n")
+            with self.subTest(outer=outer, inner=inner):
+                self.assertEqual(reported(text), expected)
+
+    def test_constants_in_reopened_namespaces(self):
+        large = "constexpr std::size_t Alignment = 16384;"
+        use = "alignas(Alignment) static char data[4];"
+        for text, expected in (
+            (f"namespace N {{ {large} }}\nnamespace N {{ {use} }}\n", [2]),
+            (f"namespace N {{ {large} }}\nnamespace M {{ {use} }}\n", []),
+            (f"namespace {{ {large} }}\nnamespace {{ {use} }}\n", [2]),
+            (f"namespace A::B {{ {large} }}\nnamespace A {{ namespace B {{ {use} }} }}\n", [2]),
+            (f"namespace A {{ {large} }}\nnamespace A::B {{ {use} }}\n", [2]),
+            (f"namespace A {{ inline namespace V1 {{ {large} }} }}\nnamespace A {{ {use} }}\n", [2]),
+            (f"namespace A {{ inline namespace V1 {{ {large} }} }}\nnamespace B {{ {use} }}\n", []),
+            (f"namespace N {{ {large} }}\nnamespace N {{ void F() {{ constexpr std::size_t Alignment = 64; {use} }} }}\n", []),
+            (f'extern "C" {{ {large} }}\n{use}\n', [2]),
+        ):
+            with self.subTest(text=text):
+                self.assertEqual(reported(text), expected)
+
+
+class AlignmentRuleTests(unittest.TestCase):
+    def findings(self, after):
+        with tempfile.TemporaryDirectory() as directory:
+            root = Path(directory)
+
+            def git(*args):
+                return subprocess.run(["git", "-c", f"safe.directory={root}", "-c", "user.name=Conventions Test",
+                                       "-c", "user.email=test@example.com", "-c", "commit.gpgsign=false", *args],
+                                      cwd=root, capture_output=True, check=True, encoding="utf-8").stdout
+
+            source = root / SOURCE
+            source.parent.mkdir(parents=True)
+            source.write_text("alignas(65536) char existing[16];\n", encoding="utf-8")
+            git("init", "-q")
+            git("add", ".")
+            git("commit", "-qm", "test: add fixture")
+            source.write_text("alignas(65536) char existing[16];\n" + after, encoding="utf-8")
+            git("commit", "-qam", "test: add over-aligned blocks")
+            original = Path.cwd()
+            try:
+                os.chdir(root)
+                check = Check("HEAD~1", "HEAD")
+                check.code()
+            finally:
+                os.chdir(original)
+            return check.findings
+
+    def test_only_added_code_is_reported(self):
+        self.assertEqual(self.findings("alignas(256) char small[16];\n"
+                                       "const char* text = \"alignas(65536)\";\n"
+                                       "alignas(0x10000) char added[16];\n"
+                                       "constexpr std::size_t BlockBytes = 65536;\n"
+                                       "alignas(BlockBytes) std::array<std::uint8_t, BlockBytes> Block{};\n"),
+                         [("alignment", SOURCE, 4, "alignas(0x10000)"), ("alignment", SOURCE, 6, "alignas(BlockBytes)")])
+
+    def test_constants_are_resolved_in_their_scope(self):
+        self.assertEqual(self.findings("namespace First {\n" + SMALL + LARGE + "}\nnamespace Second {\n" + LARGE + SMALL + "}\n"),
+                         [("alignment", SOURCE, 9, "alignas(Alignment)"), ("alignment", SOURCE, 15, "alignas(Alignment)")])
+
+    def test_reopened_namespace_keeps_its_constants(self):
+        self.assertEqual(self.findings("namespace N {\n"
+                                       "    constexpr std::size_t Alignment = 16384;\n"
+                                       "}\n"
+                                       "namespace N {\n"
+                                       "    alignas(Alignment) static char data[4];\n"
+                                       "}\n"),
+                         [("alignment", SOURCE, 6, "alignas(Alignment)")])
+
+
+if __name__ == "__main__":
+    unittest.main()