[lldb] Fix dotest argument order

[fdeazeve]

Summary

When running LLDB API tests, a user can override test arguments with
LLDB_TEST_USER_ARGS. However, these flags used to be concatenated with a
CMake-derived variable LLDB_TEST_COMMON_ARGS, as below:

set(LLDB_DOTEST_ARGS ${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS}
    CACHE INTERNAL STRING)

This is problematic, because LLDB_TEST_COMMON_ARGS must be processed
first, while LLDB_TEST_USER_ARGS args must be processed last, so that
user overrides are respected. Currently, if a user attempts to override
one of the "inferred" flags, the user's request is ignored. This is the
case, for example, with --libcxx-include-dir and
--libcxx-library-dir. Both flags are needed by the greendragon bots.

This commit removes the concatenation above, keeping the two original
variables throughout the entire flow, processing the user's flag last.

The variable LLDB_TEST_COMMON_ARGS needs to be a CACHE property, but it
is modified throughout the CMake file with set or list or string
commands, which don't work with properties. As such, a temporary
variable LLDB_TEST_COMMON_ARGS_VAR is created.

This was tested locally by invoking CMake with:
-DLLDB_TEST_USER_ARGS="--libcxx-include-dir=blah --libcxx-library-dir=blah2"
and checking that tests failed appropriately.

[fdeazeve]

Fix commit message typo

[fdeazeve]

Added missing files

[fdeazeve]

Updated review message with the latest edits to the commit message.

[JDevlieghere]

lldb/test/API/CMakeLists.txt

166

Instead of having two variables, why not move this to line 40 and make LLDB_TEST_COMMON_ARGS a cached variable and operate directly on that?

lldb/test/API/lit.site.cfg.py.in

26

Let's omit the cmake part, it doesn't really matter where these args come from.

[fdeazeve]

lldb/test/API/CMakeLists.txt

166

I actually address this in the commit message:

The variable LLDB_TEST_COMMON_ARGS needs to be a CACHE property, but it

is modified throughout the CMake file with set or list or string
commands, which don't work with properties. As such, a temporary
variable LLDB_TEST_COMMON_ARGS_VAR is created.

Does this make sense?

lldb/test/API/lit.site.cfg.py.in

26

Agreed!

[JDevlieghere]

LGTM

lldb/test/API/CMakeLists.txt

166

Gotcha, I didn't know about that limitation. I'm 99% sure you can still use the same variable and then cache it:

set(LLDB_TEST_COMMON_ARGS "")
list(APPEND LLDB_TEST_COMMON_ARGS "FOO")
list(APPEND LLDB_TEST_COMMON_ARGS "BAR")
set(LLDB_TEST_COMMON_ARGS "${LLDB_TEST_COMMON_ARGS}" CACHE INTERNAL STRING)

But maybe that's needless opaque.

[fdeazeve]

Addressed review comments

lldb/test/API/CMakeLists.txt

set(LLDB_TEST_ARCH
   ${LLDB_DEFAULT_TEST_ARCH}
   CACHE 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
   ""
   CACHE STRING "Specify additional arguments to pass to test runner. For example: '-C gcc -C clang -A i386 -A x86_64'")
 
