[lldb] Remove support and workarounds for Android 4 and older #124047
Conversation
|
@llvm/pr-subscribers-lldb Author: Brad Smith (brad0) ChangesFull diff: https://github.com/llvm/llvm-project/pull/124047.diff 7 Files Affected:
diff --git a/lldb/cmake/modules/LLDBConfig.cmake b/lldb/cmake/modules/LLDBConfig.cmake
index 9bb37f5967d4f3..747f7e6038181c 100644
--- a/lldb/cmake/modules/LLDBConfig.cmake
+++ b/lldb/cmake/modules/LLDBConfig.cmake
@@ -306,9 +306,4 @@ else()
set(LLDB_CAN_USE_DEBUGSERVER OFF)
endif()
-if ((CMAKE_SYSTEM_NAME MATCHES "Android") AND LLVM_BUILD_STATIC AND
- ((ANDROID_ABI MATCHES "armeabi") OR (ANDROID_ABI MATCHES "mips")))
- add_definitions(-DANDROID_USE_ACCEPT_WORKAROUND)
-endif()
-
include(LLDBGenerateConfig)
diff --git a/lldb/include/lldb/Host/Time.h b/lldb/include/lldb/Host/Time.h
index aee4c43247c5a3..2ca5a4026884b7 100644
--- a/lldb/include/lldb/Host/Time.h
+++ b/lldb/include/lldb/Host/Time.h
@@ -11,15 +11,6 @@
#ifndef LLDB_HOST_TIME_H
#define LLDB_HOST_TIME_H
-#ifdef __ANDROID__
-#include <android/api-level.h>
-#endif
-
-#if defined(__ANDROID_API__) && __ANDROID_API__ < 21
-#include <time64.h>
-extern time_t timegm(struct tm *t);
-#else
#include <ctime>
-#endif
#endif // LLDB_HOST_TIME_H
diff --git a/lldb/source/Host/CMakeLists.txt b/lldb/source/Host/CMakeLists.txt
index e0cd8569bf9575..cdfb6184f2219e 100644
--- a/lldb/source/Host/CMakeLists.txt
+++ b/lldb/source/Host/CMakeLists.txt
@@ -113,7 +113,6 @@ else()
if (CMAKE_SYSTEM_NAME MATCHES "Android")
add_host_subdirectory(android
android/HostInfoAndroid.cpp
- android/LibcGlue.cpp
)
endif()
elseif (CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
diff --git a/lldb/source/Host/android/LibcGlue.cpp b/lldb/source/Host/android/LibcGlue.cpp
deleted file mode 100644
index 877d735823feee..00000000000000
--- a/lldb/source/Host/android/LibcGlue.cpp
+++ /dev/null
@@ -1,28 +0,0 @@
-//===-- LibcGlue.cpp ------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// This files adds functions missing from libc on earlier versions of Android
-
-#include <android/api-level.h>
-
-#include <sys/syscall.h>
-
-#if __ANDROID_API__ < 21
-
-#include <csignal>
-#include <fcntl.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-
-#include "lldb/Host/Time.h"
-
-time_t timegm(struct tm *t) { return (time_t)timegm64(t); }
-
-int posix_openpt(int flags) { return open("/dev/ptmx", flags); }
-
-#endif
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index 0ccff41a552068..296c2273ba419c 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -472,23 +472,7 @@ Status Socket::Accept(const Timeout<std::micro> &timeout, Socket *&socket) {
NativeSocket Socket::AcceptSocket(NativeSocket sockfd, struct sockaddr *addr,
socklen_t *addrlen, Status &error) {
error.Clear();
-#if defined(ANDROID_USE_ACCEPT_WORKAROUND)
- // Hack:
- // This enables static linking lldb-server to an API 21 libc, but still
- // having it run on older devices. It is necessary because API 21 libc's
- // implementation of accept() uses the accept4 syscall(), which is not
- // available in older kernels. Using an older libc would fix this issue, but
- // introduce other ones, as the old libraries were quite buggy.
- int fd = syscall(__NR_accept, sockfd, addr, addrlen);
- if (fd >= 0) {
- int flags = ::fcntl(fd, F_GETFD);
- if (flags != -1 && ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC) != -1)
- return fd;
- SetLastError(error);
- close(fd);
- }
- return fd;
-#elif defined(SOCK_CLOEXEC) && defined(HAVE_ACCEPT4)
+#if defined(SOCK_CLOEXEC) && defined(HAVE_ACCEPT4)
int flags = SOCK_CLOEXEC;
NativeSocket fd = llvm::sys::RetryAfterSignal(
static_cast<NativeSocket>(-1), ::accept4, sockfd, addr, addrlen, flags);
diff --git a/lldb/source/Host/posix/HostInfoPosix.cpp b/lldb/source/Host/posix/HostInfoPosix.cpp
index 193f584900b632..23ba3177de317a 100644
--- a/lldb/source/Host/posix/HostInfoPosix.cpp
+++ b/lldb/source/Host/posix/HostInfoPosix.cpp
@@ -86,13 +86,6 @@ std::optional<std::string> HostInfoPosix::GetOSBuildString() {
return std::string(un.release);
}
-#ifdef __ANDROID__
-#include <android/api-level.h>
-#endif
-#if defined(__ANDROID_API__) && __ANDROID_API__ < 21
-#define USE_GETPWUID
-#endif
-
namespace {
class PosixUserIDResolver : public UserIDResolver {
protected:
@@ -107,14 +100,6 @@ struct PasswdEntry {
};
static std::optional<PasswdEntry> GetPassword(id_t uid) {
-#ifdef USE_GETPWUID
- // getpwuid_r is missing from android-9
- // The caller should provide some thread safety by making sure no one calls
- // this function concurrently, because using getpwuid is ultimately not
- // thread-safe as we don't know who else might be calling it.
- if (auto *user_info_ptr = ::getpwuid(uid))
- return PasswdEntry{user_info_ptr->pw_name, user_info_ptr->pw_shell};
-#else
struct passwd user_info;
struct passwd *user_info_ptr = &user_info;
char user_buffer[PATH_MAX];
@@ -124,7 +109,6 @@ static std::optional<PasswdEntry> GetPassword(id_t uid) {
user_info_ptr) {
return PasswdEntry{user_info_ptr->pw_name, user_info_ptr->pw_shell};
}
-#endif
return std::nullopt;
}
diff --git a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
index 7b8b42a4b7fe07..22bf698c71716e 100644
--- a/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
+++ b/lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
@@ -25,13 +25,10 @@
#include <sstream>
#ifdef __ANDROID__
-#include <android/api-level.h>
#define PT_TRACE_ME PTRACE_TRACEME
#endif
-#if defined(__ANDROID_API__) && __ANDROID_API__ < 15
-#include <linux/personality.h>
-#elif defined(__linux__)
+#if defined(__linux__)
#include <sys/personality.h>
#endif
|
|
I'm fine (and very happy) with this if @enh-google (or whoever he nominates) is. I guess this means the whole "single step workaround" (lldb/source/Plugins/Process/Linux/SingleStepCheck.h) could go away as well (maybe make a separate patch for that). |
| @@ -25,13 +25,10 @@ | |||
| #include <sstream> | |||
|
|
|||
| #ifdef __ANDROID__ | |||
There was a problem hiding this comment.
fwiw, you shouldn't need this with any current NDK (and current NDKs support API >= 21). looks like i added this in 2015 to support building gdb/gdbserver out of the box and didn't realize lldb was carrying a workaround: https://android-review.googlesource.com/c/platform/bionic/+/155038
There was a problem hiding this comment.
looking around, i see a few others that aren't true either:
#ifdef __ANDROID__
// Android does not have SUN_LEN
#ifndef SUN_LEN
#define SUN_LEN(ptr) \
(offsetof(struct sockaddr_un, sun_path) + strlen((ptr)->sun_path))
#endif
#endif // #ifdef __ANDROID__
or
#if defined(_WIN32) || defined(__ANDROID__) || defined(_AIX)
// Defines from ar, missing on Windows
for example.
please let us know any time you find yourself needing to add a workaround like this, so we can try to make it "just work".
anything like
#ifdef __ANDROID__
#include <arpa/inet.h>
#include <asm-generic/errno-base.h>
#include <cerrno>
#include <fcntl.h>
#include <linux/tcp.h>
#include <sys/syscall.h>
#include <unistd.h>
#endif // __ANDROID__
is exceptionally weird. any time you're having to include stuff from <asm (and to a lesser extent <linux/) is probably a bug on our side.
i don't suppose anyone knows the history of this?
static Environment::Envp FixupEnvironment(Environment env) {
#ifdef __ANDROID__
// If there is no PATH variable specified inside the environment then set the
// path to /system/bin. It is required because the default path used by
// execve() is wrong on android.
env.try_emplace("PATH", "/system/bin");
#endif
return env.getEnvp();
}
that's especially weird because execve() doesn't use $PATH ... it's the members of the exec family that have a 'p' in the name that do.
There was a problem hiding this comment.
Ok, I will circle back with another set of diffs to clean up some of these other things too. Thanks.
There was a problem hiding this comment.
The ptrace header issue and the bits from ar.h header I'll submit. SUN_LEN was only added 6 years ago so I figure keep it for a little bit longer.
There was a problem hiding this comment.
The ptrace header issue and the bits from ar.h header I'll submit. SUN_LEN was only added 6 years ago so I figure keep it for a little bit longer.
but it's just a macro in a header, so as long as you're building with an ndk that's less than 6 years old (and no supported ndk is more than 1 year old), "this is fine".
(the ndk has a "unified headers" model where we use clang availability annotations on functions so we can use the exact same headers regardless of what api level you're targeting. so any types/macros/etc "just work" even for OS releases where they weren't present.)
There was a problem hiding this comment.
looking at the bionic headers, getgrgid_r() wasn't available until API 24...
There was a problem hiding this comment.
(getgrgid() has been around forever, though, so if anyone cared they could reduce the scope of this. but probably just #if defined(__ANDROID__) && (__ANDROID_API__ >= 24) is clearer, and groups aren't particularly meaningful on android anyway, so probably no-one cares in the meantime?)
There was a problem hiding this comment.
looking at the bionic headers, getgrgid_r() wasn't available until API 24...
Ok, thanks.
There was a problem hiding this comment.
(getgrgid() has been around forever, though, so if anyone cared they could reduce the scope of this. but probably just
#if defined(__ANDROID__) && (__ANDROID_API__ >= 24)is clearer, and groups aren't particularly meaningful on android anyway, so probably no-one cares in the meantime?)
I guess I was wondering if with another diff it would make sense to check the Android API or a comment making it clear when it would make sense to remove. Either way..
There was a problem hiding this comment.
yeah, "if it confused you, it'll probably confuse the next person". either the comment or the more specific #if sgtm...
from that file: i don't know what lldb's deprecation policy is (or Android Studio's, as "deliverers of the lldb"), but the ndk at least still supports api >= 21, so if you're only asking me, technically i'd not consider that dead code (even if i am very skeptical that there are many developers still debugging on old devices/OS releases --- my anecdata is that most debugging happens on relatively current devices, even if end users are still running apps on much older kit). https://apilevels.com/ claims about 4% of end users are still on an old enough release to have this problem[1].
|
|
All of these workarounds are very old. They date back to the time that API level 21 was brand sparkling new, we were trying to support everything back to level 8, and NDK headers were missing many important functions. This is why you can find references to highly questionable practices like linking to an level 21 libc but running on a leve 8 device. Android support hasn't gotten much attention since those days (though it seems it still works, yay), so nobody went through the trouble of figuring out which one of them is still needed.
It's been a long time, but I'm pretty sure this was due to a test which ran something with an empty environment (some of our tests do that, sometimes deliberately, mostly by accident). It probably wasn't the binary itself that failed to launch, but that it then couldn't find the executable that it was trying to launch. The comment could be incorrect. Maybe it's (ba)sh who inserts Bottom line: I'm pretty sure that the only reason this needs to be there is to make the test suite happy (and maybe not even that, if things have changed since that time). I don't think anyone is running the lldb test suite on android. If you're not planning to start doing that, I think we could delete it.
I was asking this because I thought
As I recall, we were cherry-picking the fix into all supported kernel branches (and maybe even adding conformance tests -- I'm not sure), so that at least new devices (starting from a new kernel branch) should have it. But yes, it's possible that claim is too optimistic... |
it is, but mksh (Android's shell) does that too.
even if anyone is, it might be better to delete this hack and then someone should come and hassle me if/when there's a test failure so we can try to work out what (if anything) is Android-specific here. if nothing else, we'll have a better code comment to add next time round :-) @emrekultursay fyi, because i thought he did run the lldb tests? but maybe i'm confused about which tests exactly...
heh, i've worked on Android since 2009 and i still struggle. https://apilevels.com/ is a godsend for this kind of thing, being a rosetta stone to translate between "whatever you have" and "whatever you need". (though personally i've given up on codenames and marketing names and only use api levels any more, and have even been retroactively changing documentation too. especially because [marketing name] "Android 16" is getting awfully close to something that sounds like an api level!)
ah, yes, it's coming back to me now --- it was you who wrote most of the excellent tests in https://cs.android.com/android/platform/superproject/main/+/main:bionic/tests/sys_ptrace_test.cpp wasn't it? i think that the relevant tests weren't in CTS before api 28 though? certainly the copyright date for that whole file was 2016, so it can't have been much earlier than that, and certainly wasn't api 21. (fyi, these tests found a linux kernel virtualization bug just recently: https://lore.kernel.org/lkml/[email protected]/ --- so thank you again for those!) |
|
On the Android Studio side, we don't run tests on old API levels. We only run automated on latest API levels (and some best-effort manual tests on recent-ish API levels). Different features in Android Studio have different min-API requirements. To me, it seems OK to add "API-21 or newer required for native debugging" to future versions of Android Studio; so LGTM. |
I made a quick test which I think should replicate the thing scenario that this was working around (run
Yes, that was me. :) In particular, the It's quite possible that didn't run as a part of the CTS from the start. If you say it didn't, then I'll take your word for it.
Wow. Thank you for following up on that. I've been aware of issues with running the lldb test suite (the watchpoint tests in particular) in virtualized environments (this is the reason why the lldb linux bot runs on a physical machine and not in the cloud), but I never took the time to isolate and reproduce the bug. It's very cool to see this fixed. |
sorry, i didn't mean to imply that these tests didn't immediately go into CTS when they went into bionic: they will have automatically been included in the next release. it's just that with a copyright date of 2016, they clearly weren't in bionic early enough for api 21. (2016 would be api 24 at the earliest.)
i'll pass that on to the folks who did all the work --- i just handed the CI's auto-filed bug to the systems team and jstultz and others did the real work :-) |
No description provided.