[runtimes] Fix parsing of LIB{CXX,CXXABI,UNWIND}_TEST_PARAMS #67691

arichardson · 2023-09-28T15:00:46Z

Since 78d649a the recommended way to pass an executor is to use the _TEST_PARAMS variable, which means we now pass more complicated value (including ones that may contain multiple =) as part of this variable. However, the REGEX REPLACE being used has greedy matches so everything up to the last = becomes part of the variable name which results in invalid syntax in the generated lit config file.

This was noticed due to builder failures for those using the CrossWinToARMLinux.cmake cache file.

arichardson · 2023-09-28T15:01:35Z

@vvereschaka I tested this locally by adding a parameter with quotes to my CMake invocation, so it should hopefully work. Could you please test if this fixes the builder?

llvmbot · 2023-09-28T15:01:58Z

@llvm/pr-subscribers-libcxx
@llvm/pr-subscribers-libunwind
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-libcxxabi

Changes

Since 78d649a the recommended way to pass an executor is to use the _TEST_PARAMS variable, which means we now pass more complicated value (including ones that may contain multiple =) as part of this variable. However, the REGEX REPLACE being used has greedy matches so everything up to the last = becomes part of the variable name which results in invalid syntax in the generated lit config file.

This was noticed due to builder failures for those using the CrossWinToARMLinux.cmake cache file.

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

5 Files Affected:

(modified) clang/cmake/caches/CrossWinToARMLinux.cmake (+7-3)
(modified) libcxx/test/CMakeLists.txt (+3-9)
(modified) libcxxabi/test/CMakeLists.txt (+3-5)
(modified) libunwind/test/CMakeLists.txt (+3-8)
(added) runtimes/cmake/Modules/HandleLitArguments.cmake (+20)

diff --git a/clang/cmake/caches/CrossWinToARMLinux.cmake b/clang/cmake/caches/CrossWinToARMLinux.cmake
index fd341b182fd6563..a4dba4d8666027b 100644
--- a/clang/cmake/caches/CrossWinToARMLinux.cmake
+++ b/clang/cmake/caches/CrossWinToARMLinux.cmake
@@ -151,6 +151,10 @@ set(RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS
 
 find_package(Python3 COMPONENTS Interpreter)
 
+set(RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBUNWIND_TEST_PARAMS "${RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_TEST_PARAMS}" CACHE INTERNAL "")
+set(RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBCXXABI_TEST_PARAMS "${RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_TEST_PARAMS}" CACHE INTERNAL "")
+set(RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBCXX_TEST_PARAMS "${RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_TEST_PARAMS}" CACHE INTERNAL "")
+
 # Remote test configuration.
 if(DEFINED REMOTE_TEST_HOST)
   # Allow override with the custom values.
@@ -162,9 +166,9 @@ if(DEFINED REMOTE_TEST_HOST)
         "\\\"${Python3_EXECUTABLE}\\\" \\\"${LLVM_PROJECT_DIR}/llvm/utils/remote-exec.py\\\" --host=${REMOTE_TEST_USER}@${REMOTE_TEST_HOST}"
         CACHE STRING "")
 
-  set(RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBUNWIND_TEST_PARAMS "${RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_TEST_PARAMS} 'executor=${DEFAULT_TEST_EXECUTOR}'" CACHE STRING "")
-  set(RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBCXXABI_TEST_PARAMS "${RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_TEST_PARAMS} 'executor=${DEFAULT_TEST_EXECUTOR}'" CACHE STRING "")
-  set(RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBCXX_TEST_PARAMS    "${RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_TEST_PARAMS} 'executor=${DEFAULT_TEST_EXECUTOR}'" CACHE STRING "")
+  list(APPEND RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBUNWIND_TEST_PARAMS "executor=${DEFAULT_TEST_EXECUTOR}")
+  list(APPEND RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBCXXABI_TEST_PARAMS "executor=${DEFAULT_TEST_EXECUTOR}")
+  list(APPEND RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_LIBCXX_TEST_PARAMS    "executor=${DEFAULT_TEST_EXECUTOR}")
 endif()
 
 set(LLVM_INSTALL_TOOLCHAIN_ONLY ON CACHE BOOL "")
