Page MenuHomePhabricator

Fix LLDB Android AArch64 gcc debug info build
ClosedPublic

Authored by omjavaid on Jan 23 2017, 11:28 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

omjavaid created this revision.Jan 23 2017, 11:28 AM
labath edited edge metadata.

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 updated this revision to Diff 85548.Jan 24 2017, 2:00 AM

Guarding against HAVE_DLOPEN now. Works fine.

labath accepted this revision.Jan 24 2017, 3:52 AM

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

This revision is now accepted and ready to land.Jan 24 2017, 3:52 AM
joerg edited edge metadata.Jan 24 2017, 6:13 AM

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 added a comment.Jan 24 2017, 6:14 AM

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

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.

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 updated this revision to Diff 86407.Jan 31 2017, 3:55 AM

@labath

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

Good to go?

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 ↗(On Diff #86407)

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

joerg added a comment.Jan 31 2017, 6:51 AM

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 ↗(On Diff #86407)

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

This revision was automatically updated to reflect the committed changes.