[lldb][rpc] Fix build failures when building lldb-rpc-gen #151603
Conversation
|
@llvm/pr-subscribers-lldb Author: Chelsea Cassanova (chelcassanova) ChangesOn some cross compile builds, the tool is not correctly set and the command that runs the tool has an empty path for the tool, causing the build to fail. We can setup the tool as a build host tool using setup_host_tool. Full diff: https://github.com/llvm/llvm-project/pull/151603.diff 2 Files Affected:
diff --git a/lldb/cmake/modules/LLDBConfig.cmake b/lldb/cmake/modules/LLDBConfig.cmake
index fc84e581cc188..88b4eca4112ce 100644
--- a/lldb/cmake/modules/LLDBConfig.cmake
+++ b/lldb/cmake/modules/LLDBConfig.cmake
@@ -326,27 +326,19 @@ 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 (NOT DEFINED LLDB_CAN_USE_LLDB_RPC_SERVER)
- set(LLDB_CAN_USE_LLDB_RPC_SERVER OFF)
+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()
- 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)
- endif()
+ set(LLDB_CAN_USE_LLDB_RPC_SERVER OFF)
endif()
-if (NOT DEFINED LLDB_BUILD_LLDBRPC)
- set(LLDB_BUILD_LLDBRPC OFF)
+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()
- 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()
+ set(LLDB_BUILD_LLDBRPC ON CACHE BOOL "")
endif()
include(LLDBGenerateConfig)
diff --git a/lldb/tools/lldb-rpc-gen/CMakeLists.txt b/lldb/tools/lldb-rpc-gen/CMakeLists.txt
index 65b76431d1bea..4b2e5d7483a59 100644
--- a/lldb/tools/lldb-rpc-gen/CMakeLists.txt
+++ b/lldb/tools/lldb-rpc-gen/CMakeLists.txt
@@ -18,6 +18,11 @@ add_lldb_tool(lldb-rpc-gen
Support
)
-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")
+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()
+
|
I'd rather skip the "some" word here, this happens whenever cmake things we're cross compiling - it's not just an odd case of some cross compiles. |
lldb/cmake/modules/LLDBConfig.cmake
Outdated
There was a problem hiding this comment.
Wouldn't this enable this build configuration for macOS universal builds, where we still expect this to fail?
There was a problem hiding this comment.
Yes, I had forgot to add that check in, my mistake.
There was a problem hiding this comment.
This doesn't seem right to me. And in actual tests with cross builds, this still hits the same /bin/sh: 1: -p: not found error.
We shouldn't need any sort of if (CMAKE_CROSSCOMPILING) here at all - we should unconditionally use setup_host_tool. That cmake function then internally checks whether it can expect to use the just-built tool, or if it needs to do a nested build to produce a native lldb-rpc-gen that can be used instead. Again, have a look at the corresponding clang-tidy case - it doesn't have any such conditionals.
Then after that, you do need to modify the following code that actually uses lldb-rpc-gen too. It needs to call the tool ${lldb_rpc_gen_exe} instead of $<TARGET_FILE:lldb-rpc-gen>. This variable then contains the path of the right lldb-rpc-gen executable that should be used. And whenever you're using it, you need to add a dependency on ${lldb_rpc_gen_target}, which is the cmake/ninja target which produces ${lldb_rpc_gen_exe}, possibly through a nested cmake build.
See https://github.com/llvm/llvm-project/blob/llvmorg-22-init/clang-tools-extra/clang-tidy/misc/CMakeLists.txt#L12-L15 how this is done in the case of clang-tidy. I understand that lldb-rpc-gen is donig something more complicated here, but this needs to be hooked up in one way or another.
There was a problem hiding this comment.
We shouldn't need any sort of if (CMAKE_CROSSCOMPILING) here at all - we should unconditionally use setup_host_tool. That cmake function then internally checks whether it can expect to use the just-built tool, or if it needs to do a nested build to produce a native lldb-rpc-gen that can be used instead. Again, have a look at the corresponding clang-tidy case - it doesn't have any such conditionals.
Then after that, you do need to modify the following code that actually uses lldb-rpc-gen too. It needs to call the tool
Noted, I hadn't realized that setup_host_tool could be used to replace how we setup the tool's binary entirely here, that is my mistake.
There was a problem hiding this comment.
When you unconditionally adopt setup_host_tool, you can also get rid of this manual variable for pointing to a prebuilt lldb-rpc-gen binary; the setup_host_tool (which internally calls get_host_tool_path) machinery automatically configures a similar cmake variable to let you manually override the path to the binary, and it also checks in the directory LLVM_NATIVE_TOOL_DIR if you happen to have a preexisting binary there, and then based on that decide to either compile a new one or use the existing one.
There was a problem hiding this comment.
Noted that the original way that we setup the binary can now be removed.
|
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 This is in a rather large contrast to other existing build time code generation tools (like Your initial attempts seem quite close to actually getting this right (already attempting to use 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 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 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 I also see that the existing logic for setting up lldb-rpc here does seem to try to account for this to some extent: But it's very unclear to me exactly how this is meant to work - the condition seems to miss a negation with regards to 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 --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()
|
Ah, so if I'm understanding correctly I'm still using LLDB_RPC_GEN_EXE as the variable for the tool path even though
The original logic for cross compile builds was implemented to prevent an issue where the files generated from the rpc-gen tool were processed too early, before the Clang toolchain itself was also built: #148996 (comment), which would cause errors of missing includes of files like cstddef.
Thank you very very much for the help, cross compile builds are configurations I haven't adjusted to working with yet so this is fairly new for me. Also, for the universal binary issue, technically building for a universal binary is doing a cross compile build. To avoid the issue of there being 2 compile jobs when clang tooling expects 1, we could possibly modify the build such that in a universal build we only process the input header files for the host architecture. |
Yes, as a user, if you want to point to a preexisting executable, you'd still set it in
Right - yes, that's also what I meant with this possibly being brittle. But this seems to assume that everybody's cross builds are done in some specific sequence, with regards to how The safest, with regards to cross builds, probably is to default to not enabling this when cross compiling, unless either
Yes, but it's also (theoretically) possibly to be doing a universal build for 2 architectures, while neither of them is the native host one; so you can't generally assume that one of them is the host architecture. So for that case it's probably fine to just pick the first/any architecture out of the ones included. |
Noted, I'll change how we set the variable for the tool.
Given how many configurations there are to account for here this does look to be the best option.
Hm, I hadn't thought about it that way. In that case then we can modify this to process files for the first architecture for the header files. |
On some cross compile builds, the tool is not correctly set and the command that runs the tool has an empty path for the tool, causing the build to fail. We can setup the tool as a build host tool using setup_host_tool.
chelcassanova
force-pushed
the
fix-lldb-rpc-build-failures
branch
from
482c3f6 to
75ddbac
Compare
LLVM_HOST_TRIPLE and LLVM_DEFAULT_TARGET_TRIPLE is not for detecting the cross builds. This is necessary to detect the Clang cross toolchain build. This is "one-step" build that builds the host tools (non cross builds), such as clang/clang++/llvm-objcopy/etc, and the target libraries using just-built Clang compiler, such as compiler_rt/libc++/libc++abi/libunwind. The libraries will be cross compiled in case LLVM_HOST_TRIPLE != LLVM_DEFAULT_TARGET_TRIPLE. So, the simple CMAKE_CROSSCOMPILING does not work for detection of that kind of LLVM/Clang build configurations. |
|
@chelcassanova the latest changes have the same problem with the header files for the cross toolchain builds. |
Right - I see. But why does this build configuration have to affect whether it should build lldb-rpc-gen or not? Is it because the clang in this build can't manage to process the lldb headers (which are built for the current host)? |
Right, "the clang in this build can't manage to process the lldb headers" because they include the target c++ headers, which are not available yet on a moment of executing the lldb-rpc-gen tool during the cross-toolchain build.
They should work for the target host also. A little more details:
in addition:
Something like that. PS. Probably it is optimal to stop trying to detect all these known and unknown situations and manage building over |
Hmm, that's very odd. At the point when
Actually IMO the issue is the other way around; if the host build doesn't use libc++ headers, then the lldb-rpc-gen is supposed to parse headers in exactly the same way as the host build as well, not waiting for some target runtimes to be built.
That's maybe one indication, yes... But as far as I can see, this really boils down to something else; it's not so much about cross vs not cross, it's about "can the currently built clang libraries be used for host compilation/parsing" - and target specific defaults like Specifically the case about cross compiling LLVM itself, is another factor though, because if we'd enable building
Yes, I agree about that. |
4 participants
On some cross compile builds, the tool is not correctly set and the command that runs the tool has an empty path for the tool, causing the build to fail. We can setup the tool as a build host tool using setup_host_tool.