[lldb][Android] Use a lambda for calls to ::open in RetryAfterSignal

[splhack]

Summary

lldb-server for Android does not build with NDK r21 and above due to
RetryAfterSignal and Bionic ::open mismatch.
https://github.com/llvm/llvm-project/issues/54727

Apply the LLVM patch to LLDB.
https://github.com/llvm/llvm-project/commit/0a0e411204a2baa520fd73a8d69b664f98b428ba

In Bionic, open can be overloaded for _FORTIFY_SOURCE support, causing
compile errors of RetryAfterSignal due to overload resolution. Wrapping
the call in a lambda avoids this.

[hans]

+labath

[bulbazord]

I don't really work on android-related code anymore but this change seems alright to me. There's prior art in LLVM which makes it a lot easier to accept. It's unfortunate that we need this workaround because of an issue with Bionic, but that's life.

You may want to wait until another person has a chance to look at this though.

[yinghuitan]

Can we wrap this as a helper function and shared to avoid duplication? Otherwise looks good.

[splhack]

Add lldb/include/lldb/Host/common/RetryAfterSignal.h

[splhack]

@yinghuitan yup, RetryAfterSignal::Open helper functions look better.

[splhack]

rebase onto main

[JDevlieghere]

Should we make this the default behavior in FileSystem? Either way I think this belongs there rather than in Host.

[splhack]

@JDevlieghere does FileSystem::RetryAfterSignal::Open sound good to you?
Or FileSystem::RetryAfterSignalOpen?

[splhack]

oh wait, FileSystem::Open is already RetryAfterSignal::Open in this diff.
will replace RetryAfterSignal::Open with FileSystem::Open.

[splhack]

Replace llvm::sys::RetryAfterSignal(-1, ::open) with
FileSystem::Instance().Open.

[JDevlieghere]

Nice. LGTM.

[leonardchan]

Hi. I think this is leading to the lldb test failures we're seeing on our linux builder (https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8778187770933863505/+/u/lldb/test/stdout?format=raw):

********************
FAIL: lldb-unit :: Host/./HostTests/13/39 (1722 of 2262)
******************** TEST 'lldb-unit :: Host/./HostTests/13/39' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/b/s/w/ir/x/w/staging/llvm_build/tools/lldb/unittests/Host/./HostTests-lldb-unit-1483998-13-39.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=39 GTEST_SHARD_INDEX=13 /b/s/w/ir/x/w/staging/llvm_build/tools/lldb/unittests/Host/./HostTests
--

Note: This is test shard 14 of 39.
[==========] Running 2 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 1 test from File
[ RUN      ] File.GetWaitableHandleFileno
[       OK ] File.GetWaitableHandleFileno (1 ms)
[----------] 1 test from File (1 ms total)

[----------] 1 test from TerminalTest
[ RUN      ] TerminalTest.SetRaw
/b/s/w/ir/x/w/cipd/bin/../include/c++/v1/optional:1023: assertion this->has_value() failed: optional operator* called on a disengaged value#0 0x0000560219fba7a8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/b/s/w/ir/x/w/staging/llvm_build/tools/lldb/unittests/Host/./HostTests+0x1417a8)

--
exit: -6
--
shard JSON output does not exist: /b/s/w/ir/x/w/staging/llvm_build/tools/lldb/unittests/Host/./HostTests-lldb-unit-1483998-13-39.json
********************

Would you be able to send a fix or revert? Thanks.

[splhack]

@leonardchan yes, I'm looking into. -6 is ENXIO? Is it using POSIX code path? Is it possible to see symbolicated stack trace of the crash?

[splhack]

found symbolicated stack trace in
https://lab.llvm.org/buildbot/#/builders/96/builds/40769/steps/6/logs/stdio

TerminalTest does not instantiate FileSystem. now preparing to submit a diff to fix it.

 #0 0x0000aaaacc46d6d4 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xa06d4)
 #1 0x0000aaaacc46bc08 llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0x9ec08)
 #2 0x0000aaaacc46df78 SignalHandler(int) Signals.cpp:0:0
 #3 0x0000ffffb0a38598 (linux-vdso.so.1+0x598)
 #4 0x0000ffffb023f200 __pthread_kill_implementation ./nptl/./nptl/pthread_kill.c:44:76
 #5 0x0000ffffb01fa67c gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x0000ffffb01e7130 abort ./stdlib/./stdlib/abort.c:81:7
 #7 0x0000aaaacc3f62e4 testing::AssertionResult testing::internal::CmpHelperEQ<lldb_private::URI, lldb_private::URI>(char const*, char const*, lldb_private::URI const&, lldb_private::URI const&) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0x292e4)
 #8 0x0000aaaacc4bb1d0 lldb_private::FileSystem::Instance() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xee1d0)
 #9 0x0000aaaacc4c3f18 lldb_private::PseudoTerminal::OpenSecondary(int) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xf6f18)
