Fix LLDB Android AArch64 gcc debug info build

[omjavaid]

Summary

lldb-server android aarch64 build fails to link with following error:

/home/omair/work/lldb-dev/llvm/lib/Support/Unix/Signals.inc:419: undefined reference to `dladdr'
/home/omair/work/lldb-dev/llvm/lib/Support/Unix/Signals.inc:431: undefined reference to `dladdr'

I am using gcc-4.9 from Android NDK-r13b.

This patch fixes the issue.

[labath]

The way this was fixed in https://reviews.llvm.org/D26504 was by guarding the code with HAVE_DLOPEN (I presume I did not catch this usage because it is only enabled it debug builds (?)). I think we should do the same here for consistency. Can you check if that will do the trick?

+Joerg, who reviewed the previous patch.

[omjavaid]

Guarding against HAVE_DLOPEN now. Works fine.

[labath]

Looks fine to me. Joerg, do you have any thoughts on this?

[joerg]

Two comments on this. First, I think we should invert the conditionals and prefer backtrace_symbols_fd.
That one will work in some situations where dladdr doesn't, e.g. if the main binary is not stripped but was linked without -rdynamic.

The second comment is that HAVE_DLOPEN is the wrong check, since the code is only using dladdr. While the result is likely to be the same, CMakefile.txt should look for dladdr.

[joerg]

One more comment: the same check should be used in Path.inc line 183.

[omjavaid]

Thanks @joerg

So I have given this a thought and have a inclination towards sticking to my first patch that was testing for following:

#if HAVE_DLFCN_H && GNUG && !defined(CYGWIN) && !defined(ANDROID)

dlfcn.h not always defines dladdr one such case is ANDROID and we can safely skip that by appending !defined(ANDROID) to line 415 like we are skipping for cygwin.

But reversing conditions will actually mean that above code will never be run at all as we can only run the code once dlfcn.h is present but we ll be checking against that.

Also we need not change anything in path.inc for now because we are already skipping dladdr as we are checking for linux on line 170 which is defined by Anroid and Linux gcc.

I dont understand the code to make more than a trivial change here.

[labath]

In D29042#656858, @omjavaid wrote:

dlfcn.h not always defines dladdr one such case is ANDROID and we can safely skip that by appending !defined(ANDROID) to line 415 like we are skipping for cygwin.

dladdr is not available on android only in case of a fully static build (a practice, which is actually a bit discouraged by the android ndk). If you put #ifdef __ANDROID__ here, you will disable the dladdr even in situations where it would work perfectly well, so I am against that. Adding a proper build-time check for dladdr will probably also allow us to remove the if(CYGWIN) and avoid adding more && !defined(FOO) branches here in the future.

[omjavaid]

@labath

I have added look up for dladdr and it works for android debug info build.

Good to go?

[labath]

Looks fine to me, but it's joerg who we should be asking that. Just make sure the dladdr code stays enabled on targets that support it (e.g., try it on linux host).

My feeling is that switching of priorities between backtrace_symbols_fd and dladdr would be better left for a separate patch. For example, one argument against doing the switch could be that backtrace_symbols_fd does not respect the output stream (it always prints to stderr, whereas, the dladdr branch prints to OS.

lib/Support/Unix/Path.inc

183

You need to add this symbol to include/llvm/Config/config.h.cmake, otherwise this will never be true.

[joerg]

Yeah, switching the branches can be done separately. That's fine. Otherwise, this is starting to look like a proper simplification, so thumbs up.

lib/Support/Unix/Signals.inc

415

Why the check for GNUish C++ here? I think presence of dladdr is all that matters.

cmake/config-ix.cmake

if( PURE_WINDOWS )
   check_function_exists(__main HAVE___MAIN)
   check_function_exists(__cmpdi2 HAVE___CMPDI2)
 endif()
 if( HAVE_DLFCN_H )
   if( HAVE_LIBDL )
     list(APPEND CMAKE_REQUIRED_LIBRARIES dl)
   endif()
   check_symbol_exists(dlopen dlfcn.h HAVE_DLOPEN)
+  check_symbol_exists(dladdr dlfcn.h HAVE_DLADDR)
   if( HAVE_LIBDL )
     list(REMOVE_ITEM CMAKE_REQUIRED_LIBRARIES dl)
   endif()
 endif()
 
 check_symbol_exists(__GLIBC__ stdio.h LLVM_USING_GLIBC)
 if( LLVM_USING_GLIBC )
   add_llvm_definitions( -D_GNU_SOURCE )

lib/Support/Unix/Path.inc

if (sys::fs::exists(aPath)) {
       ssize_t len = readlink(aPath.str().c_str(), exe_path, sizeof(exe_path));
       if (len >= 0)
           return std::string(exe_path, len);
   } else {
       // Fall back to the classical detection.
       if (getprogpath(exe_path, argv0))
         return exe_path;
   }
-#elif defined(HAVE_DLFCN_H)
+#elif defined(HAVE_DLFCN_H) && defined(HAVE_DLADDR)
   // Use dladdr to get executable path if available.
   Dl_info DLInfo;
   int err = dladdr(MainAddr, &DLInfo);
   if (err == 0)
     return "";
 
   // If the filename is a symlink, we need to resolve and return the location of
   // the actual executable.

lib/Support/Unix/Signals.inc

if (!depth)
     depth = unwindBacktrace(StackTrace,
                         static_cast<int>(array_lengthof(StackTrace)));
 #endif
   if (!depth)
     return;
 
   if (printSymbolizedStackTrace(Argv0, StackTrace, depth, OS))
     return;
-#if HAVE_DLFCN_H && __GNUG__ && !defined(__CYGWIN__)
+#if HAVE_DLFCN_H && HAVE_DLADDR && __GNUG__
   int width = 0;
   for (int i = 0; i < depth; ++i) {
     Dl_info dlinfo;
     dladdr(StackTrace[i], &dlinfo);
     const char* name = strrchr(dlinfo.dli_fname, '/');
 
     int nwidth;
     if (!name) nwidth = strlen(dlinfo.dli_fname);