Page MenuHomePhabricator

[llvm][lldb] use FindLibEdit.cmake everywhere
ClosedPublic

Authored by upsj on Apr 29 2022, 7:55 AM.

Details

Summary

Currently, LLVM's LineEditor and LLDB both use libedit, but find them in different (inconsistent) ways.
This causes issues e.g. when you are using a locally installed version of libedit, which will not be used
by clang-query, but by lldb if picked up by FindLibEdit.cmake

Diff Detail

Event Timeline

upsj created this revision.Apr 29 2022, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2022, 7:55 AM
upsj requested review of this revision.Apr 29 2022, 7:55 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 29 2022, 7:55 AM
upsj updated this revision to Diff 426068.Apr 29 2022, 8:38 AM

The previous setup didn't work for clang-query, so I added a CMake IMPORTED target for LibEdit and used it everywhere

upsj updated this revision to Diff 426071.Apr 29 2022, 8:44 AM

fix broken build without libedit, improve find module

upsj updated this revision to Diff 426072.Apr 29 2022, 8:45 AM

revert remaining unnecessary change from initial diff

upsj updated this revision to Diff 426080.Apr 29 2022, 9:02 AM

improve handling of disabled libedit

upsj updated this revision to Diff 426215.Apr 30 2022, 3:40 AM

restore some of the previous behavior

I vaguely recall that some lldb bots use a stand-alone build.

Say I have a build directory /tmp/RelA with -DLLVM_ENABLE_PROJECTS=clang

cmake -GNinja -S~/llvm-project/lldb -Bout/lldb -DCMAKE_PREFIX_PATH=/tmp/RelA

will print

CMake Warning at cmake/modules/LLDBConfig.cmake:52 (find_package):                                                                                                                                                                                                                          
  By not providing "FindLibEdit.cmake" in CMAKE_MODULE_PATH this project has                                                                                                                                                                                                                
  asked CMake to find a package configuration file provided by "LibEdit", but                                                                                                                                                                                                               
  CMake did not find one.                                                                                                                                                                                                                                                                   
                                                                                                                                                                                                                                                                                            
  Could not find a package configuration file provided by "LibEdit" with any                                                                                                                                                                                                                
  of the following names:                                                                                                                                                                                                                                                                   
                                                                                                                                                                                                                                                                                            
    LibEditConfig.cmake                                                                                                                                                                                                                                                                     
    libedit-config.cmake                                                                                                                                                                                                                                                                    
                                                                                                                                                                                                                                                                                            
  Add the installation prefix of "LibEdit" to CMAKE_PREFIX_PATH or set      
  "LibEdit_DIR" to a directory containing one of the above files.  If                                    
  "LibEdit" provides a separate development package or SDK, be sure it has
  been installed.                                  
Call Stack (most recent call first):                                   
  cmake/modules/LLDBConfig.cmake:59 (add_optional_dependency)                                                                                 
  CMakeLists.txt:28 (include)

I wish that https://discourse.llvm.org/t/rfc-stand-alone-build-support/61291/37 will make stand-alone builds duplicate less code.

MaskRay edited reviewers, added: JDevlieghere; removed: MaskRay.Apr 30 2022, 12:44 PM
MaskRay added a subscriber: MaskRay.

I vaguely recall that some lldb bots use a stand-alone build.

Yup, that's correct. The bot in question is https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-standalone/

upsj updated this revision to Diff 426628.May 3 2022, 3:18 AM

fix standalone builds