diff --git a/libcxx/test/CMakeLists.txt b/libcxx/test/CMakeLists.txt
index 0d5fa6959f17881..0d4771efaf725f4 100644
--- a/libcxx/test/CMakeLists.txt
+++ b/libcxx/test/CMakeLists.txt
@@ -1,3 +1,5 @@
+include(HandleLitArguments)
+
 add_subdirectory(tools)
 
 # By default, libcxx and libcxxabi share a library directory.
@@ -9,10 +11,6 @@ endif()
 set(AUTO_GEN_COMMENT "## Autogenerated by libcxx configuration.\n# Do not edit!")
 set(SERIALIZED_LIT_PARAMS "# Lit parameters serialized here for llvm-lit to pick them up\n")
 
-macro(serialize_lit_param param value)
-  string(APPEND SERIALIZED_LIT_PARAMS "config.${param} = ${value}\n")
-endmacro()
-
 if (LIBCXX_EXECUTOR)
   message(DEPRECATION "LIBCXX_EXECUTOR is deprecated, please add executor=... to LIBCXX_TEST_PARAMS")
   serialize_lit_param(executor "\"${LIBCXX_EXECUTOR}\"")
@@ -38,11 +36,7 @@ if (LLVM_USE_SANITIZER)
   serialize_lit_param(use_sanitizer "\"${LLVM_USE_SANITIZER}\"")
 endif()
 
-foreach(param IN LISTS LIBCXX_TEST_PARAMS)
-  string(REGEX REPLACE "(.+)=(.+)" "\\1" name "${param}")
-  string(REGEX REPLACE "(.+)=(.+)" "\\2" value "${param}")
-  serialize_lit_param("${name}" "\"${value}\"")
-endforeach()
+serialize_lit_params_list(LIBCXX_TEST_PARAMS)
 
 if (NOT DEFINED LIBCXX_TEST_DEPS)
   message(FATAL_ERROR "Expected LIBCXX_TEST_DEPS to be defined")
diff --git a/libcxxabi/test/CMakeLists.txt b/libcxxabi/test/CMakeLists.txt
index b65ac3a9d16422c..bc601dd44058d12 100644
--- a/libcxxabi/test/CMakeLists.txt
+++ b/libcxxabi/test/CMakeLists.txt
@@ -1,4 +1,6 @@
 include(AddLLVM) # for configure_lit_site_cfg and add_lit_testsuite
+include(HandleLitArguments)
+
 macro(pythonize_bool var)
   if (${var})
     set(${var} True)
@@ -52,11 +54,7 @@ else()
   serialize_lit_param(target_triple "\"${LLVM_DEFAULT_TARGET_TRIPLE}\"")
 endif()
 