#10 0x0000aaaacc45304c TerminalTest::SetUp() crtstuff.c:0:0
#11 0x0000aaaacc48bccc testing::Test::Run() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xbeccc)
#12 0x0000aaaacc48d580 testing::TestInfo::Run() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xc0580)
#13 0x0000aaaacc48dca8 testing::TestSuite::Run() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xc0ca8)
#14 0x0000aaaacc49b55c testing::internal::UnitTestImpl::RunAllTests() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xce55c)
#15 0x0000aaaacc49adb8 testing::UnitTest::Run() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xcddb8)
#16 0x0000aaaacc483540 main (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xb6540)
#17 0x0000ffffb01e73fc __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#18 0x0000ffffb01e74cc call_init ./csu/../csu/libc-start.c:128:20
#19 0x0000ffffb01e74cc __libc_start_main ./csu/../csu/libc-start.c:379:5
#20 0x0000aaaacc3f4670 _start (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0x27670)
--
exit: -6
--

[splhack]

@leonardchan I think D153091 will fix the abort (-6 should be SIGABRT), therefore it should be the assertion check of re-initialization in FileSystem::Instance.
Is there a way to run the Linux builder with D153091 to make sure it will fix it?

[splhack]

Verified D153091 fixed TerminalTest failures.

from Failed Tests (13)
https://lab.llvm.org/buildbot/#/builders/96/builds/40769

to Failed Tests (1)
https://lab.llvm.org/buildbot/#/builders/96/builds/40781

will submit a diff to fix the last one, MainLoopTest.DetectsEOF

[ RUN      ] MainLoopTest.DetectsEOF
 #0 0x0000aaaae2a5c738 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xa0738)
 #1 0x0000aaaae2a5ac6c llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0x9ec6c)
 #2 0x0000aaaae2a5cfdc SignalHandler(int) Signals.cpp:0:0
 #3 0x0000ffff957c0598 (linux-vdso.so.1+0x598)
 #4 0x0000ffff94fcf200 __pthread_kill_implementation ./nptl/./nptl/pthread_kill.c:44:76
 #5 0x0000ffff94f8a67c gsignal ./signal/../sysdeps/posix/raise.c:27:6
 #6 0x0000ffff94f77130 abort ./stdlib/./stdlib/abort.c:81:7
 #7 0x0000aaaae29e52e4 testing::AssertionResult testing::internal::CmpHelperEQ<lldb_private::URI, lldb_private::URI>(char const*, char const*, lldb_private::URI const&, lldb_private::URI const&) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0x292e4)
 #8 0x0000aaaae2aaa234 lldb_private::FileSystem::Instance() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xee234)
 #9 0x0000aaaae2ab2f7c lldb_private::PseudoTerminal::OpenSecondary(int) (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xf6f7c)
#10 0x0000aaaae29f5458 MainLoopTest_DetectsEOF_Test::TestBody() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0x39458)
#11 0x0000aaaae2a7ae80 testing::Test::Run() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xbee80)
#12 0x0000aaaae2a7c5e4 testing::TestInfo::Run() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xc05e4)
#13 0x0000aaaae2a7cd0c testing::TestSuite::Run() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xc0d0c)
#14 0x0000aaaae2a8a5c0 testing::internal::UnitTestImpl::RunAllTests() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xce5c0)
#15 0x0000aaaae2a89e1c testing::UnitTest::Run() (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xcde1c)
#16 0x0000aaaae2a725a4 main (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0xb65a4)
#17 0x0000ffff94f773fc __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#18 0x0000ffff94f774cc call_init ./csu/../csu/libc-start.c:128:20
#19 0x0000ffff94f774cc __libc_start_main ./csu/../csu/libc-start.c:379:5
#20 0x0000aaaae29e3670 _start (/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/tools/lldb/unittests/Host/./HostTests+0x27670)

--
exit: -6

[splhack]

D153102 should fix the last test failure.

lldb/include/lldb/Host/FileSystem.h

public:
   Status Readlink(const FileSpec &src, FileSpec &dst);
 
   Status ResolveSymbolicLink(const FileSpec &src, FileSpec &dst);
 
   /// Wraps ::fopen in a platform-independent way.
   FILE *Fopen(const char *path, const char *mode);
 
   /// Wraps ::open in a platform-independent way.
-  int Open(const char *path, int flags, int mode);
+  int Open(const char *path, int flags, int mode = 0600);
 
   llvm::Expected<std::unique_ptr<File>>
   Open(const FileSpec &file_spec, File::OpenOptions options,
        uint32_t permissions = lldb::eFilePermissionsFileDefault,
        bool should_close_fd = true);
 
   /// Get a directory iterator.
   /// \{

lldb/source/Host/common/PseudoTerminal.cpp

 //===-- PseudoTerminal.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
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Host/Config.h"
+#include "lldb/Host/FileSystem.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Errno.h"
 #include <cassert>
 #include <climits>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
 #include <mutex>
