Bug 1791682 - [1/3] Allow comments in thread-checker definition files…

… r=andi

Expand the processor for ThreadAllows.txt and ThreadFileAllows.txt to
also recognize and permit comments prefixed by `#`.

Differential Revision: https://phabricator.services.mozilla.com/D157760

build/clang-plugin/ThreadAllows.py

@@ -2,58 +2,82 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this file,
 # You can obtain one at http://mozilla.org/MPL/2.0/.
 import json
+import os
+import posixpath
 
-FIRST_LINE = "// This file was generated by generate_thread_allows.py. DO NOT EDIT."
+from os import PathLike
 
+# `typing.Literal` not available until Python 3.8;
+# `typing_extensions` not generally available here
+from typing import Iterable, Set
 
-def generate_allows(input_paths):
-    """
-    This script reads in the ThreadAllows.txt and ThreadFileAllows.txt lists
-    and generates a header file containing a two arrays of allowed threads.
-    These can be the following formats:
-        -Files which the checker should ignore
-            These files either contain definitions of NS_NewNamedThread or
-            use args which the plugin can't cast (such as func args).
-        -Thread names which the checker should ignore
-            Specifies which individual thread names to ignore.
-    """
-    file_list = []
-    name_list = []
-    lines = set()
+FIRST_LINE = "// This file was generated by {}. DO NOT EDIT.".format(
+    # `posixpath` for forward slashes, for presentation purposes
+    posixpath.relpath(__file__, os.getenv("TOPSRCDIR", "/"))
+)
+
+
+def generate_allowed_items(
+    which: str,  # should be: Literal["files", "names"],
+    paths: Iterable[PathLike],
+) -> str:
+    def remove_trailing_comment(s: str) -> str:
+        return s[0 : s.find("#")]
 
-    for path in input_paths:
+    def read_items_from_path(path: PathLike) -> Set[str]:
+        out = set()
         with open(path) as file:
