[libunwind] Remove __ANDROID_API__ < 18 workaround

[MaskRay]

Summary

https://github.com/android/ndk/wiki/Changelog-r24 shows that the NDK has
moved forward to at least a minimum target API of 19. Remove old workaround.

[rprichard]

Yeah, this change looks good to me.

[smeenai]

Hmm, would we reconsider this? The NDK has moved its minimum version forward, but that doesn't necessarily mean everyone else has as well. Several of Meta's Android apps still support API 16 and above, because we still have a sizeable user base on those platforms. We build the toolchain and standard libraries (including libunwind) from source and try to keep those up to date, so we're going to be adversely affected by this change.

[smeenai]

Ping.

[rprichard]

This particular workaround seems fairly inexpensive and self-contained, so I wouldn't object to keeping it around. Though, would LLVM have to keep it around forever then?

Maybe in other parts of LLVM, it's harder to support older API levels, so it'd be nice there to require newer API levels (and let downstream users maintain support themselves if they need it). For example, I'm currently working on getting libc++ tests working on Android (and switching to using CMake to build Android's libc++), and there are some troublesome Android bugs I've run into that were fixed in API 21. I managed to come up with workarounds for older devices, but they're hairy/annoying, and since the next NDK release is likely to raise the minimum supported SDK to 21 anyway, I was planning to omit the workarounds.

Examples:

• I wrote an adb_run.py script that syncs a directory and runs an executable. There's a bug in pre-21 devices where adbd leaks FDs on the device, which results in ETXTBSY errors. I have a horrid workaround that involves scanning /proc/*/fd/*, but it's unnecessary if we require API 21 to run the tests.
• There are a bunch of floating-point printf bugs that were fixed in API 21 (and one more in 23). At the moment, I have a separate feature test for each bug in libcxx/utils/libcxx/test/features.py, but I'm planning to omit most of them.
• The NDK will drop libandroid_support.a once API 21 is the minimum SDK version. LLVM knows that it has to link libc++.so against libandroid_support.a, but it links libandroid_support.a *after* libc whereas I think the NDK always links it before libc. This turns out to matter because the KitKat libc.so stubs expose swprintf, and the swprintf on that Android version just returns ENOTSUP. libandroid_support.a defines a real swprintf, but it needs to come first to override libc.so. I can upload my patch fixing the link order. (This bug can be fixed easily, but it demonstrates how it could be expensive to keep around something as simple as target_link_libraries(${target} PUBLIC android_support). See D73516.)

[smeenai]

Apologies for the delayed response here ... I was out of the office for a long time.

I completely understand the question around having to keep this workaround forever, and the burden induced by having to support older versions. I myself had a project I really cared about blocked for two years because of issues with old Android versions, and I'm sure all of you on the Android and NDK teams have many similar stories. At the same time, the reason we still support these old systems is because we have millions of users on them, which makes it really hard to leave them behind both from a business and personal perspective.

I'll start a conversation with our app teams about upping their minimum API requirements. I might be able to make a case for 19, but 21 would be a pretty hard sell (though I'd also love it cos it'd simplify a ton of things on our end too). When you say the next NDK release, according to https://github.com/android/ndk/wiki, that won't occur till Q3 2023, right? That gives us a lot of time to evaluate this, at least.

I'll just keep this around as a local patch in the meantime.

libunwind/src/AddressSpace.hpp

 #endif
 #if !defined(Elf_Phdr)
   typedef ElfW(Phdr) Elf_Phdr;
 #endif
 #if !defined(Elf_Addr)
   typedef ElfW(Addr) Elf_Addr;
 #endif
 
-static Elf_Addr calculateImageBase(struct dl_phdr_info *pinfo) {
-  Elf_Addr image_base = pinfo->dlpi_addr;
-#if defined(__ANDROID__) && __ANDROID_API__ < 18
-  if (image_base == 0) {
-    // Normally, an image base of 0 indicates a non-PIE executable. On
-    // versions of Android prior to API 18, the dynamic linker reported a
-    // dlpi_addr of 0 for PIE executables. Compute the true image base
-    // using the PT_PHDR segment.
-    // See https://github.com/android/ndk/issues/505.
-    for (Elf_Half i = 0; i < pinfo->dlpi_phnum; i++) {
-      const Elf_Phdr *phdr = &pinfo->dlpi_phdr[i];
-      if (phdr->p_type == PT_PHDR) {
-        image_base = reinterpret_cast<Elf_Addr>(pinfo->dlpi_phdr) -
-          phdr->p_vaddr;
-        break;
-      }
-    }
-  }
-#endif
-  return image_base;
-}
-
 struct _LIBUNWIND_HIDDEN dl_iterate_cb_data {
   LocalAddressSpace *addressSpace;
   UnwindInfoSections *sects;
   uintptr_t targetAddr;
 };
 
 #if defined(_LIBUNWIND_USE_FRAME_HEADER_CACHE)
 #include "FrameHeaderCache.hpp"

 #if defined(_LIBUNWIND_USE_FRAME_HEADER_CACHE)
   if (TheFrameHeaderCache.find(pinfo, pinfo_size, data))
     return 1;
 #else
   // Avoid warning about unused variable.
   (void)pinfo_size;
 #endif
 
-  Elf_Addr image_base = calculateImageBase(pinfo);
+  Elf_Addr image_base = pinfo->dlpi_addr;
 
   // Most shared objects seen in this callback function likely don't contain the
   // target address, so optimize for that. Scan for a matching PT_LOAD segment
   // first and bail when it isn't found.
   bool found_text = false;
   for (Elf_Half i = 0; i < pinfo->dlpi_phnum; ++i) {
     if (checkAddrInSegment(&pinfo->dlpi_phdr[i], image_base, cbdata)) {
       found_text = true;