#else
   return llvm::errorCodeToError(llvm::errc::not_supported);
 #endif
 }
 
 llvm::Error PseudoTerminal::OpenSecondary(int oflag) {
   CloseSecondaryFileDescriptor();
 
   std::string name = GetSecondaryName();
-  m_secondary_fd = llvm::sys::RetryAfterSignal(-1, ::open, name.c_str(), oflag);
+  m_secondary_fd = FileSystem::Instance().Open(name.c_str(), oflag);
   if (m_secondary_fd >= 0)
     return llvm::Error::success();
 
   return llvm::errorCodeToError(
       std::error_code(errno, std::generic_category()));
 }
 
 #if !HAVE_PTSNAME_R || defined(__APPLE__)

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

 // Enable this special support for Apple builds where we can have unlimited
 // select bounds. We tried switching to poll() and kqueue and we were panicing
 // the kernel, so we have to stick with select for now.
 #define _DARWIN_UNLIMITED_SELECT
 #endif
 
 #include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
 #include "lldb/Host/Config.h"
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Socket.h"
 #include "lldb/Host/SocketAddress.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/SelectHelper.h"
 #include "lldb/Utility/Timeout.h"
 
 #include <cerrno>
 #include <cstdlib>

 }
 
 ConnectionStatus ConnectionFileDescriptor::ConnectFile(
     llvm::StringRef s, socket_id_callback_type socket_id_callback,
     Status *error_ptr) {
 #if LLDB_ENABLE_POSIX
   std::string addr_str = s.str();
   // file:///PATH
-  int fd = llvm::sys::RetryAfterSignal(-1, ::open, addr_str.c_str(), O_RDWR);
+  int fd = FileSystem::Instance().Open(addr_str.c_str(), O_RDWR);
   if (fd == -1) {
     if (error_ptr)
       error_ptr->SetErrorToErrno();
     return eConnectionStatusError;
   }
 
   if (::isatty(fd)) {
     // Set up serial terminal emulation
#if LLDB_ENABLE_POSIX
   if (!serial_options) {
     if (error_ptr)
       *error_ptr = serial_options.takeError();
     else
       llvm::consumeError(serial_options.takeError());
     return eConnectionStatusError;
   }
 
-  int fd = llvm::sys::RetryAfterSignal(-1, ::open, path.str().c_str(), O_RDWR);
+  int fd = FileSystem::Instance().Open(path.str().c_str(), O_RDWR);
   if (fd == -1) {
     if (error_ptr)
       error_ptr->SetErrorToErrno();
     return eConnectionStatusError;
   }
 
   llvm::Expected<std::unique_ptr<SerialPort>> serial_sp = SerialPort::Create(
       fd, File::eOpenOptionReadWrite, serial_options.get(), true);

lldb/source/Host/posix/FileSystemPosix.cpp

Status FileSystem::ResolveSymbolicLink(const FileSpec &src, FileSpec &dst) {
   return Status();
 }
 
 FILE *FileSystem::Fopen(const char *path, const char *mode) {
   return llvm::sys::RetryAfterSignal(nullptr, ::fopen, path, mode);
 }
 
 int FileSystem::Open(const char *path, int flags, int mode) {
-  return llvm::sys::RetryAfterSignal(-1, ::open, path, flags, mode);
+  // Call ::open in a lambda to avoid overload resolution in RetryAfterSignal
+  // when open is overloaded, such as in Bionic.
+  auto lambda = [&]() { return ::open(path, flags, mode); };
+  return llvm::sys::RetryAfterSignal(-1, lambda);
 }

lldb/source/Host/posix/PipePosix.cpp

 //===-- PipePosix.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
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/posix/PipePosix.h"
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/SelectHelper.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Errno.h"
-#include "llvm/Support/FileSystem.h"
 #include <functional>
 #include <thread>
 
 #include <cerrno>
 #include <climits>
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <sys/types.h>
Status PipePosix::OpenAsReader(llvm::StringRef name,
   if (CanRead() || CanWrite())
     return Status("Pipe is already opened");
 
   int flags = O_RDONLY | O_NONBLOCK;
   if (!child_process_inherit)
     flags |= O_CLOEXEC;
 
   Status error;
-  int fd = llvm::sys::RetryAfterSignal(-1, ::open, name.str().c_str(), flags);
+  int fd = FileSystem::Instance().Open(name.str().c_str(), flags);
   if (fd != -1)
     m_fds[READ] = fd;
   else
     error.SetErrorToErrno();
 
   return error;
 }
 

lldb/source/Host/posix/ProcessLauncherPosixFork.cpp

 //===-- ProcessLauncherPosixFork.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
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Host/posix/ProcessLauncherPosixFork.h"
+#include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostProcess.h"
 #include "lldb/Host/Pipe.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Errno.h"
-#include "llvm/Support/FileSystem.h"
 
 #include <climits>
 #include <sys/ptrace.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 #include <sstream>
 #include <csignal>
#if defined(__linux__)
 
   value = personality(ADDR_NO_RANDOMIZE | value);
   if (value == -1)
     ExitWithError(error_fd, "personality set");
 #endif
 }
 
 static void DupDescriptor(int error_fd, const char *file, int fd, int flags) {
-  int target_fd = llvm::sys::RetryAfterSignal(-1, ::open, file, flags, 0666);
+  int target_fd = FileSystem::Instance().Open(file, flags, 0666);
 
   if (target_fd == -1)
     ExitWithError(error_fd, "DupDescriptor-open");
 
   if (target_fd == fd)
     return;
 
   if (::dup2(target_fd, fd) == -1)