Details
- Reviewers
tfiala
Diff Detail
Event Timeline
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?
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?
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!
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 |
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.
- 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?
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)
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.
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.
LGTM. Much cleaner than first pass. Thanks, Tong, and thanks others for the constructive feedback!
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.
If launching a process via posix_spawn() isn't supported on Android, then how *do* you launch a process on Android?