[Lldb-commits] [lldb] [lldb][rpc] Fix build failures when building lldb-rpc-gen (PR #151603)

[Martin Storsjö via lldb-commits]

mstorsjo wrote:

Now I've had time to study lldb-rpc-gen a little bit closer...

For myself and others, to clarify what lldb-rpc-gen does (as how command options flow into it that aren't immediately obvious): It is a tool that includes clang tooling libraries. It runs at the regular LLVM/LLDB build directory, reading `compile_commans.json` from there, then using them to figure out how to compile certain files, and use those to preprocess(?) and parse some LLDB headers.

This is in a rather large contrast to other existing build time code generation tools (like `llvm-tblgen` or `clang-tidy-confusable-chars-gen`); those are relatively small executables that on their own generate code to be used during the build - and those tools are mostly independent of the build environment.

Your initial attempts seem quite close to actually getting this right (already attempting to use `get_host_tool_path` etc), but both the initial attempt and this one missed one subtle detail: The cmake functions `get_host_tool_path` and `setup_host_tool` do take a cmake variable name `LLDB_RPC_GEN_EXE` as input, which allows the user to set this variable if they want to point to the tool, but it doesn't set the final path in that variable, but in a different variable.

I managed to simplify the setup of all this, and got a cross build to actually successfully build and use a nested cmake build to build `lldb-rpc-gen`, just like it does for other tools. My diff to achieve this looks like this:

```diff
diff --git a/lldb/cmake/modules/LLDBConfig.cmake b/lldb/cmake/modules/LLDBConfig
.cmake
index 88b4eca4112c..f108110ff94b 100644
--- a/lldb/cmake/modules/LLDBConfig.cmake
+++ b/lldb/cmake/modules/LLDBConfig.cmake
@@ -334,11 +334,4 @@ else()
 endif()
 
 
-if (CMAKE_CROSSCOMPILING)
-  set(LLDB_BUILD_LLDBRPC OFF CACHE BOOL "")
-  get_host_tool_path(lldb-rpc-gen LLDB_RPC_GEN_EXE lldb_rpc_gen_exe lldb_rpc_gen_target)
-else()
-  set(LLDB_BUILD_LLDBRPC ON CACHE BOOL "")
-endif()
-
 include(LLDBGenerateConfig)
diff --git a/lldb/tools/CMakeLists.txt b/lldb/tools/CMakeLists.txt
index e2f039527ad7..d3096765ad8f 100644
--- a/lldb/tools/CMakeLists.txt
+++ b/lldb/tools/CMakeLists.txt
@@ -10,9 +10,7 @@ add_subdirectory(lldb-fuzzer EXCLUDE_FROM_ALL)
 
 add_lldb_tool_subdirectory(lldb-instr)
 add_lldb_tool_subdirectory(lldb-dap)
-if (LLDB_BUILD_LLDBRPC)
-  add_lldb_tool_subdirectory(lldb-rpc-gen)
-endif()
+add_lldb_tool_subdirectory(lldb-rpc-gen)
 if (LLDB_CAN_USE_LLDB_RPC_SERVER)
   add_subdirectory(lldb-rpc)
 endif()
diff --git a/lldb/tools/lldb-rpc-gen/CMakeLists.txt b/lldb/tools/lldb-rpc-gen/CMakeLists.txt
index 4b2e5d7483a5..7a50072865ab 100644
--- a/lldb/tools/lldb-rpc-gen/CMakeLists.txt
+++ b/lldb/tools/lldb-rpc-gen/CMakeLists.txt
@@ -18,11 +18,6 @@ add_lldb_tool(lldb-rpc-gen
       Support
   )
 
-if (CMAKE_CROSSCOMPILING)
-  setup_host_tool(lldb-rpc-gen LLDB_RPC_GEN_EXE lldb_rpc_gen_exe lldb_rpc_gen_target)
-else()
-  if (NOT DEFINED LLDB_RPC_GEN_EXE)
-    set(LLDB_RPC_GEN_EXE $<TARGET_FILE:lldb-rpc-gen> CACHE STRING "Executable that generates lldb-rpc-server")
-  endif()
-endif()
+add_dependencies(lldb-rpc-gen clang-resource-headers)
 
+setup_host_tool(lldb-rpc-gen LLDB_RPC_GEN_EXE lldb_rpc_gen_exe lldb_rpc_gen_target)
diff --git a/lldb/tools/lldb-rpc/LLDBRPCGeneration.cmake b/lldb/tools/lldb-rpc/LLDBRPCGeneration.cmake
index a4cacf8692a8..91188ca393a1 100644
--- a/lldb/tools/lldb-rpc/LLDBRPCGeneration.cmake
+++ b/lldb/tools/lldb-rpc/LLDBRPCGeneration.cmake
@@ -1,9 +1,3 @@
-if (NOT DEFINED LLDB_RPC_GEN_EXE)
-  message(FATAL_ERROR
-    "Unable to generate lldb-rpc sources because LLDB_RPC_GEN_EXE is not
-    defined. If you are cross-compiling, please build lldb-rpc-gen for your host
-    platform.")
-endif()
 set(lldb_rpc_generated_dir "${CMAKE_CURRENT_BINARY_DIR}/generated")
 set(lldb_rpc_server_generated_source_dir "${lldb_rpc_generated_dir}/server")
 
@@ -60,14 +54,14 @@ add_custom_command(OUTPUT ${lldb_rpc_gen_byproducts}
   COMMAND ${CMAKE_COMMAND} -E make_directory
     ${lldb_rpc_server_generated_source_dir}
 
-  COMMAND ${LLDB_RPC_GEN_EXE}
+  COMMAND ${lldb_rpc_gen_exe}
     -p ${CMAKE_BINARY_DIR}
     --output-dir=${lldb_rpc_generated_dir}
     ${sysroot_arg}
     --extra-arg="-USWIG"
     ${api_headers}
 
-  DEPENDS ${LLDB_RPC_GEN_EXE} ${api_headers}
+  DEPENDS ${lldb_rpc_gen_target} ${api_headers}
   COMMENT "Generating sources for lldb-rpc-server..."
   WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
 )
```

I had to add the dependency on `clang-resource-headers`, so that the tool `<builddir>/NATIVE/bin/lldb-rpc-gen` gets the clang resource headers it expects in `<builddir>/NATIVE/lib/clang/<version>/include`.


However, even with all this in place, I fear that it may be brittle - in particular during cross builds. (I wouldn't always assume that the clang tooling flawlessly would figure out the ins and outs if I'm using an unusual cross compiler.)

And secondly - building `lldb-rpc-gen` requires building most of LLVM and Clang, which adds a lot of extra work to cross builds. Currently, when cross compiling, it builds `llvm-tblgen`, `clang-tblgen` and `lldb-tblgen` and similar tools natively - which is extra overhead to the build. But this is usually just a couple hundred object files - so it's easier to just let cmake do that, rather than to fiddle with manually pointing to a preexisting native build (with the risk of it being out of sync etc). But here, this essentially would make the nested native build duplicate 70% of the whole llvm build - which is quite out of scope for what's probably expected.


I also see that the existing logic for setting up lldb-rpc here does seem to try to account for this to some extent:
```
# In a cross-compile build, we need to skip building the generated
# lldb-rpc sources in the first phase of host build so that they can
# get built using the just-built Clang toolchain in the second phase.
if ((CMAKE_CROSSCOMPILING OR LLVM_HOST_TRIPLE MATCHES "${LLVM_DEFAULT_TARGET_TRIPLE}") AND
```
But it's very unclear to me exactly how this is meant to work - the condition seems to miss a negation with regards to `CMAKE_CROSS_COMPILING`? (And the check for `LLVM_HOST_TRIPLE` and `LLVM_DEFAULT_TARGET_TRIPLE` seems unusual here - we don't normally check `LLVM_DEFAULT_TARGET_TRIPLE` like that - just checking `CMAKE_CROSSCOMPILING` should be enough.)

So we can probably do something like my inline diff above, to make the build succeed _if_ we want to build this, but we probably should default to having this disabled in general:
```diff

diff --git a/lldb/cmake/modules/LLDBConfig.cmake b/lldb/cmake/modules/LLDBConfig.cmake
index f108110ff94b..388e37d0da78 100644
--- a/lldb/cmake/modules/LLDBConfig.cmake
+++ b/lldb/cmake/modules/LLDBConfig.cmake
@@ -323,14 +323,16 @@ else()
     set(LLDB_CAN_USE_DEBUGSERVER OFF)
 endif()
 
-# In a cross-compile build, we need to skip building the generated
-# lldb-rpc sources in the first phase of host build so that they can
-# get built using the just-built Clang toolchain in the second phase.
-if ((CMAKE_CROSSCOMPILING OR LLVM_HOST_TRIPLE MATCHES "${LLVM_DEFAULT_TARGET_TRIPLE}") AND
-    CMAKE_SYSTEM_NAME MATCHES "AIX|Android|Darwin|FreeBSD|Linux|NetBSD|OpenBSD|Windows")
-  set(LLDB_CAN_USE_LLDB_RPC_SERVER ON)
-else()
-  set(LLDB_CAN_USE_LLDB_RPC_SERVER OFF)
+if (NOT DEFINED LLDB_CAN_USE_LLDB_RPC_SERVER)
+  # In a cross-compile build, we don't want to try to build a lldb-rpc-gen, as
+  # as that essentially forces rebuilding a whole separate native copy of clang.
+  # Allow the caller to set this variable manually to opt in/out of it.
+  if (NOT CMAKE_CROSSCOMPILING AND
+      CMAKE_SYSTEM_NAME MATCHES "AIX|Android|Darwin|FreeBSD|Linux|NetBSD|OpenBSD|Windows")
+    set(LLDB_CAN_USE_LLDB_RPC_SERVER ON)
+  else()
+    set(LLDB_CAN_USE_LLDB_RPC_SERVER OFF)
+  endif()
 endif()

```

https://github.com/llvm/llvm-project/pull/151603