Differential D8447
Fix for aarch64 lldb-server native builds Authored by omjavaid on Mar 19 2015, 10:07 AM.
Details
Diff Detail Event Timeline
Comment Actions Hi Omair, While it's good to move away from wrong hard-coded values, but we should make it more generic. If you're building all together, there's a CMake function called get_host_triple() at LLVM's cmake/modules/GetHostTriple.cmake that you could use, or just use LLVM_HOST_TRIPLE. If that's wrong, than I think Zach is right that this may have to be fixed on the LLVM side.
Comment Actions You can specify LLVM_HOST_TRIPLE from the command line (e.g.: -DLLVM_HOST_TRIPLE=aarch64-unknown-linux-android) and it have to be specified correctly because it used in some place in lldb-server. I think we should use that one instead of introducing a new variable. Comment Actions Tamas: Renato: Currently it looks something like this: if (NOT LLDB_DISABLE_PYTHON) if(UNIX) > we are bound to come here on any UNIX/Linux system
proper fix. if(CMAKE_SIZEOF_VOID_P EQUAL 8) > And here we are forcing the triple to x86_64-linux-gnu in case weencounter a 64bit target which is wrong. set(CMAKE_LIBRARY_ARCHITECTURE "x86_64-linux-gnu")
endif()
endif()
if (MSVC)
if ("${PYTHON_INCLUDE_DIR}" STREQUAL "" OR "${PYTHON_LIBRARY}" STREQUAL "")
message("-- LLDB Embedded python disabled. Embedding python onWindows requires " "manually specifying PYTHON_INCLUDE_DIR *and* PYTHON_LIBRARY")
set(LLDB_DISABLE_PYTHON 1)
else()
message("-- Found PythonLibs: ${PYTHON_LIBRARY}")
include_directories(${PYTHON_INCLUDE_DIR})
endif()
else()
find_package(PythonLibs REQUIRED)
include_directories(${PYTHON_INCLUDE_DIRS})
endif()endif() Comment Actions This probably means that the LLDB CMake file is not well integrated. I don't know how much LLDB needs to be compiled inside LLVM, but that seems to be the standard for everything else. Does anyone know how to use the CMake modules on other projects?
Yes, this is completely wrong. But instead of guessing, we should be using what LLVM does. AFAIK, LLVM uses that GetHostTriple module to guess the triple, and later on, it uses that if the user hasn't specified in the command line. We should do the exact same thing on all projects, including LLDB. But unfortunately, like you, I know very little of CMake and how it's used in other projects... :( I'm hoping someone around could give us a hand. cheers, Comment Actions I still think this needs to be fixed in LLVM and not in LLDB. CMAKE_LIBRARY_ARCHITECTURE is a variable that any library, not just LLDB, might want to check. And if it's not correct in LLDB, then it's not going to be correct anywhere else either. I've added chandlerc. I know a little about LLVM's CMake, but not as much as Chandler, so maybe he could give us a hand and see if he agrees that this belongs in LLVM CMake. Comment Actions I can't find CMAKE_LIBRARY_ARCHITECTURE anywhere else in the LLVM projects... But since this is a CMAKE variable, not an LLVM one, we may need to set it with the host triple, so that it's correct for all projects that might need it in the future. Omair, What about setting that variable with the result of LLVM_HOST_TRIPLE set at LLVM src/cmake/config-ix.cmake line 324? Maybe adding a line right after it to set should do the trick. Comment Actions As it turns out that this issue only occurs on 64bit host architecture so I am adding this trivial solution with attached updated patch. Comment Actions Hi Omair, It looks good to me, but wait for Zachary's comment before committing. cheers,
Comment Actions Closing the loop here: this is not the correct patch. Zach is completely correct here. If some core part of LLVM (and thus LLDB) is getting the CMAKE_LIBRARY_ARCHITECTURE wrong, we need to go fix *that*. It isn't at all clear why it would be wrong, but this patch is not the correct fix. | ||||||||||||||||||
This is not a proper fix for target identification, but it does solve the more pressing problem of getting a hard-coded triple.