JDevlieghere added inline comments.May 3 2022, 12:42 PM
lldb/CMakeLists.txt
3–5 ↗(On Diff #426628)

Can this go in lldb/cmake/modules/LLDBStandalone.cmake?

upsj updated this revision to Diff 426983.May 4 2022, 6:19 AM
upsj marked an inline comment as done.

move standalone-specific code to LLDBStandalone.cmake

MaskRay accepted this revision.May 4 2022, 10:00 AM

LGTM.

This revision is now accepted and ready to land.May 4 2022, 10:00 AM
upsj added a comment.May 4 2022, 11:20 PM

I don't have commit access to LLVM, so feel free to merge it

Tobias Ribizel <ribizel@kit.edu>

This revision was landed with ongoing or failed builds.May 12 2022, 3:59 PM
This revision was automatically updated to reflect the committed changes.

Hi, Sorry for the late notification, but I think this change may not apply correctly to all configs.

We're seeing a breakage in Fuchsia's Clang CI builders: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814329895908917697/overview

FAILED: lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o 
/b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/x/w/cipd/bin/clang++ --sysroot=/b/s/w/ir/x/w/cipd/linux -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/staging/llvm_build/lib/LineEditor -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor -I/b/s/w/ir/cache/vpython/79db7c/lib/python3.8/site-packages/tensorflow/include -I/b/s/w/ir/x/w/staging/llvm_build/include -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/include -isystem /b/s/w/ir/x/w/staging/zlib_install/include -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/x/w/staging/llvm_build=../staging/llvm_build -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O3 -DNDEBUG  -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -MF lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o.d -o lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -c /b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp
/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp:18:10: fatal error: 'histedit.h' file not found
#include <histedit.h>
         ^~~~~~~~~~~~

since the error is a missing header for histedit.h, and this change alters how libedit is found in CMake, I assume this is the root cause.

I'm guessing some part of the old CMake checks like the old HAVE_HISTEDIT_H may not have propagated correctly in all cases?

If this will be hard to fix, would you mind reverting until a fix is ready?

Hi, Sorry for the late notification, but I think this change may not apply correctly to all configs.

We're seeing a breakage in Fuchsia's Clang CI builders: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814329895908917697/overview

FAILED: lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o 
/b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/x/w/cipd/bin/clang++ --sysroot=/b/s/w/ir/x/w/cipd/linux -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/staging/llvm_build/lib/LineEditor -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor -I/b/s/w/ir/cache/vpython/79db7c/lib/python3.8/site-packages/tensorflow/include -I/b/s/w/ir/x/w/staging/llvm_build/include -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/include -isystem /b/s/w/ir/x/w/staging/zlib_install/include -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/x/w/staging/llvm_build=../staging/llvm_build -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O3 -DNDEBUG  -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -MF lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o.d -o lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -c /b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp
/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp:18:10: fatal error: 'histedit.h' file not found
#include <histedit.h>
         ^~~~~~~~~~~~

since the error is a missing header for histedit.h, and this change alters how libedit is found in CMake, I assume this is the root cause.

I'm guessing some part of the old CMake checks like the old HAVE_HISTEDIT_H may not have propagated correctly in all cases?

If this will be hard to fix, would you mind reverting until a fix is ready?

It's not clear to me that this patch caused the issue for you, so I don't think it is right to ask for a revert now. You need to provide more evidence.
lldb-x86_64-debian and lldb-cmake-standalone on https://lldb.llvm.org/resources/bots.html work well with the change.

phosek added a subscriber: phosek.EditedMay 13 2022, 10:41 AM

Hi, Sorry for the late notification, but I think this change may not apply correctly to all configs.

We're seeing a breakage in Fuchsia's Clang CI builders: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814329895908917697/overview

FAILED: lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o 
/b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/x/w/cipd/bin/clang++ --sysroot=/b/s/w/ir/x/w/cipd/linux -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/staging/llvm_build/lib/LineEditor -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor -I/b/s/w/ir/cache/vpython/79db7c/lib/python3.8/site-packages/tensorflow/include -I/b/s/w/ir/x/w/staging/llvm_build/include -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/include -isystem /b/s/w/ir/x/w/staging/zlib_install/include -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/x/w/staging/llvm_build=../staging/llvm_build -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O3 -DNDEBUG  -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -MF lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o.d -o lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -c /b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp
/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp:18:10: fatal error: 'histedit.h' file not found
#include <histedit.h>
         ^~~~~~~~~~~~

since the error is a missing header for histedit.h, and this change alters how libedit is found in CMake, I assume this is the root cause.

I'm guessing some part of the old CMake checks like the old HAVE_HISTEDIT_H may not have propagated correctly in all cases?

If this will be hard to fix, would you mind reverting until a fix is ready?

It's not clear to me that this patch caused the issue for you, so I don't think it is right to ask for a revert now. You need to provide more evidence.
lldb-x86_64-debian and lldb-cmake-standalone on https://lldb.llvm.org/resources/bots.html work well with the change.

I looked a bit more into the error. In the CMake log I see the following:

-- Found LibEdit: /usr/include (found version ".")

This is incorrect because in our build we set CMAKE_SYSROOT=/b/s/w/ir/x/w/cipd/linux so CMake shouldn't be looking in paths like /usr/include because at build time, Clang will be invoked with --sysroot=/b/s/w/ir/x/w/cipd/linux and it won't consider include paths like /usr/include.

I don't think this issue was introduced in this change, it's a pre-existing issue with the FindLibEdit.cmake module, but it was uncovered by this change.

Hi, Sorry for the late notification, but I think this change may not apply correctly to all configs.

We're seeing a breakage in Fuchsia's Clang CI builders: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814329895908917697/overview

FAILED: lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o 
/b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/x/w/cipd/bin/clang++ --sysroot=/b/s/w/ir/x/w/cipd/linux -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/staging/llvm_build/lib/LineEditor -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor -I/b/s/w/ir/cache/vpython/79db7c/lib/python3.8/site-packages/tensorflow/include -I/b/s/w/ir/x/w/staging/llvm_build/include -I/b/s/w/ir/x/w/llvm-llvm-project/llvm/include -isystem /b/s/w/ir/x/w/staging/zlib_install/include -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/x/w/staging/llvm_build=../staging/llvm_build -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O3 -DNDEBUG  -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -MF lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o.d -o lib/LineEditor/CMakeFiles/LLVMLineEditor.dir/LineEditor.cpp.o -c /b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp
/b/s/w/ir/x/w/llvm-llvm-project/llvm/lib/LineEditor/LineEditor.cpp:18:10: fatal error: 'histedit.h' file not found
#include <histedit.h>
         ^~~~~~~~~~~~

since the error is a missing header for histedit.h, and this change alters how libedit is found in CMake, I assume this is the root cause.

I'm guessing some part of the old CMake checks like the old HAVE_HISTEDIT_H may not have propagated correctly in all cases?

If this will be hard to fix, would you mind reverting until a fix is ready?

It's not clear to me that this patch caused the issue for you, so I don't think it is right to ask for a revert now. You need to provide more evidence.
lldb-x86_64-debian and lldb-cmake-standalone on https://lldb.llvm.org/resources/bots.html work well with the change.

I looked a bit more into the error. In the CMake log I see the following:

-- Found LibEdit: /usr/include (found version ".")

This is incorrect because in our build we set CMAKE_SYSROOT=/b/s/w/ir/x/w/cipd/linux so CMake shouldn't be looking in paths like /usr/include because at build time, Clang will be invoked with --sysroot=/b/s/w/ir/x/w/cipd/linux and it won't consider include paths like /usr/include.

I don't think this issue was introduced in this change, it's a pre-existing issue with the FindLibEdit.cmake module, but it was uncovered by this change.

If the patch exposes a preexisting lurking bug, reverting it temporarily is fine if that helps you.

It's probably fine to reland this after D125570 lands, since that should unblock us.