This is an archive of the discontinued LLVM Phabricator instance.

Make llgs build on Android. No functionality change.
ClosedPublic

Authored by endlessroad on Sep 25 2014, 1:28 PM.

Details

Reviewers
tfiala

Diff Detail

Event Timeline

endlessroad retitled this revision from to Make llgs build on Android. No functionality changes..
endlessroad updated this object.
endlessroad edited the test plan for this revision. (Show Details)
endlessroad added a reviewer: tfiala.
endlessroad added a subscriber: Unknown Object (MLST).
tfiala edited edge metadata.Sep 25 2014, 1:33 PM

Just to clarify for others and give context, Tong:

These are changes that allow llgs to build, within the AOSP source tree, using the Android.mk build system, when using the llvm/clang/lldb git repos from AOSP replaced with the experimental ones in github.com/tfiala/aosp-{llvm,clang,lldb}. Is that right, Tong?

In D5495#4, @tfiala wrote:

Just to clarify for others and give context, Tong:

These are changes that allow llgs to build, within the AOSP source tree, using the Android.mk build system, when using the llvm/clang/lldb git repos from AOSP replaced with the experimental ones in github.com/tfiala/aosp-{llvm,clang,lldb}. Is that right, Tong?

Yep :-) (also needs github.com/tfiala/aosp-compiler-rt)

These are changes that allow llgs to build, within the AOSP source tree, using the Android.mk build system, when using the llvm/clang/lldb git repos from AOSP replaced with the experimental ones in github.com/tfiala/aosp-{llvm,clang,lldb}. Is that right, Tong?

Missed one - and aosp-compiler-rt.

Correct, Tong?

Haha okay, jinx :-)

endlessroad edited edge metadata.

LGTM.

We might later want to look at how/where we're constructing IOHandlerEditLine. It seems like it might first show up at too low a level of the stack based on your need to comment it out (unneeded in llgs and not wanting to bring in the libedit dependency).

I'll ping Greg to see if he wants somebody to look over before we approve and move this in.

I don't have time to look at this right now, but that's an awful lot of #ifndef ANDROID in places I wouldn't really expect these defines. Particularly the ones that deal with the IOHandlerEditLine. Either you're building stuff for llgs that you don't really need to (why does it need the command interpreter) or we really need a non-interactive configuration for the command interpreter that substitutes the EditLine I/O handler with one that is not interactive... Anyway, that part does seem ugly to me.

Also one of the uses of ANDROID is to not include some python goo, but there's already a "build without python" define for that purpose.

Jim

I think part of our challenge is when we try to build remote pieces with
what is effectively lldb-core, we really could use at least a 2-layer
division of the core. The true low-level goo in a core, and some
higher-level bits that only the lldb client really needs but isn't needed
for a debug stub/monitor.

Absolutely true :-)

But in my remaining time I'm afraid that's not gonna happen...
So we can only do ANDROID tricks first, and then refactor...

Yikes, I really don't like the "#ifndef ANDROID" stuff everywhere. That will encourage others to do the same kind of thing. We should make a LLDB_NO_EDITLINE or some other define.

the IOHandlerEditline class does have support for non-editline based input. It uses this when the input file handle isn't interactive. You should actually leave the IOHandlerEditline class in and use the LLDB_NO_EDITLINE define to leave the actual editline based code out and always for it to use the file descriptor methods.

then the changes in files that were disabling the IOHandlerEditline are not needed.

