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

[phosek]

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

64

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

64

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

64

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

64

Can templates have default arguments?

[thakis]

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

64

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

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

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

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/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/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/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/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/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 = [ ... ]
 #     ...
 #   }
 
executable(target_name) {
     # 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
+
+    has_custom_main = false
+    if(defined(invoker.has_custom_main)) {
+      has_custom_main = invoker.has_custom_main
+    }
+    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
 }