-set(LLDB_TEST_COMMON_ARGS
+set(LLDB_TEST_COMMON_ARGS_VAR
   -u CXXFLAGS
   -u CFLAGS
   )
 
 # Set the path to the default lldb test executable.
 set(LLDB_DEFAULT_TEST_EXECUTABLE "${LLVM_RUNTIME_OUTPUT_INTDIR}/lldb${CMAKE_EXECUTABLE_SUFFIX}")
 
 set(LLDB_DEFAULT_TEST_DSYMUTIL "${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/dsymutil${CMAKE_EXECUTABLE_SUFFIX}")
set(LLDB_TEST_DEBUG_TEST_CRASHES
     0
     CACHE BOOL "(Windows only) Enables debugging of tests in the test suite by showing the crash dialog when lldb crashes")
 
   set(LLDB_TEST_HIDE_CONSOLE_WINDOWS
     1
     CACHE BOOL "(Windows only) Hides the console window for an inferior when it is launched through the test suite")
 
   if (LLDB_TEST_DEBUG_TEST_CRASHES)
-    set(LLDB_TEST_COMMON_ARGS ${LLDB_TEST_COMMON_ARGS} --enable-crash-dialog)
+    set(LLDB_TEST_COMMON_ARGS_VAR ${LLDB_TEST_COMMON_ARGS_VAR} --enable-crash-dialog)
   endif()
 
   if (NOT LLDB_TEST_HIDE_CONSOLE_WINDOWS)
-    set(LLDB_TEST_COMMON_ARGS ${LLDB_TEST_COMMON_ARGS} --show-inferior-console)
+    set(LLDB_TEST_COMMON_ARGS_VAR ${LLDB_TEST_COMMON_ARGS_VAR} --show-inferior-console)
   endif()
 endif()
 
 if (NOT ${CMAKE_SYSTEM_NAME} MATCHES "Windows|Darwin")
-  list(APPEND LLDB_TEST_COMMON_ARGS
+  list(APPEND LLDB_TEST_COMMON_ARGS_VAR
     --env ARCHIVER=${CMAKE_AR} --env OBJCOPY=${CMAKE_OBJCOPY})
 endif()
 
 if (NOT "${LLDB_LIT_TOOLS_DIR}" STREQUAL "")
   if (NOT EXISTS "${LLDB_LIT_TOOLS_DIR}")
     message(WARNING "LLDB_LIT_TOOLS_DIR ${LLDB_LIT_TOOLS_DIR} does not exist.")
   endif()
 endif()
 
 if(CMAKE_HOST_APPLE)
   if(LLDB_BUILD_FRAMEWORK)
     set(LLDB_FRAMEWORK_DIR ${LLDB_FRAMEWORK_ABSOLUTE_BUILD_DIR}/LLDB.framework)
   endif()
 
   # Use the same identity for testing
   get_property(code_sign_identity_used GLOBAL PROPERTY LLDB_DEBUGSERVER_CODESIGN_IDENTITY)
   if(code_sign_identity_used)
-    list(APPEND LLDB_TEST_COMMON_ARGS --codesign-identity "${code_sign_identity_used}")
+    list(APPEND LLDB_TEST_COMMON_ARGS_VAR --codesign-identity "${code_sign_identity_used}")
   endif()
 
   if(LLDB_USE_SYSTEM_DEBUGSERVER)
     lldb_find_system_debugserver(system_debugserver_path)
     if(LLDB_BUILD_FRAMEWORK)
       add_custom_target(debugserver
         COMMAND ${CMAKE_COMMAND} -E copy_if_different
         ${system_debugserver_path} $<TARGET_FILE_DIR:liblldb>/Resources
         COMMENT "Copying the system debugserver to LLDB.framework's resource directory for testing.")
     else()
       add_custom_target(debugserver
         COMMAND ${CMAKE_COMMAND} -E copy_if_different
         ${system_debugserver_path} ${LLVM_RUNTIME_OUTPUT_INTDIR}
         COMMENT "Copying the system debugserver to LLDB's binaries directory for testing.")
     endif()
     message(STATUS "LLDB tests use out-of-tree debugserver: ${system_debugserver_path}")
-    list(APPEND LLDB_TEST_COMMON_ARGS --out-of-tree-debugserver)
+    list(APPEND LLDB_TEST_COMMON_ARGS_VAR --out-of-tree-debugserver)
     add_lldb_test_dependency(debugserver)
   else()
     message(STATUS "LLDB tests use just-built debug server")
   endif()
 endif()
 
-set(LLDB_DOTEST_ARGS ${LLDB_TEST_COMMON_ARGS};${LLDB_TEST_USER_ARGS} CACHE INTERNAL STRING)
 set(dotest_args_replacement ${LLVM_BUILD_MODE})
 
 if(LLDB_BUILT_STANDALONE)
   # In paths to our build-tree, replace CMAKE_CFG_INTDIR with our configuration name placeholder.
   string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} config_runtime_output_dir ${LLVM_RUNTIME_OUTPUT_INTDIR})