size_t
FileSpec::ResolvePartialUsername (const char *partial_name, StringList &matches)
{
#if defined(LLDB_CONFIG_TILDE_RESOLVES_TO_USER) && !defined(ANDROID)

size_t extant_entries = matches.GetSize();

setpwent();

This:

#ifdef ANDROID
// Android does not have SUN_LEN
#ifndef SUN_LEN
#define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path) + strlen((ptr)->sun_path))
#endif
#endif

Can just become:

#ifndef SUN_LEN
#define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path) + strlen((ptr)->sun_path))
#endif

But this should probably be only moved to the files that need it. "Host.h" seems like too public of a place for this kind of specific stuff.

All #include changes where a more descriptive path are given are fine.

The changes in lldb-gdbserver.cpp:

#ifndef ANDROID
#include "lldb/lldb-python.h"
#endif

Should be removed and us the #define that disables python.

Why do we need to #include <spawn.h> in "ProcessGDBRemote.cpp"? Seems like we shouldn't need that anymore and the change in ProcessGDBRemote.cpp should be removed.

The change in FileSpec.cpp:

#if defined(LLDB_CONFIG_TILDE_RESOLVES_TO_USER) && !defined(ANDROID)

should not be here. The LLDB_CONFIG_TILDE_RESOLVES_TO_USER should not be defined for android builds.

All preprocessor check in common/Host.cpp are ok since this is a host file. But in general, there should be minimal android checks in non-host code.

Check all places that are including <spawn.h> and try to reroute these to use Host code. I am guessing a lot of places that include it no longer need to.

Sorry Tong, should have looked more closely at the Python part --- I thought we were already excluding Python and some of these fell out after that.

RE: this patch or follow up, we don't want a quick check-in that needs a rework, let's get it right the first time. (I see you're already working on that).

Basically Greg's comments :-)

Thanks, Tong!

zturner added inline comments.
source/Host/common/Host.cpp
978–980

If launching a process via posix_spawn() isn't supported on Android, then how *do* you launch a process on Android?

source/Host/posix/HostInfoPosix.cpp
98–100 ↗(On Diff #14085)

Is this unsupported on Android because it doesn't compile, or for some other reason? Also, Android implies Linux right? If so, this check is better in HostInfoLinux

source/Host/posix/HostThreadPosix.cpp
53–58 ↗(On Diff #14085)

Same as before. Is this unsupported because it doesn't compile, or for some other reason? Also more appropriate in HostThreadLinux.

I also want to echo all the comments that I really don't like the #ifdefs everywhere. I've been working for the past 2 months specifically to get rid of #ifdefs, so this is like a punch in the stomach :(

Eventually there should be separate files for Android.

I just want to post a quick patch to let it build on Android first.

Cleanups on the way.

source/Host/common/Host.cpp
978–980

No launching; attaching only.

source/Host/posix/HostInfoPosix.cpp
98–100 ↗(On Diff #14085)

Android uses its own C library, which does not provide getgrgid_r()

source/Host/posix/HostThreadPosix.cpp
53–58 ↗(On Diff #14085)

Same as before

Eventually there should be separate files for Android.

I just want to post a quick patch to let it build on Android first.

Cleanups on the way.

I'll defer to Greg and Jim about whether they will accept all the #ifdefs. But regardless, assuming Android implies Linux (which I believe it does) please move the Android specific checks to HostThreadLinux and HostInfoLinux.

endlessroad retitled this revision from Make llgs build on Android. No functionality changes. to Make llgs build on Android. No functionality change..
  • Add macro LLDB_DISABLE_LIBEDIT instead of using "#ifndef ANDROID" in IOHandler related changes
  • Remove some unused include (<spawn.h>, "lldb-python.h")
  • Unset LLDB_CONFIG_TILDE_RESOLVES_TO_USER for android
  • Move android specific SUN_LEN definition to source/Host/Socket.cpp

I'm not seeing the changes to HostInfoPosix and HostThreadPosix that I
suggested. Are those still coming in a followup?

In D5495#27, @zturner wrote:

I'm not seeing the changes to HostInfoPosix and HostThreadPosix that I
suggested. Are those still coming in a followup?

With Android, it's *mostly* Linux (at the kernel level) except we have a whole slew of different runtime libraries that differ. So, for much of the behavior, it will be like Linux/POSIX, but with certain libc calls and whatnot that just don't exist. Ideally we minimize how much we differ and only when needed.

Tong, can you take a look at that? If we need a Linux-derived Android variant of these classes, this might be a good time to look at those. Thanks.

(paraphrased) how do you launch on Android, then?

In general you don't. The Android zygote takes care of launching for userland, and debuggers usually run attach-only.

That's just a half-truth, though. We do have low-level, internal capabilities for launching, but since that is not a supported path for developers, they are not exposed in a way we can access with POSIX-like calls. The launch code might end up only showing up in an AOSP environment but, in any event, the attach path is the crucial one for Android right now.

Add Host{Info, Thread}Android. They cannot directly inherit Host{Info, Thread}Posix, because that would need linking the base classes, which will cause linker error (some POSIX functions do not exist on Android)

Add Host{Info, Thread}Android. They cannot directly inherit Host{Info, Thread}Posix, because that would need linking the base classes, which will cause linker error (some POSIX functions do not exist on Android)

It didn't occur to me that there would be linker errors. As a result, I think your original solution of putting the #ifdef in Host[Info/Thread]Posix.cpp is probably better than this. This results in a lot of code duplication as well, I'm fine with undoing these changes and going back to your original patch. Sorry for the trouble.

Reupload DIff version 4.

Hi Jim, Greg,

All your original comments have been addressed.

I'll review and get these in over the weekend unless I hear major objections.

-Todd

Hey Tong,

What are all the flags you assume are set when building this on the Android side? In addition to building it with stock normal flags on Linux, I want to use the same flag set.

Looks like the define flag set includes at least:
LLDB_DISABLE_LIBEDIT
LLDB_DISABLE_PYTHON
(and of course from the compiler, ANDROID).

Anything else?

-Todd

Hey Tong,

So I was looking through the LLVM/Clang code at their LIBEDIT support (which I vaguely remember going in or being tweaked in the last half year or so). There is a HAVE_LIBEDIT flag that is defined if libedit is present. While it makes sense for us to be able to disable lldb's libedit support when HAVE_LIBEDIT is true, we certainly *do* want to turn on LLDB_DISABLE_LIBEDIT when HAVE_LIBEDIT is not defined.

I think that can happen as a follow-up patch. It would be great if you could work that in later.

tfiala accepted this revision.Sep 27 2014, 9:59 AM
tfiala edited edge metadata.

LGTM. Much cleaner than first pass. Thanks, Tong, and thanks others for the constructive feedback!

This revision is now accepted and ready to land.Sep 27 2014, 9:59 AM
tfiala closed this revision.Sep 27 2014, 10:04 AM

svn commit
Sending include/lldb/Core/IOHandler.h
Sending include/lldb/Host/linux/Config.h
Sending source/Core/IOHandler.cpp
Sending source/Host/common/Host.cpp
Sending source/Host/common/Socket.cpp
Sending source/Host/linux/Host.cpp
Sending source/Host/posix/HostInfoPosix.cpp
Sending source/Host/posix/HostThreadPosix.cpp
Sending source/Interpreter/CommandInterpreter.cpp
Sending source/Plugins/Process/Linux/LinuxThread.h
Sending source/Plugins/Process/Linux/NativeProcessLinux.cpp
Sending source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
Sending source/Plugins/Process/Linux/ProcessLinux.cpp
Sending source/Plugins/Process/Linux/ProcessLinux.h
Sending source/Plugins/Process/Linux/ProcessMonitor.cpp
Sending source/Plugins/Process/POSIX/POSIXThread.cpp
Sending source/Plugins/Process/POSIX/POSIXThread.h
Sending source/Plugins/Process/POSIX/ProcessPOSIX.cpp
Sending source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_arm64.cpp
Sending source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_mips64.cpp
Sending source/Plugins/Process/POSIX/RegisterContextPOSIXProcessMonitor_x86.cpp
Sending source/Plugins/Process/Utility/RegisterInfos_arm64.h
Sending source/Plugins/Process/elf-core/RegisterContextPOSIXCore_mips64.cpp
Sending source/Plugins/Process/elf-core/RegisterContextPOSIXCore_x86_64.cpp
Sending source/Plugins/Process/elf-core/ThreadElfCore.cpp
Sending source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Sending source/Target/ProcessLaunchInfo.cpp
Sending source/Target/Thread.cpp
Sending tools/lldb-gdbserver/lldb-gdbserver.cpp
Transmitting file data .............................
Committed revision 218568.