[lldb] Add terminfo dependency for ncurses support

[ajordanr-google]

For some operating systems (e.g. chromiumos), terminfo is a separate
package and library from ncurses. Both are still requirements for curses
support in lldb, individually.

This is a rework of this original spack commit:
spack/spack@9ea2612

Instead though, this PR uses CMake to detect whether the symbol is present
and defined in the curses library, and only falls back to a separate tinfo
if not found.

Without this fix, LLDB cannot be built on these systems.

Fixes #101368

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jordan R AW (ajordanr-google)

Changes

For some operating systems, terminfo is a separate package and library from ncurses. Both are still requirements for curses support in lldb, individually.

This is a rebase of this original spack commit:
spack/spack@9ea2612

Fixes #101368

---

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

1 Files Affected:

• (modified) lldb/cmake/modules/FindCursesAndPanel.cmake (+8-4)

diff --git a/lldb/cmake/modules/FindCursesAndPanel.cmake b/lldb/cmake/modules/FindCursesAndPanel.cmake
index aaadf214bf54b..df4980cc5e0d1 100644
--- a/lldb/cmake/modules/FindCursesAndPanel.cmake
+++ b/lldb/cmake/modules/FindCursesAndPanel.cmake
@@ -2,12 +2,15 @@
 # FindCursesAndPanel
 # -----------
 #
-# Find the curses and panel library as a whole.
+# Find the curses, terminfo, and panel library as a whole.
+# NOTE: terminfo and curses libraries are required separately, as
+# some systems do not bundle them together.
 
-if(CURSES_INCLUDE_DIRS AND CURSES_LIBRARIES AND PANEL_LIBRARIES)
+if(CURSES_INCLUDE_DIRS AND CURSES_LIBRARIES AND TINFO_LIBRARIES AND PANEL_LIBRARIES)
   set(CURSESANDPANEL_FOUND TRUE)
 else()
   find_package(Curses QUIET)
+  find_package(TINFO_LIBRARIES NAMES tinfo DOC "The curses tinfo library" QUIET)
   find_library(PANEL_LIBRARIES NAMES panel DOC "The curses panel library" QUIET)
   include(FindPackageHandleStandardArgs)
   find_package_handle_standard_args(CursesAndPanel
@@ -16,9 +19,10 @@ else()
                                     REQUIRED_VARS
                                       CURSES_INCLUDE_DIRS
                                       CURSES_LIBRARIES
+                                      TINFO_LIBRARIES
                                       PANEL_LIBRARIES)
-  if(CURSES_FOUND AND PANEL_LIBRARIES)
-    mark_as_advanced(CURSES_INCLUDE_DIRS CURSES_LIBRARIES PANEL_LIBRARIES)
+  if(CURSES_FOUND AND TINFO_LIBRARIES AND PANEL_LIBRARIES)
+    mark_as_advanced(CURSES_INCLUDE_DIRS CURSES_LIBRARIES TINFO_LIBRARIES PANEL_LIBRARIES)
   endif()
 endif()
 

[ajordanr-google]

Example downstream bug report: https://issuetracker.google.com/369219560 LLVM Next: lldb ncurses linking dependency, error: undefined symbol: acs_map, stdscr, curs_set, keypad, define_key, which this PR fixes.

[JDevlieghere]

• This isn't using TINFO_LIBRARIES anywhere. Presumably you'll need to add it to LLDB_CURSES_LIBS like the original spack patch.
• This doesn't work on systems that have ncruses and panel, but no separate terminfo library. The purpose of the CMake module is to find ncurses and panel as a unit, and we don't want to fail if terminfo is part of ncurses, which is what happens with the current patch. I don't know if there's a way to set optional variables through (at least not at first glance), in which case we might need to auto-detect it with add_optional_dependency and a separate module for finding it. The auto-detection should make that work out of the box on all platforms and platforms that know they need terminfo can set the CMake flag to "on" to force a configuration time warning if it's not found.

[ajordanr-google]

This isn't using TINFO_LIBRARIES anywhere. Presumably you'll need to add it to LLDB_CURSES_LIBS like the original spack patch.

Hmm. This patch does seem to work locally, so it's not required. I would have thought the logic is entirely in the find_package invocation, and the variables are just for the if check. But no issue adding it to the LLDB_CURSES_LIBS, will do that now!

---

I understand the issue in point two--this patch may unnecessarily require a separate tinfo library even when the headers and objects are available via ncurses. But could you explain more on your recommended solutions, particularly on why we would need to set an optional variable?

If we're going to require a CMAKE flag anyways to show a warning, why not just guard it on a LLDB_USE_SEPARATE_TERMINFO=ON flag and not worry about autodetection? It's already breaking with a link time error, surely that's a louder signal than a configure-time warning.

[JDevlieghere]

I understand the issue in point two--this patch may unnecessarily require a separate tinfo library even when the headers and objects are available via ncurses. But could you explain more on your recommended solutions, particularly on why we would need to set an optional variable?

If we're going to require a CMAKE flag anyways to show a warning, why not just guard it on a LLDB_USE_SEPARATE_TERMINFO=ON flag and not worry about autodetection? It's already breaking with a link time error, surely that's a louder signal than a configure-time warning.

Yes, you could do that, but then platforms that have a separate terminfo library need to remember to turn this on. If they don't they'll hit the link time warning you mentioned and then need to remember or find the appropriate variable. The advantage of the auto detection is that it works out of the box: if CMake can find the library, we'll link against it, if not then we won't. Anyway, doing that means adding a module to find it, and maybe that's overkill for terminfo as it presumably only affects a small subset of platforms. So I'm fine with either.