-  string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_DOTEST_ARGS "${LLDB_DOTEST_ARGS}")
+  string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_TEST_COMMON_ARGS_VAR "${LLDB_TEST_COMMON_ARGS_VAR}")
+  string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_TEST_USER_ARGS "${LLDB_TEST_USER_ARGS}")
   string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_SOURCE_DIR "${LLDB_SOURCE_DIR}")
   string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_FRAMEWORK_DIR "${LLDB_FRAMEWORK_DIR}")
   string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_TEST_BUILD_DIRECTORY "${LLDB_TEST_BUILD_DIRECTORY}")
   string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_TEST_EXECUTABLE "${LLDB_TEST_EXECUTABLE}")
   string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_TEST_COMPILER "${LLDB_TEST_COMPILER}")
   string(REPLACE ${LLVM_RUNTIME_OUTPUT_INTDIR} ${config_runtime_output_dir} LLDB_TEST_DSYMUTIL "${LLDB_TEST_DSYMUTIL}")
 
   # Remaining ones must be paths to the provided LLVM build-tree.
if(NOT CMAKE_CONFIGURATION_TYPES)
       endif()
     endif()
   else()
     # Common case: LLVM used a single-configuration generator like Ninja.
     set(dotest_args_replacement ".")
   endif()
 endif()
 
-string(REPLACE ${CMAKE_CFG_INTDIR} ${dotest_args_replacement} LLDB_DOTEST_ARGS "${LLDB_DOTEST_ARGS}")
+string(REPLACE ${CMAKE_CFG_INTDIR} ${dotest_args_replacement} LLDB_TEST_COMMON_ARGS_VAR "${LLDB_TEST_COMMON_ARGS_VAR}")
+string(REPLACE ${CMAKE_CFG_INTDIR} ${dotest_args_replacement} LLDB_TEST_USER_ARGS "${LLDB_TEST_USER_ARGS}")
 string(REPLACE ${CMAKE_CFG_INTDIR} ${dotest_args_replacement} LLDB_SOURCE_DIR "${LLDB_SOURCE_DIR}")
 string(REPLACE ${CMAKE_CFG_INTDIR} ${dotest_args_replacement} LLDB_TEST_BUILD_DIRECTORY "${LLDB_TEST_BUILD_DIRECTORY}")
 string(REPLACE ${CMAKE_CFG_INTDIR} ${dotest_args_replacement} LLDB_TEST_EXECUTABLE "${LLDB_TEST_EXECUTABLE}")
 string(REPLACE ${CMAKE_CFG_INTDIR} ${dotest_args_replacement} LLDB_TEST_COMPILER "${LLDB_TEST_COMPILER}")
 string(REPLACE ${CMAKE_CFG_INTDIR} ${dotest_args_replacement} LLDB_TEST_DSYMUTIL "${LLDB_TEST_DSYMUTIL}")
 
