[lit][NFC] Refactor use_clang into two functions

[boomanaiden154]

This patch refactors use_clang into two functions. This is intended for
use within the clang-tools-extra test suite to avoid a race condition
where the clang binary exists but is not yet ready for execution which
results in a lit configuration failure.

Details are in #145703.

[llvmbot]

@llvm/pr-subscribers-testing-tools

Author: Aiden Grossman (boomanaiden154)

Changes

This patch refactors use_clang into two functions. This is intended for
use within the clang-tools-extra test suite to avoid a race condition
where the clang binary exists but is not yet ready for execution which
results in a lit configuration failure.

Details are in #145703.

---

Full diff: https://github.com/llvm/llvm-project/pull/147436.diff

1 Files Affected:

• (modified) llvm/utils/lit/lit/llvm/config.py (+98-80)

diff --git a/llvm/utils/lit/lit/llvm/config.py b/llvm/utils/lit/lit/llvm/config.py
index 5459d431d696b..649636d4bcf4c 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -520,19 +520,30 @@ def use_llvm_tool(
                 self.lit_config.note("using {}: {}".format(name, tool))
         return tool
 
-    def use_clang(
+    def _get_clang_paths(self, additional_tool_dirs):
+        # Put Clang first to avoid LLVM from overriding out-of-tree clang
+        # builds.
+        exe_dir_props = [
+            self.config.name.lower() + "_tools_dir",
+            "clang_tools_dir",
+            "llvm_tools_dir",
+        ]
+        paths = [
+            getattr(self.config, pp)
+            for pp in exe_dir_props
+            if getattr(self.config, pp, None)
+        ]
+        paths = additional_tool_dirs + paths
+        return paths
+
+    def clang_setup(
         self,
         additional_tool_dirs=[],
-        additional_flags=[],
-        required=True,
-        use_installed=False,
     ):
-        """Configure the test suite to be able to invoke clang.
-
-        Sets up some environment variables important to clang, locates a
-        just-built or optionally an installed clang, and add a set of standard
-        substitutions useful to any test suite that makes use of clang.
+        """Perform the setup needed to be able to invoke clang.
 
+        This function performs all the necessary setup to execute clang (or
+        tooling based on clang) but does not actually add clang as a tool.
         """
         # Clear some environment variables that might affect Clang.
         #
@@ -573,20 +584,9 @@ def use_clang(
         self.clear_environment(possibly_dangerous_env_vars)
 
         # Tweak the PATH to include the tools dir and the scripts dir.
-        # Put Clang first to avoid LLVM from overriding out-of-tree clang
-        # builds.
-        exe_dir_props = [
-            self.config.name.lower() + "_tools_dir",
-            "clang_tools_dir",
-            "llvm_tools_dir",
-        ]
-        paths = [
-            getattr(self.config, pp)
-            for pp in exe_dir_props
-            if getattr(self.config, pp, None)
-        ]
-        paths = additional_tool_dirs + paths
-        self.with_environment("PATH", paths, append_path=True)
+        self.with_environment(
+            "PATH", self._get_clang_paths(additional_tool_dirs), append_path=True
+        )
 
         lib_dir_props = [
             self.config.name.lower() + "_libs_dir",
@@ -611,63 +611,6 @@ def use_clang(
         if pext:
             self.config.substitutions.append(("%pluginext", pext))
 
-        # Discover the 'clang' and 'clangcc' to use.
-        self.config.clang = self.use_llvm_tool(
-            "clang",
-            search_env="CLANG",
-            required=required,
-            search_paths=paths,
-            use_installed=use_installed,
-        )
-        if self.config.clang:
-            self.config.available_features.add("clang")
-            builtin_include_dir = self.get_clang_builtin_include_dir(self.config.clang)
-            tool_substitutions = [
-                ToolSubst(
-                    "%clang", command=self.config.clang, extra_args=additional_flags
-                ),
-                ToolSubst(
-                    "%clang_analyze_cc1",
-                    command="%clang_cc1",
-                    # -setup-static-analyzer ensures that __clang_analyzer__ is defined
-                    extra_args=["-analyze", "-setup-static-analyzer"]
-                    + additional_flags,
-                ),
-                ToolSubst(
-                    "%clang_cc1",
-                    command=self.config.clang,
-                    extra_args=[
-                        "-cc1",
-                        "-internal-isystem",
-                        builtin_include_dir,
-                        "-nostdsysteminc",
-                    ]
-                    + additional_flags,
-                ),
-                ToolSubst(
-                    "%clang_cpp",
-                    command=self.config.clang,
-                    extra_args=["--driver-mode=cpp"] + additional_flags,
-                ),
-                ToolSubst(
-                    "%clang_cl",
-                    command=self.config.clang,
-                    extra_args=["--driver-mode=cl"] + additional_flags,
-                ),
-                ToolSubst(
-                    "%clang_dxc",
-                    command=self.config.clang,
-                    extra_args=["--driver-mode=dxc"] + additional_flags,
-                ),
-                ToolSubst(
-                    "%clangxx",
-                    command=self.config.clang,
-                    extra_args=["--driver-mode=g++"] + additional_flags,
-                ),
-            ]
-            self.add_tool_substitutions(tool_substitutions)
-            self.config.substitutions.append(("%resource_dir", builtin_include_dir))
-
         # There will be no default target triple if one was not specifically
         # set, and the host's architecture is not an enabled target.
         if self.config.target_triple:
@@ -729,6 +672,81 @@ def add_std_cxx(s):
         add_std_cxx("%std_cxx20-")
         add_std_cxx("%std_cxx23-")
 
+    def use_clang(
+        self,
+        additional_tool_dirs=[],
+        additional_flags=[],
+        required=True,
+        use_installed=False,
+    ):
+        """Configure the test suite to be able to invoke clang.
+
+        Sets up some environment variables important to clang, locates a
+        just-built or optionally an installed clang, and add a set of standard
+        substitutions useful to any test suite that makes use of clang.
+
+        """
+        self.clang_setup(additional_tool_dirs)
+
+        paths = self._get_clang_paths(additional_tool_dirs)
+
+        # Discover the 'clang' and 'clangcc' to use.
+        self.config.clang = self.use_llvm_tool(
+            "clang",
+            search_env="CLANG",
+            required=required,
+            search_paths=paths,
+            use_installed=use_installed,
+        )
+        if self.config.clang:
+            self.config.available_features.add("clang")
+            builtin_include_dir = self.get_clang_builtin_include_dir(self.config.clang)
+            tool_substitutions = [
+                ToolSubst(
+                    "%clang", command=self.config.clang, extra_args=additional_flags
+                ),
+                ToolSubst(
+                    "%clang_analyze_cc1",
+                    command="%clang_cc1",
+                    # -setup-static-analyzer ensures that __clang_analyzer__ is defined
+                    extra_args=["-analyze", "-setup-static-analyzer"]
+                    + additional_flags,
+                ),
+                ToolSubst(
+                    "%clang_cc1",
+                    command=self.config.clang,
+                    extra_args=[
+                        "-cc1",
+                        "-internal-isystem",
+                        builtin_include_dir,
+                        "-nostdsysteminc",
+                    ]
+                    + additional_flags,
+                ),
+                ToolSubst(
+                    "%clang_cpp",
+                    command=self.config.clang,
+                    extra_args=["--driver-mode=cpp"] + additional_flags,
+                ),
+                ToolSubst(
+                    "%clang_cl",
+                    command=self.config.clang,
+                    extra_args=["--driver-mode=cl"] + additional_flags,
+                ),
+                ToolSubst(
+                    "%clang_dxc",
+                    command=self.config.clang,
+                    extra_args=["--driver-mode=dxc"] + additional_flags,
+                ),
+                ToolSubst(
+                    "%clangxx",
+                    command=self.config.clang,
+                    extra_args=["--driver-mode=g++"] + additional_flags,
+                ),
+            ]
+            self.add_tool_substitutions(tool_substitutions)
+            self.config.substitutions.append(("%resource_dir", builtin_include_dir))
+
         # FIXME: Find nicer way to prohibit this.
         def prefer(this, to):
             return '''\"*** Do not use '%s' in tests, use '%s'. ***\"''' % (to, this)

[pogo59]

Could you do a stacked patch to show how this helps clang-tools-extra? Presumably you've tried it to verify the fix manually.

[boomanaiden154]

Could you do a stacked patch to show how this helps clang-tools-extra? Presumably you've tried it to verify the fix manually.

It's #147437. I put it up but forgot to link the two explicitly in the comments.

[pogo59]

NFC refactor LGTM