-foreach(param IN LISTS LIBCXXABI_TEST_PARAMS)
-  string(REGEX REPLACE "(.+)=(.+)" "\\1" name "${param}")
-  string(REGEX REPLACE "(.+)=(.+)" "\\2" value "${param}")
-  serialize_lit_param("${name}" "\"${value}\"")
-endforeach()
+serialize_lit_params_list(LIBCXXABI_TEST_PARAMS)
 
 configure_file("${CMAKE_CURRENT_SOURCE_DIR}/configs/cmake-bridge.cfg.in"
                "${CMAKE_CURRENT_BINARY_DIR}/cmake-bridge.cfg"
diff --git a/libunwind/test/CMakeLists.txt b/libunwind/test/CMakeLists.txt
index 084da0ab8f354d7..2e99c182862ae1f 100644
--- a/libunwind/test/CMakeLists.txt
+++ b/libunwind/test/CMakeLists.txt
@@ -1,4 +1,6 @@
 include(AddLLVM) # for add_lit_testsuite
+include(HandleLitArguments)
+
 macro(pythonize_bool var)
   if (${var})
     set(${var} True)
@@ -14,9 +16,6 @@ pythonize_bool(LIBUNWIND_USES_ARM_EHABI)
 set(AUTO_GEN_COMMENT "## Autogenerated by libunwind configuration.\n# Do not edit!")
 set(SERIALIZED_LIT_PARAMS "# Lit parameters serialized here for llvm-lit to pick them up\n")
 
-macro(serialize_lit_param param value)
-  string(APPEND SERIALIZED_LIT_PARAMS "config.${param} = ${value}\n")
-endmacro()
 
 if (LIBUNWIND_EXECUTOR)
   message(DEPRECATION "LIBUNWIND_EXECUTOR is deprecated, please add executor=... to LIBUNWIND_TEST_PARAMS")
@@ -35,11 +34,7 @@ else()
   serialize_lit_param(target_triple "\"${LLVM_DEFAULT_TARGET_TRIPLE}\"")
 endif()
 
-foreach(param IN LISTS LIBUNWIND_TEST_PARAMS)
-  string(REGEX REPLACE "(.+)=(.+)" "\\1" name "${param}")
-  string(REGEX REPLACE "(.+)=(.+)" "\\2" value "${param}")
-  serialize_lit_param("${name}" "\"${value}\"")
-endforeach()
+serialize_lit_params_list(LIBUNWIND_TEST_PARAMS)
 
 configure_file("${CMAKE_CURRENT_SOURCE_DIR}/configs/cmake-bridge.cfg.in"
                "${CMAKE_CURRENT_BINARY_DIR}/cmake-bridge.cfg"
diff --git a/runtimes/cmake/Modules/HandleLitArguments.cmake b/runtimes/cmake/Modules/HandleLitArguments.cmake
new file mode 100644
index 000000000000000..00bbe931c3fcdbf
--- /dev/null
+++ b/runtimes/cmake/Modules/HandleLitArguments.cmake
@@ -0,0 +1,20 @@
+
+macro(serialize_lit_param param value)
+  string(APPEND SERIALIZED_LIT_PARAMS "config.${param} = ${value}\n")
+endmacro()
+
+macro(serialize_lit_string_param param value)
+  # Ensure that all quotes in the value are escaped for a valid python string.
+  string(REPLACE "\"" "\\\"" _escaped_value "${value}")
+  string(APPEND SERIALIZED_LIT_PARAMS "config.${param} = \"${_escaped_value}\"\n")
+endmacro()
+
+macro(serialize_lit_params_list list)
+  foreach(param IN LISTS ${list})
+    string(FIND "${param}" "=" _eq_index)
+    string(SUBSTRING "${param}" 0 ${_eq_index} name)
+    string(SUBSTRING "${param}" ${_eq_index} -1 value)
+    string(SUBSTRING "${value}" 1 -1 value) # strip the leading =
+    serialize_lit_string_param("${name}" "${value}")
+  endforeach()
+endmacro()

ldionne · 2023-09-28T15:30:49Z

runtimes/cmake/Modules/HandleLitArguments.cmake

@@ -0,0 +1,20 @@
+
+macro(serialize_lit_param param value)
+  string(APPEND SERIALIZED_LIT_PARAMS "config.${param} = ${value}\n")


This looks reasonable, but could we avoid using the global variable SERIALIZED_LIT_PARAMS here? We should thread it through the functions instead, otherwise these functions all "return their result" by modifying the global variable, which is yucky.

clang/cmake/caches/CrossWinToARMLinux.cmake

This introduces new macros that expect an output variable and a new macro serialize_lit_string_param() that ensure that the serialized value is escaped appropriately to store it into a python change.

Since 78d649a the recommended way to pass an executor is to use the _TEST_PARAMS variable, which means we now pass more complicated value (including ones that may contain multiple `=`) as part of this variable. However, the `REGEX REPLACE` being used has greedy matches so everything up to the last = becomes part of the variable name which results in invalid syntax in the generated lit config file. This was noticed due to builder failures for those using the CrossWinToARMLinux.cmake cache file. Co-authored-by: Vladimir Vereschaka <[email protected]>

vvereschaka · 2023-09-29T22:16:38Z

@arichardson,
any progress?!

arichardson · 2023-09-29T22:18:43Z

@arichardson, any progress?!

Sorry thought I'd pushed the latest version earlier, turns out there was an error during the push. Updated now.

vvereschaka · 2023-09-29T22:20:32Z

@arichardson,
thank you. I'm going to test it during the next 15-20 minutes.

vvereschaka · 2023-09-29T22:49:45Z

@arichardson ,
works fine on the builders locally. Thank you.
Would yo merge these changes if they are ready?

arichardson · 2023-09-29T23:33:00Z

@arichardson , works fine on the builders locally. Thank you. Would yo merge these changes if they are ready?

Thanks for testing! I'll merge these changes as soon as @ldionne is happy with the new macros. Once that happens I'll push the first commit manually and then merge the second one via the github UI (since we can't do "rebase and merge" due to repo settings).

vvereschaka · 2023-10-02T17:47:27Z

@ldionne @arichardson any progress?

vvereschaka · 2023-10-04T18:26:07Z

@ldionne @arichardson ,
the problem is still there for more than 10 days already. Do you guys plan to fix it!

arichardson · 2023-10-04T18:36:09Z

@ldionne @arichardson , the problem is still there for more than 10 days already. Do you guys plan to fix it!

I am waiting for @ldionne to approve this PR.

vvereschaka · 2023-10-04T21:37:13Z

@arichardson ,
would you commit these changes without @ldionne 's approval? or revert your #66545?

ldionne · 2023-10-04T22:11:55Z

Thanks for the fix and sorry for the delay!

arichardson requested review from ldionne and vvereschaka September 28, 2023 15:00

arichardson requested review from a team as code owners September 28, 2023 15:00

llvmbot added cmake Build system in general and CMake in particular clang Clang issues not falling into any other category libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libc++abi libc++abi C++ Runtime Library. Not libc++. libunwind labels Sep 28, 2023

ldionne requested changes Sep 28, 2023

View reviewed changes

vvereschaka reviewed Sep 28, 2023

View reviewed changes

clang/cmake/caches/CrossWinToARMLinux.cmake Show resolved Hide resolved

arichardson and others added 2 commits September 29, 2023 15:16

[runtimes] Factor out serialize_lit_param to a separate file

70cfa2f

This introduces new macros that expect an output variable and a new macro serialize_lit_string_param() that ensure that the serialized value is escaped appropriately to store it into a python change.

arichardson force-pushed the libcxx-executor-fixes branch from 7f2045e to b4e14f8 Compare September 29, 2023 22:17

arichardson requested a review from ldionne September 29, 2023 23:33

vvereschaka approved these changes Sep 29, 2023

View reviewed changes

ldionne merged commit e599422 into llvm:main Oct 4, 2023

arichardson deleted the libcxx-executor-fixes branch October 5, 2023 14:20

stepthomas mentioned this pull request Oct 10, 2023

AMDGPU stepthomas atomic csub no rtn forms ver2 stepthomas/llvm-project#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[runtimes] Fix parsing of LIB{CXX,CXXABI,UNWIND}_TEST_PARAMS #67691

[runtimes] Fix parsing of LIB{CXX,CXXABI,UNWIND}_TEST_PARAMS #67691

arichardson commented Sep 28, 2023

arichardson commented Sep 28, 2023

llvmbot commented Sep 28, 2023 •

edited

Loading

ldionne Sep 28, 2023

vvereschaka commented Sep 29, 2023

arichardson commented Sep 29, 2023

vvereschaka commented Sep 29, 2023

vvereschaka commented Sep 29, 2023

arichardson commented Sep 29, 2023

vvereschaka commented Oct 2, 2023

vvereschaka commented Oct 4, 2023

arichardson commented Oct 4, 2023

vvereschaka commented Oct 4, 2023

ldionne commented Oct 4, 2023

		@@ -0,0 +1,20 @@

		macro(serialize_lit_param param value)
		string(APPEND SERIALIZED_LIT_PARAMS "config.${param} = ${value}\n")

[runtimes] Fix parsing of LIB{CXX,CXXABI,UNWIND}_TEST_PARAMS #67691

[runtimes] Fix parsing of LIB{CXX,CXXABI,UNWIND}_TEST_PARAMS #67691

Conversation

arichardson commented Sep 28, 2023

arichardson commented Sep 28, 2023

llvmbot commented Sep 28, 2023 • edited Loading

ldionne Sep 28, 2023

Choose a reason for hiding this comment

vvereschaka commented Sep 29, 2023

arichardson commented Sep 29, 2023

vvereschaka commented Sep 29, 2023

vvereschaka commented Sep 29, 2023

arichardson commented Sep 29, 2023

vvereschaka commented Oct 2, 2023

vvereschaka commented Oct 4, 2023

arichardson commented Oct 4, 2023

vvereschaka commented Oct 4, 2023

ldionne commented Oct 4, 2023

llvmbot commented Sep 28, 2023 •

edited

Loading