+set(LLDB_TEST_COMMON_ARGS ${LLDB_TEST_COMMON_ARGS_VAR} CACHE INTERNAL STRING)
+
 configure_lit_site_cfg(
   ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
   ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg.py
   MAIN_CONFIG
   ${CMAKE_CURRENT_SOURCE_DIR}/lit.cfg.py)
 
 # Targets for running the test suite on the different Apple simulators.
 add_lit_testsuite(check-lldb-simulator-ios

lldb/test/API/lit.cfg.py

 if supported:
   lit_config.maxIndividualTestTime = 600
 else:
   lit_config.warning("Could not set a default per-test timeout. " + errormsg)
 
 # Build dotest command.
 dotest_cmd = [os.path.join(config.lldb_src_root, 'test', 'API', 'dotest.py')]
 
-if is_configured('dotest_args_str'):
-  dotest_cmd.extend(config.dotest_args_str.split(';'))
+if is_configured('dotest_common_args_str'):
+  dotest_cmd.extend(config.dotest_common_args_str.split(';'))
 
 # Library path may be needed to locate just-built clang and libcxx.
 if is_configured('llvm_libs_dir'):
   dotest_cmd += ['--env', 'LLVM_LIBS_DIR=' + config.llvm_libs_dir]
 
 # Include path may be needed to locate just-built libcxx.
 if is_configured('llvm_include_dir'):
   dotest_cmd += ['--env', 'LLVM_INCLUDE_DIR=' + config.llvm_include_dir]

 elif 'lldb-simulator-tvos' in config.available_features:
   dotest_cmd += ['--apple-sdk', 'appletvsimulator',
                  '--platform-name', 'tvos-simulator']
 
 if is_configured('enabled_plugins'):
   for plugin in config.enabled_plugins:
     dotest_cmd += ['--enable-plugin', plugin]
 
+# `dotest` args come from three different sources:
+# 1. Derived by CMake based on its configs (LLDB_TEST_COMMON_ARGS), which end
+# up in `dotest_common_args_str`.
+# 2. CMake user parameters (LLDB_TEST_USER_ARGS), which end up in
+# `dotest_user_args_str`.
+# 3. With `llvm-lit "--param=dotest-args=..."`, which end up in
+# `dotest_lit_args_str`.
+# Check them in this order, so that more specific overrides are visited last.
+# In particular, (1) is visited at the top of the file, since the script
+# derives other information from it.
+
+if is_configured('dotest_user_args_str'):
+  dotest_cmd.extend(config.dotest_user_args_str.split(';'))
+
 if is_configured('dotest_lit_args_str'):
   # We don't want to force users passing arguments to lit to use `;` as a
   # separator. We use Python's simple lexical analyzer to turn the args into a
   # list. Pass there arguments last so they can override anything that was
   # already configured.
   dotest_cmd.extend(shlex.split(config.dotest_lit_args_str))
 
 # Load LLDB test format.

lldb/test/API/lit.site.cfg.py.in

 config.host_triple = "@LLVM_HOST_TRIPLE@"
 config.shared_libs = @LLVM_ENABLE_SHARED_LIBS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.target_triple = "@LLVM_TARGET_TRIPLE@"
 config.lldb_build_directory = "@LLDB_TEST_BUILD_DIRECTORY@"
 config.python_executable = "@Python3_EXECUTABLE@"
 config.lua_executable = "@Lua_EXECUTABLE@"
 config.lua_test_entry = "TestLuaAPI.py"
-config.dotest_args_str = lit_config.substitute("@LLDB_DOTEST_ARGS@")
+config.dotest_common_args_str = lit_config.substitute("@LLDB_TEST_COMMON_ARGS@")
+config.dotest_user_args_str = lit_config.substitute("@LLDB_TEST_USER_ARGS@")
 config.lldb_enable_python = @LLDB_ENABLE_PYTHON@
 config.dotest_lit_args_str = None
 config.enabled_plugins = []
 config.lldb_executable = lit_config.substitute('@LLDB_TEST_EXECUTABLE@')
 config.test_arch = '@LLDB_TEST_ARCH@'
 config.test_compiler = lit_config.substitute('@LLDB_TEST_COMPILER@')
 config.dsymutil = lit_config.substitute('@LLDB_TEST_DSYMUTIL@')
 config.has_libcxx = '@LLDB_HAS_LIBCXX@'

lldb/utils/lldb-dotest/CMakeLists.txt

 # Make lldb-dotest a custom target.
 add_custom_target(lldb-dotest)
 add_dependencies(lldb-dotest lldb-test-depends)
 set_target_properties(lldb-dotest PROPERTIES FOLDER "lldb utils")
 
-get_property(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)
+get_property(LLDB_TEST_USER_ARGS GLOBAL PROPERTY LLDB_TEST_USER_ARGS_PROPERTY)
+get_property(LLDB_TEST_COMMON_ARGS GLOBAL PROPERTY LLDB_TEST_COMMON_ARGS_PROPERTY)
 set(LLDB_LIBS_DIR "${LLVM_LIBRARY_OUTPUT_INTDIR}")
 
 llvm_canonicalize_cmake_booleans(
   LLDB_BUILD_INTEL_PT
 )
 
 set(LLVM_TOOLS_DIR "${LLVM_TOOLS_BINARY_DIR}")
 set(vars
-  LLDB_DOTEST_ARGS
+  LLDB_TEST_COMMON_ARGS
+  LLDB_TEST_USER_ARGS
   LLDB_SOURCE_DIR
   LLDB_FRAMEWORK_DIR
   LLDB_TEST_BUILD_DIRECTORY
   LLDB_TEST_EXECUTABLE
   LLDB_TEST_COMPILER
   LLDB_TEST_DSYMUTIL
   LLDB_LIBS_DIR
   LLVM_TOOLS_DIR