HomePhabricator

Default to linking lldb-server statically for Android.
AuditedrL242318

Description

Default to linking lldb-server statically for Android.

Reviewers: vharron, tberghammer

Subscribers: chaoren, labath, tberghammer, lldb-commits

Differential Revision: http://reviews.llvm.org/D10858

Event Timeline

tberghammer raised a concern with this commit.Jul 16 2015, 7:13 AM
tberghammer added a subscriber: tberghammer.

I reverted this CL because it causes lldb-server crash on the android builder at startup (resulting in 100% test failure).

The issue happens when you run lldb-platform with "--listen localhost:5432". If you run it with "--listen *:5432" or with "--listen 127.0.0.1:5432" then everything works fine.

Backtrace of the crash:

* thread #1: tid = 24092, 0x01081518 lldb-server`freeaddrinfo(ai=0x00000000) + 4 at getaddrinfo.c:331, name = 'lldb-server', stop reason = signal SIGSEGV: invalid address (fault address: 0x14)
  * frame #0: 0x01081518 lldb-server`freeaddrinfo(ai=0x00000000) + 4 at getaddrinfo.c:331
    frame #1: 0x000a4c80 lldb-server`lldb_private::SocketAddress::getaddrinfo(char const*, char const*, int, int, int, int) + 156
    frame #2: 0x000a4570 lldb-server`lldb_private::Socket::BlockingAccept(llvm::StringRef, bool, lldb_private::Socket*&) + 768
    frame #3: 0x0007f970 lldb-server`main_platform(int, char**) + 1812
    frame #4: 0x000791dc lldb-server`main + 68
    frame #5: 0x01091f0e lldb-server`::__libc_init(raw_args=<unavailable>, onexit=<unavailable>, slingshot=0x00079198, structors=0x00000000)(), int (*)(int, char **, char **), const structors_array_t *const) + 166 at libc_init_static.cpp:111
    frame #6: 0x0007cb98 lldb-server`_start + 92

Logs from logcat:
07-16 07:08:40.896 197 24563 W SocketClient: write error (Broken pipe)
07-16 07:08:40.896 197 24563 W DnsProxyListener: Error writing DNS result to client

I'm wondering if we really need to support localhost (or any other
hostname) on Android. It's really unnecessary for our uses (also bad
security to begin with). We could update the builder to use 127.0.0.1. What
do you think?

chaoren accepted this commit.Jul 16 2015, 4:54 PM

Re-landed after update to buildbot: rL242488.

The buildbot still failing because of this change but I hope it will be fixed when the master is restarted.

Generally I have 2 concern with this change even if the buildbot is modified:

  • As a user I would most likely try to run lldb-server with "--listen localhost:5432" and expect it to work. We can say we don't accept it, but then a meaningful error message should be displayed instead of a segfault. The other option is to change the command line arguments to specify the port and the host separately, and disallow the specification of the host on android (I think it is nice to have it for Linux)
  • I believe we hit this issue because we statically linking against an incompatible version of API level. The current issue can be worked around easily but I wouldn't be surprised if we will hit other similar issues. From my point of view this static linking exposes too much risk because to be confident with the stability of lldb-server we have to test it with each API level what is not sustainable (practically any networking function can fail when we using incompatible API levels).

We can say we don't accept it, but then a meaningful error message should

be displayed instead of a segfault.

Yes, the segfault is caused by freeing a null addrinfo*. After I land
D11285, it should give an actual error message. Static linking with API 9
gives no errors on either API 9 or 21, so I think this issue should be
fixable in the toolchain (b.android.com/180408).

From my point of view this static linking exposes too much risk because

to be confident with the stability of lldb-server we have to test it with
each API level what is not sustainable.

There is going to be this risk no matter what we went with (dynamic linking
or static linking against API 9), since, if I understood correctly,
networking is neither to guaranteed to be forward nor backwards compatible.
We could have one lldb-server linked against each API level, but then we'd
need to deal with bugs from all versions of bionic. This is the least worst
approach, according to enh.
tberghammer added a comment.

The buildbot still failing because of this change but I hope it will be
fixed when the master is restarted.

Generally I have 2 concern with this change even if the buildbot is
modified:

  • As a user I would most likely try to run lldb-server with "--listen

localhost:5432" and expect it to work. We can say we don't accept it, but
then a meaningful error message should be displayed instead of a segfault.
The other option is to change the command line arguments to specify the
port and the host separately, and disallow the specification of the host on
android (I think it is nice to have it for Linux)

  • I believe we hit this issue because we statically linking against an

incompatible version of API level. The current issue can be worked around
easily but I wouldn't be surprised if we will hit other similar issues.
From my point of view this static linking exposes too much risk because to
be confident with the stability of lldb-server we have to test it with each
API level what is not sustainable (practically any networking function can
fail when we using incompatible API levels).

Users:

chaoren (Author, Auditor)
tberghammer (Auditor)

http://reviews.llvm.org/rL242318

Also, yes the build bot will be failing until the master restarts and we'll
need to build lldb-server against the latest API, but the inferiors need to
be linked dynamically against compatible API levels.

chaoren accepted this commit.Jul 29 2015, 2:29 PM
tberghammer accepted this commit.Jul 31 2015, 7:34 AM