[ajordanr-google]

Thanks, this does clear it up! Yeah I'd rather have it work out of the box.

I'll see what I can do for doing the autodetection ergonomically, and get back to you. If not, I'll add a flag.

[ajordanr-google]

Tested with a system that has bundled terminfo in curses and one without locally!

[JDevlieghere]

LGTM. Thanks for bearing with me!

[ajordanr-google]

This apparently broke a downstream edge case with fuchsia: https://issues.fuchsia.dev/397455029, where they set all but the TINFO_LIBRARIES variable.

I have given them a downstream fix, but wondering if I can rework the patch so that this isn't an issue. I don't think we need to revert, given this only happens with edge-case defines and it's easily fixed downstream by setting TINFO_LIBRARIES = CURSES_LIBRARIES.

I'm thinking we could make it easier for folks who hit similar errors by doing one of the following:

1. Checking that when CURSES_LIBRARIES is manually set, see if it has the symbols. If not, then check if TINFO_LIBRARIES is set. Error out if not.
2. Don't check for the existence of TINFO_LIBRARIES at all, assume CURSES_LIBRARIES is set "correctly" for their use case. This can work as long as it's clearly stated that's what one must do if CURSES_LIBRARIES is passed.

I don't really have a preference here.

I also notice with this commit that we're changing CURSES_LIBRARIES to be a path to a list of paths. Not sure if anyone depends on that fact? Do we care?

[JDevlieghere]

Okay, I think I understand what's going on with fuchsia: they manually set CURSES_INCLUDE_DIRS, CURSES_LIBRARIES, and PANEL_LIBRARIES. On line 17 we go into the else-case because TINFO_LIBRARIES is not set.

(As an aside, while I'm reading this: why is this checking TINFO_LIBRARIES and not HAS_TERMINFO_SYMBOLS, that's what this package sets. I don't think we should be mentioning TINFO_LIBRARIES at all, unless we know someone is setting that manually?)

So next we check if the provided CURSES_LIBRARIES has our symbol. Presumably for fuchsia, this fails and that's why we try to look for tinfo with CMake? But if CMake can't find it, and they didn't specify it, then isn't this supposed to fail? I guess I'm still missing something.

I also notice with this commit that we're changing CURSES_LIBRARIES to be a path to a list of paths. Not sure if anyone depends on that fact? Do we care?

It's fine (and common) for CURSES_LIBRARIES to contain multiple libraries, but I don't think it's commonly a list. CMake is weird in that everything is a string and we might just get away with it, but it's probably better to do:

set(CURSES_LIBRARIES "${CURSES_LIBRARIES } ${TINFO_LIBRARIES}")

[ajordanr-google]

Okay, I think I understand what's going on with fuchsia: they manually set CURSES_INCLUDE_DIRS, CURSES_LIBRARIES, and PANEL_LIBRARIES. On line 17 we go into the else-case because TINFO_LIBRARIES is not set.

That's correct!

So next we check if the provided CURSES_LIBRARIES has our symbol...

I believe they don't even get this far. They apparently have an issue with find_package/find_library lookup mechanism, and that fails. So it breaks then without even trying to check for the symbol.

(As an aside, while I'm reading this: why is this checking TINFO_LIBRARIES and not HAS_TERMINFO_SYMBOLS, that's what this package sets. I don't think we should be mentioning TINFO_LIBRARIES at all, unless we know someone is setting that manually?)

Mostly this variable was derived from the spack commit which uses it. That's the only place I know it exists. Technically now fuchsia is using it too, after they implemented the fix (which also logically makes some sense, TINFO_LIBRARIES in their case IS CURSES_LIBRARIES.

---

If we were to hide TINFO_LIBRARIES entirely in the conditional, we must check CURSES_LIBRARIES to see if what's passed has the necessary terminfo symbols (or just tell people it needs to be there). I'm fine with that. We shouldn't need to check HAS_TERMINFO_SYMBOLS if they're providing this all directly.

---

Cool cool, will do about the CURSES_LIBRARIES update.

[ajordanr-google]

Turns out set(CURSES_LIBRARIES "${CURSES_LIBRARIES } ${TINFO_LIBRARIES}") definitely does NOT work. I think there's some string quoting which gives:

ninja -v -j96 -l999 distribution libclang.so                                                                                                                                                                                                                                           
ninja: error: '/usr/lib/libform.so /usr/lib/libtinfo.so', needed by 'lib64/liblldb.so.20.0.0git', missing and no known rule to make it
 * ERROR: sys-devel/llvm-9999::chromiumos failed (compile phase):

Both libform.so and libtinfo.so exist at those locations, but obviously the joint path with a space does not.

Using list(APPEND *_LIBRARIES ${...}) is pretty common elsewhere throughout llvm-project, so I think this is normal. (e.g. mlir/cmake/modules/FindLevelZero.cmake, libunwind/cmake/Modules/HandleLibunwindFlags.cmake, etc.). Also LINK_LIBS in add_lldb_library is a list, so it should work out.

Everything else I think have working in a new patch. Will upload now.

[DavidSpickett]

If this and the follow up are working well, consider backporting them to 20.x.

[ajordanr-google]

We haven't had any issues since on our end. I'll check with Fuchsia again to be sure, then will try to backport.

[ajordanr-google]

/cherry-pick 8d017e6 eec242a

[llvmbot]

/cherry-pick 8d017e6 eec242a

Error: Command failed due to missing milestone.

[ajordanr-google]

/cherry-pick 8fff0c1 bb6a273

[llvmbot]

/pull-request #129342