[gn build] Add build files for LLVM unittests with a custom main() function

[phosek]

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni

53 ↗

(On Diff #179931)

I'm not a big fan of the name, have you considered naming it e.g. gtest?

[thakis]

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni

53 ↗

(On Diff #179931)

unittest_with_custom_gtest? Or just gtest?

The "normal" unittest target is called unittest, and this one is a variant on that, so unittest_foo with some value for foo seems like a good name to me. The defining characteristic of these tests is that they contain a custom main() function, hence the current name. I'm very open for different naming suggestions, but imho this template should be named similarly to the existing unittest template. (Also, unittest is used like 50 times, this one only 3 times, so making it look familiar and self-consistent seems more important to me than making it short.)

Also, D56324 is another, similar patch that adds yet another similar template.

[phosek]

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni

53 ↗

(On Diff #179931)

How about just adding an argument to unittest template, e.g. has_main? It seems like that's the only difference between the two templates so that also avoids the duplication.

[thakis]

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni

53 ↗

(On Diff #179931)

Can templates have default arguments?

[thakis]

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni

53 ↗

(On Diff #179931)

Err sorry, I see what you mean. That's a good suggestion, will do.

[thakis]

way better

[thakis]

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni

49 ↗

(On Diff #180353)

I tried doing if (defined(invoker.has_custom_main) && invoker.has_custom_main) but then GN claimed:

FAILED: build.ninja 
../../../gn/out/gn --root=../.. -q --dotfile=../../llvm/utils/gn/.gn gen .
ERROR at //llvm/unittests/CodeGen/GlobalISel/BUILD.gn:19:21: Assignment had no effect.
  has_custom_main = true
                    ^---
You set the variable "has_custom_main" here and it was unused before it went
out of scope.

[phosek]

LGTM

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni

49 ↗

(On Diff #180353)

The way this is typically done is:

has_custom_main = false # default value
forward_variables_from(invoker, "*") # this is going to override the default value if set

That way you don't need the extra condition on lines 46-48.

[thakis]

Thanks!

llvm/utils/gn/secondary/llvm/utils/unittest/unittest.gni

49 ↗

(On Diff #180353)

Ah, that explains the "assignment had no effect" error: It doesn't complain about the assignment to has_custom_main in the template use (even though that's where the diag points to), but that forward_variables_from forwards that into the local scope here where it isn't used. If I change the forward_variables_from(invoker, "*") to forward_variables_from(invoker, "*", [ "has_custom_main" ]) then what I wanted to do works too.

Here's how each approach looks:

@@ -13,7 +17,7 @@
 template("unittest") {
   executable(target_name) {
     # Foward everything (configs, sources, deps, ...).
-    forward_variables_from(invoker, "*")
+    forward_variables_from(invoker, "*", [ "has_custom_main" ])
     assert(!defined(invoker.output_dir), "cannot set unittest output_dir")
     assert(!defined(invoker.testonly), "cannot set unittest testonly")
 
@@ -37,7 +41,12 @@ template("unittest") {
     # If you change output_dir here, look through
     # `git grep target_out_dir '*/unittests/*'` and update those too.
     output_dir = target_out_dir
-    deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+
+    if (defined(invoker.has_custom_main) && invoker.has_custom_main) {
+      deps += [ "//llvm/utils/unittest:gtest" ]
+    } else {
+      deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+    }

vs

@@ -12,7 +16,9 @@
 
 template("unittest") {
   executable(target_name) {
-    # Foward everything (configs, sources, deps, ...).
+    has_custom_main = false  # Default value.
+
+    # Foward everything (has_custom_main if set; configs, sources, deps, ...).
     forward_variables_from(invoker, "*")
     assert(!defined(invoker.output_dir), "cannot set unittest output_dir")
     assert(!defined(invoker.testonly), "cannot set unittest testonly")
@@ -37,7 +43,12 @@ template("unittest") {
     # If you change output_dir here, look through
     # `git grep target_out_dir '*/unittests/*'` and update those too.
     output_dir = target_out_dir
-    deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+
+    if (has_custom_main) {
+      deps += [ "//llvm/utils/unittest:gtest" ]
+    } else {
+      deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+    }

The first approach requires knowing about forward_variables_from's 3rd arg, but is slightly shorter. I went with the version that you suggested.

llvm/trunk/utils/gn/secondary/llvm/unittests/BUILD.gn

 import("//llvm/lib/Target/targets.gni")
 
 group("unittests") {
   deps = [
     "ADT:ADTTests",
     "Analysis:AnalysisTests",
     "AsmParser:AsmParserTests",
     "BinaryFormat:BinaryFormatTests",
     "Bitcode:BitcodeTests",
     "CodeGen:CodeGenTests",
-
-    # FIXME: Add.
-    #"CodeGen/GlobalISel:GlobalISelTests",
+    "CodeGen/GlobalISel:GlobalISelTests",
     "DebugInfo/CodeView:DebugInfoCodeViewTests",
     "DebugInfo/DWARF:DebugInfoDWARFTests",
     "DebugInfo/MSF:DebugInfoMSFTests",
     "DebugInfo/PDB:DebugInfoPDBTests",
     "Demangle:DemangleTests",
     "ExecutionEngine:ExecutionEngineTests",
     "ExecutionEngine/MCJIT:MCJITTests",
     "ExecutionEngine/Orc:OrcJITTests",
     "FuzzMutate:FuzzMutateTests",
     "IR:IRTests",
     "LineEditor:LineEditorTests",
     "Linker:LinkerTests",
     "MC:MCTests",
-
-    # FIXME: Add.
-    #"MI:MITests",
+    "MI:MITests",
     "Object:ObjectTests",
     "ObjectYAML:ObjectYAMLTests",
     "OptRemarks:OptRemarksTests",
     "Option:OptionTests",
     "Passes:PluginsTests",
     "ProfileData:ProfileDataTests",
     "Support:SupportTests",
     "Support/DynamicLibrary:DynamicLibraryTests",
     "TextAPI:TextAPITests",
     "Transforms/IPO:IPOTests",
     "Transforms/Scalar:ScalarTests",
     "Transforms/Utils:UtilsTests",
     "Transforms/Vectorize:VectorizeTests",
     "XRay:XRayTests",
-
-    # FIXME: Add more:
-    #"tools/llvm-cfi-verify:CFIVerifyTests",
+    "tools/llvm-cfi-verify:CFIVerifyTests",
     "tools/llvm-exegesis:LLVMExegesisTests",
   ]
 
   # Target-dependent unit tests.
   # FIXME: This matches how they are set up in the cmake build,
   # but if we disable an arch after building with it on, this
   # setup leaves behind stale executables.
   # FIXME: Add AArch64, ARM these once the Targets exist.

llvm/trunk/utils/gn/secondary/llvm/unittests/CodeGen/GlobalISel/BUILD.gn

+import("//llvm/utils/unittest/unittest.gni")
+
+unittest("GlobalISelTests") {
+  deps = [
+    "//llvm/lib/CodeGen",
+    "//llvm/lib/CodeGen/GlobalISel",
+    "//llvm/lib/CodeGen/MIRParser",
+    "//llvm/lib/IR",
+    "//llvm/lib/MC",
+    "//llvm/lib/Support",
+    "//llvm/lib/Target",
+    "//llvm/lib/Target:TargetsToBuild",
+  ]
+  sources = [
+    "LegalizerHelperTest.cpp",
+    "LegalizerInfoTest.cpp",
+    "PatternMatchTest.cpp",
+  ]
+  has_custom_main = true
+}

llvm/trunk/utils/gn/secondary/llvm/unittests/MI/BUILD.gn

+import("//llvm/utils/unittest/unittest.gni")
+
+unittest("MITests") {
+  deps = [
+    "//llvm/lib/CodeGen",
+    "//llvm/lib/CodeGen/MIRParser",
+    "//llvm/lib/IR",
+    "//llvm/lib/MC",
+    "//llvm/lib/Support",
+    "//llvm/lib/Target",
+    "//llvm/lib/Target:TargetsToBuild",
+  ]
+  sources = [
+    "LiveIntervalTest.cpp",
+  ]
+  has_custom_main = true
+}

llvm/trunk/utils/gn/secondary/llvm/unittests/tools/llvm-cfi-verify/BUILD.gn

+import("//llvm/utils/unittest/unittest.gni")
+
+unittest("CFIVerifyTests") {
+  deps = [
+    "//llvm/lib/DebugInfo/Symbolize",
+    "//llvm/lib/MC",
+    "//llvm/lib/MC/MCParser",
+    "//llvm/lib/Object",
+    "//llvm/lib/Support",
+    "//llvm/lib/Target:AllTargetsAsmParsers",
+    "//llvm/lib/Target:AllTargetsAsmPrinters",
+    "//llvm/lib/Target:AllTargetsDescs",
+    "//llvm/lib/Target:AllTargetsDisassemblers",
+    "//llvm/lib/Target:AllTargetsInfos",
+    "//llvm/tools/llvm-cfi-verify/lib",
+  ]
+  sources = [
+    "FileAnalysis.cpp",
+    "GraphBuilder.cpp",
+  ]
+  has_custom_main = true
+}

llvm/trunk/utils/gn/secondary/llvm/utils/unittest/unittest.gni

 # This file defines a template for adding a unittest binary.
 #
 # It's a thin wrapper around GN's built-in executable() target type and
-# accepts the same parameters.
+# accepts the same parameters, and in addition this paramater:
+#
+#   has_custom_main (optional)
+#       [bool] If set, link against gtest instead of UnitTestMain; for tests
+#              that define their own main() function.
 #
 # Example use:
 #
 #   unittest("FormatTest") {
 #     sources = [ ... ]
 #     ...
 #   }
 
 template("unittest") {
   executable(target_name) {
-    # Foward everything (configs, sources, deps, ...).
+    has_custom_main = false  # Default value.
+
+    # Foward everything (has_custom_main if set; configs, sources, deps, ...).
     forward_variables_from(invoker, "*")
     assert(!defined(invoker.output_dir), "cannot set unittest output_dir")
     assert(!defined(invoker.testonly), "cannot set unittest testonly")
 
     # Common settings for all unit tests.
     # Unit test binaries shouldn't go right in out/gn/bin, for two reasons:
     # 1. That's where production binaries go.
     # 2. The CMake build doesn't put the unit tests of all projects (clang,
     #    lld,...) in one directory, so it's not guaranteed that there won't
     #    be name collisions between test binaries from separate projects.
     # Each lit suite takes an foo_obj_root parameter and puts temporary files
     # for lit tests at foo_obj_root/test and looks for unit test binaries
     # below foo_obj_root/unittests. As long as the BUILD.gn files processing
     # the lit.site.cfg.py.in files match the output dir here, it doesn't
     # matter all that much where the unit test binaries go, with the weak
     # constraints that test binaries of different projects should go in
     # different folders, and that it's not too difficult to manually
     # run the unit test binary if necessary. Using target_out_dir here
     # means that //clang/unittests/Format gets its binary in
     # out/gn/obj/clang/unittests/Format/FormatTests, which seems fine.
     #
     # If you change output_dir here, look through
     # `git grep target_out_dir '*/unittests/*'` and update those too.
     output_dir = target_out_dir
+
+    if (has_custom_main) {
+      deps += [ "//llvm/utils/unittest:gtest" ]
+    } else {
       deps += [ "//llvm/utils/unittest/UnitTestMain" ]
+    }
     testonly = true
   }
 }
 
 set_defaults("unittest") {
   configs = shared_binary_target_configs
 }