Make CMake choose the test exe target architecture according to the build.

[zturner]

Summary

(I'm using chandlerc and rnk mostly for the CMake stuff, but naturally they are unfamiliar with the dotest stuff, so that's fair game if anyone else has any comments or concerns)

Previously, CMake was invoking the test runner and not specifying what architecture to use when building test executables. The Makefiles for the test executables then had logic to choose x64 by default. This doesn't work on Windows because the test compiler would then try to link against the 64-bit MSVCRT and not find them since only the 32-bit MSVCRT was in the path.

This patch addresses this by figuring out, at CMake time, whether or not you are building LLDB with a 64 or 32-bit toolchain. Then, it explicitly passes this value to the test runner, causing the test runner to build tests whose architecture matches that of LLDB itself. This can still be overridden by setting the CMake variable LLDB_TEST_EXECUTABLE_ARCH=(x64|x86)

[chandlerc]

CMakeLists.txt

22–26 ↗

(On Diff #12047)

So, this isn't the "native" architecture. For an LLVM cross compiler, it will not execute on the local machine at all. It's just the target architecture.

I don't think you want to have an 'LLDB_TARGET_ARCH' variable. That doesn't really make sense, especially in the singular.

I think you should just set this in the test-specific cmake, and I think you should make it 'LLDB_TEST_TRIPLE' which is cached, user-overridable, and defaults to 'LLVM_DEFAULT_TARGET_TRIPLE'.

test/CMakeLists.txt

20

Why isn't this just part of the COMMON_TEST_ARGS below?

22–23

If this is overridable then it should be cached with a descriptive string...

45

... and we shouldn't clobber it.

[zturner]

CMakeLists.txt

22–26 ↗

(On Diff #12047)

LLDB doesn't support cross-compilation anyway, so I think it's safe to assume that the host and target are the same.

All we're trying to figure out, ultimately, is whether to pass -m32 or -m64 to clang. So allowing the user to specify a full triple seems like overkill and is likely to be confusing, it seems more straightforward if they just pass in "x86" or "x64".

[zturner]

Fixed the variable caching and other cleanup

[zturner]

Trying again, the correct patch didn't get attached last time.

[rnk]

Seems fine to me, but I'll let Chandler respond.

CMakeLists.txt

22–26 ↗

(On Diff #12047)

Just doing the architecture seems reasonable for now. The triple could be useful if we ever want to support running the test suite in a way that is remotely debugging binaries in a VM with a different OS or something. That's probably either not useful or a long way off.

test/CMakeLists.txt

22

nit: s/STRING/string/, we appear to prefer lower-case cmake function names.

[zturner]

Fix some issues with overriding one of the CMake variables, and other minor cleanup.

[chandlerc]

Minor comment tweak. Otherwise this looks good now. Thanks.

I guess Greg wants you to wait for Todd's LGTM? :: shruug :: I know nothing about LLDB, just cmake. =]

test/CMakeLists.txt

18–19

This comment isn't quite accurate... Maybe:

"The default architecture with which to compile test executables is the default LLVM target architecture, which itself defaults to the host architecture."

[tfiala]

I'll have a peek when I get to a computer (on vaca...)

[zturner]

This was already submitted

test/CMakeLists.txt

add_custom_target(${name}
     COMMENT "${comment}"
     )
 endfunction()
 
 if ("${LLDB_TEST_COMPILER}" STREQUAL "")
     string(REGEX REPLACE ".*ccache\ +" "" LLDB_TEST_COMPILER ${CMAKE_C_COMPILER} ${CMAKE_C_COMPILER_ARG1})
 endif()
 
-# Users can override LLDB_TEST_ARGS to modify the way LLDB tests are run. See help below.
-set(LLDB_TEST_ARGS
-  -C
-  ${LLDB_TEST_COMPILER}
-  CACHE STRING "Specify compiler(s) and architecture(s) with which run LLDB tests. For example: '-C gcc -C clang -A i386 -A x86_64'"
-  )
-string(REPLACE " " ";" LLDB_TEST_ARGS ${LLDB_TEST_ARGS})
+# The default architecture with which to compile test executables is the architecture that LLDB
+# itself was built with.
+string(TOLOWER "${LLVM_TARGET_ARCH}" LLDB_DEFAULT_TEST_ARCH)
+if( LLDB_DEFAULT_TEST_ARCH STREQUAL "host" )
+  string(REGEX MATCH "^[^-]*" LLDB_DEFAULT_TEST_ARCH ${LLVM_HOST_TRIPLE})
+endif ()
 
-set(LLDB_TRACE_DIR "${CMAKE_BINARY_DIR}/lldb-test-traces"
-  CACHE STRING "Set directory to output LLDB test traces (for tests that do not pass.)"
-  )
+# Allow the user to override the default by setting LLDB_TEST_ARCH
+set(LLDB_TEST_ARCH
+  ${LLDB_DEFAULT_TEST_ARCH}
+  CACHED STRING "Specify the architecture to run LLDB tests as (x86|x64).  Determines whether tests are compiled with -m32 or -m64")
+
+# Users can override LLDB_TEST_USER_ARGS to specify arbitrary arguments to pass to the script
+set(LLDB_TEST_USER_ARGS
+  -C ${LLDB_TEST_COMPILER}
+  CACHE STRING "Specify additional arguments to pass to test runner. For example: '-C gcc -C clang -A i386 -A x86_64'")
 
-set(LLDB_COMMON_TEST_ARGS
+set(LLDB_TEST_COMMON_ARGS
+  --arch=${LLDB_TEST_ARCH}
   --executable
   ${CMAKE_BINARY_DIR}/bin/lldb${CMAKE_EXECUTABLE_SUFFIX}
   -s
-  ${LLDB_TRACE_DIR}
+  ${CMAKE_BINARY_DIR}/lldb-test-traces
   -u CXXFLAGS
   -u CFLAGS
   )
 
 add_python_test_target(check-lldb-single
   ${LLDB_SOURCE_DIR}/test/dotest.py
-  "${LLDB_COMMON_TEST_ARGS};${LLDB_TEST_ARGS}"
-  "Testing LLDB with args: ${LLDB_COMMON_TEST_ARGS};${LLDB_TEST_ARGS}"
+  "${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}"
+  "Testing LLDB with args: ${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}"
   )
 
-set(LLDB_DOSEP_ARGS
-  -o;\"-q;${LLDB_COMMON_TEST_ARGS};${LLDB_TEST_ARGS}\"
-  )
+set(LLDB_DOSEP_ARGS -o;\"-q;${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}\")
 
 # If tests crash cause LLDB to crash, or things are otherwise unstable, or if machine-parsable
 # output is desired (i.e. in continuous integration contexts) check-lldb-sep is a better target.
 add_python_test_target(check-lldb
   ${LLDB_SOURCE_DIR}/test/dosep.py
   "${LLDB_DOSEP_ARGS}"
   "Testing LLDB (with a separate subprocess per test)"
   )

test/dotest.py

else:
         compilers = ['clang']
 
     if args.archs:
         archs = args.archs
         for arch in archs:
             if arch.startswith('arm') and platform_system == 'Darwin':
                 os.environ['SDKROOT'] = commands.getoutput('xcodebuild -version -sdk iphoneos.internal Path')
     else:
-        if (platform_system == 'Darwin' or (platform_system == 'Linux' and compilers == ['clang'])) and platform_machine == 'x86_64':
-            archs = ['x86_64', 'i386']
-        else:
-            archs = [platform_machine]
 
     if args.categoriesList:
         categoriesList = set(validate_categories(args.categoriesList))
         useCategories = True
     else:
         categoriesList = []
 
     if args.skipCategories:

test/make/Makefile.rules

ifeq "$(OS)" "Darwin"
   DSFLAGS =
   DSYM = $(EXE).dSYM
   AR := libtool
   ARFLAGS := -static -o
 else
   # On non-Apple platforms, -arch becomes -m
   ARCHFLAG := -m
 
-  # i386 becomes 32, and amd64 or x86_64 becomes 64
+  # i386,i686 -> 32
+  # amd64, x86_64, x64 -> 64
   ifeq "$(ARCH)" "amd64"
-      override ARCH := $(subst amd64,64,$(ARCH))
+    override ARCH := $(subst amd64,64,$(ARCH))
   endif
   ifeq "$(ARCH)" "x86_64"
-      override ARCH := $(subst x86_64,64,$(ARCH))
+    override ARCH := $(subst x86_64,64,$(ARCH))
   endif
+  ifeq "$(ARCH)" "x64"
+    override ARCH := $(subst x64,64,$(ARCH))
+  endif
   ifeq "$(ARCH)" "i386"
-      override ARCH := $(subst i386,32,$(ARCH))
+    override ARCH := $(subst i386,32,$(ARCH))
   endif
+  ifeq "$(ARCH)" "i686"
+    override ARCH := $(subst i686,32,$(ARCH))
+  endif
 
   ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
     DSYM = $(EXE).debug
   endif
 endif
 
 CFLAGS ?= -g -O0
 CFLAGS += $(ARCHFLAG)$(ARCH) $(FRAMEWORK_INCLUDES) $(CFLAGS_EXTRAS)