Skip to content

[lldb] Remove support and workarounds for Android 4 and older #124047

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 23, 2025
Merged

[lldb] Remove support and workarounds for Android 4 and older #124047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions lldb/cmake/modules/LLDBConfig.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
9 changes: 0 additions & 9 deletions lldb/include/lldb/Host/Time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 0 additions & 1 deletion lldb/source/Host/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
28 changes: 0 additions & 28 deletions lldb/source/Host/android/LibcGlue.cpp
View file
Open in desktop

This file was deleted.

18 changes: 1 addition & 17 deletions lldb/source/Host/common/Socket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 0 additions & 16 deletions lldb/source/Host/posix/HostInfoPosix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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];
Expand All @@ -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;
}

Expand Down
5 changes: 1 addition & 4 deletions lldb/source/Host/posix/ProcessLauncherPosixFork.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,10 @@
#include <sstream>

#ifdef __ANDROID__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will circle back with another set of diffs to clean up some of these other things too. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the bionic headers, getgrgid_r() wasn't available until API 24...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the bionic headers, getgrgid_r() wasn't available until API 24...

Ok, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, "if it confused you, it'll probably confuse the next person". either the comment or the more specific #if sgtm...

#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

Expand Down
Loading