-            lines.update(file.readlines())
-
-    for line in sorted(lines):
-        """
-        We are assuming lines ending in .cpp, .h are files. Threads should
-        NOT have names containing filenames. Please don't do that.
-        """
-        line = line.strip()
-        if line.endswith(".cpp") or line.endswith(".h"):
-            file_list.append(line)
-        else:
-            name_list.append(line)
-    file_list_s = ",\n  ".join(json.dumps(elem) for elem in file_list)
-    name_list_s = ",\n  ".join(json.dumps(elem) for elem in name_list)
-    output_string = (
-        FIRST_LINE
-        + """
+            for line in file.readlines():
+                line = remove_trailing_comment(line).strip()
+                if not line:
+                    continue  # comment or empty line; discard
+                out.add(line)
+        return out
+
+    allowed = set().union(*(read_items_from_path(path) for path in paths))
+    # BUG: `json.dumps` may not correctly handle use of the quote character in
+    #   thread names
+    allowed_list_s = ",\n  ".join(json.dumps(elem) for elem in sorted(allowed))
 
-static const char *allow_thread_files[] = {
-  %s
-};
+    return f"""\
+static const char *allow_thread_{which}[] = {{
+  {allowed_list_s}
+}};"""
 
-static const char *allow_thread_names[] = {
-  %s
-};
 
+def generate_allows(
+    *, allowed_names: Iterable[PathLike], allowed_files: Iterable[PathLike]
+) -> str:
     """
-        % (file_list_s, name_list_s)
+    This function reads in the specified sets of files -- ordinarily,
+    ["ThreadAllows.txt"] and ["ThreadFileAllows.txt"] -- and generates the text
+    of a header file containing two arrays with their contents, for inclusion by
+    the thread-name checker.
+
+    The checker will reject the creation of any thread via NS_NewNamedThread
+    unless either:
+      - the thread's name is a literal string which is found in the set of
+        allowed thread names; or
+      - the thread's creation occurs within a file which is found in the set of
+        unchecked files.
+
+    The latter condition exists mostly for the definition of NS_NewNamedThread,
+    but there also exist a few cases where the thread name is dynamically
+    computed (and so can't be checked).
+    """
+    output_string = (
+        FIRST_LINE
+        + "\n\n"
+        + generate_allowed_items("files", allowed_files)
+        + "\n\n"
+        + generate_allowed_items("names", allowed_names)
+        + "\n"
     )
     return output_string
 
 
-def generate_file(output, *input_paths):
-    output.write(generate_allows(input_paths))
+# Entry point used by build/clang-plugin/moz.build (q.v.).
+def generate_file(output, allowed_names, allowed_files):
+    output.write(
+        generate_allows(allowed_names=[allowed_names], allowed_files=[allowed_files])
+    )

build/clang-plugin/ThreadAllows.txt

@@ -1,3 +1,41 @@
+# This file is ingested by `ThreadAllows.py` to produce a list of thread names
+# which our clang plugin will allow to be used with `NS_NewNamedThread`.
+#
+# Permitted thread names are a maximum of 15 characters in length, and must be
+# string literals at their point-of-use in the code -- i.e., in the invocation
+# of `NS_NewNamedThread`.
+#
+# Within this file, each permitted thread name is on a separate line. Comments
+# begin with `#`, as seen here. Leading and trailing whitespace, trailing
+# comments, and blank lines are ignored, and may be used freely.
+#
+# Please explain the addition of any new thread names in comments, preferably
+# with a pointer to a relevant bug. (Do not add thread names only used in tests
+# to this file; instead, add the test file to `ThreadFileAllows.txt`.)
+
+######
+# Gecko/Firefox thread names
+#
+# (None documented yet -- but see "Unsorted thread names", below.)
+
+######
+# Thunderbird-only thread names
+IMAP
+
+######
+# Other
+Checker Test  # used only as part of tests for the thread-checker itself
+
+######
+# Unsorted thread names
+#
+# Thread names below this point are grandfathered in. Please do not add new
+# thread names to this list -- and please remove any that you can, whether by
+# documenting and moving them or by confirming that they are no longer required.
+#
+# In particular, if a thread name is only used for testing, please consider
+# moving its declarator to `ThreadFileAllows.txt`.
+
 ApplyUpdates
 AsyncShutdownPr
 AsyncShutdownWt
@@ -16,7 +54,6 @@ Cameras IPC
 CanvasRenderer
 ChainedPipePump
 ChainedPipeRecv
-Checker Test
 Compositor
 Cookie
 CrashRep Inject
@@ -37,7 +74,6 @@ GeckoProfGTest
 GraphRunner
 HTML5 Parser
 ICS parser
-IMAP
 IPC Launch
 IPCFuzzLoop
 IPDL Background

build/clang-plugin/ThreadFileAllows.txt

@@ -1,3 +1,25 @@
+# This file is ingested by `ThreadAllows.py` to produce a list of files which
+# our clang plugin will allow to use `NS_NewNamedThread`.
+#
+# Note that this file contains a list of _files_, not _paths_.
+
+######
+# Release files
+
+# declaration and definition of `NS_NewNamedThread`
+nsThreadUtils.h
+nsThreadUtils.cpp
+
+# Thread-pools are permitted to make dynamically many threads, using dynamic
+# thread names with explicit numbering.
+nsThreadPool.cpp
+
+######
+# Unsorted files
+#
+# Files below this point are grandfathered in. Please do not add new files to
+# this list -- and please remove any that you can, whether by documenting and
+# moving them or by confirming that they are no longer required.
 ActorsParent.cpp
 DecodePool.cpp
 GeckoChildProcessHost.cpp
@@ -6,6 +28,3 @@ LazyIdleThread.h
 VRThread.cpp
 mozStorageConnection.cpp
 nr_socket_prsock.cpp
-nsThreadPool.cpp
-nsThreadUtils.cpp
-nsThreadUtils.h

build/clang-plugin/import_mozilla_checks.py

@@ -103,7 +103,9 @@ def generate_thread_allows(mozilla_path, module_path):
     names = os.path.join(mozilla_path, "../../build/clang-plugin/ThreadAllows.txt")
     files = os.path.join(mozilla_path, "../../build/clang-plugin/ThreadFileAllows.txt")
     with open(os.path.join(module_path, "ThreadAllows.h"), "w") as f:
-        f.write(ThreadAllows.generate_allows({files, names}))
+        f.write(
+            ThreadAllows.generate_allows(allowed_names=[names], allowed_files=[files])
+        )
 
 
 def do_import(mozilla_path, clang_tidy_path, import_options):