Suppress SIGSEGV on Android when the program will recover

[clayborg]

Summary

SIGSEGV signals are sent to Android processes when a NULL dereference happens in Java code that is being run as native code in dex, odex and oat files. In this patch I modified the Platforms to be allowed to modify a stop reason for a thread. The PlatformAndroid will watch for SIGSEGV stop reasons and see if the frame that caused the SIGSEGV comes from a dex, odex or oat file, and if so, it will suppress the stop reason. Many IDEs are manually figuring this out and learning to skip the signal so users don't see it. Even when IDEs do this, the IDE might end up showing a SIGSEGV as a valid stop reason for a thread. Also, a SIGSEGV thread might be selected when another thread hits a breakpoint. So we suppress these signals to avoid spurious thread changes and improve android debugging.

[jingham]

If you can get the address of the bad access from the signal, you could also check that it was 0x0 and only suppress the SIGSEGV if it is?

Also, do you want to put in a setting to turn this behavior off? If the code in any of the files of this type were to crash for some other reason than a Java NULL dereference, you'd have no way to use lldb to debug the issue. lldb will just auto-continue and then either the program will terminate because the SIGSEGV handler doesn't handle this signal or the SIGSEGV handler will crash or whatever... That will be too late to be useful.

But I don't know anything about how these .oat and .dex/.odex files are constructed, and maybe they can't crash for any other reason than emulating a Java NULL access, in which case this looks fine.

source/Plugins/Platform/Android/PlatformAndroid.cpp

424

Given that this is a pretty big behavior change, I would exactly match on the three extensions rather than use endswith, so it only affects the file types you care about.

[labath]

Thank you for implementing this. We've had code like this in android studio for a long time, but it's definitely better doing it in lldb instead.

I'll give some more background so that Jim and anyone else looking at this can understand what's going on. These files (I forgot what's the difference between them) contain the compiled version of Java code. As java is a "safe" language, the code should not generate any signals (modulo compiler bugs) that the runtime can't handle. The SEGV is just the implementation's way of avoiding null checks everywhere -- on the assumption that NullPointerExceptions are rare, its faster to not generate them and clean up the occasional mess in the signal handler.
For that reason, an android app (*) will always have a SEGV handler. This handler will be invoked even for non-bening SEGVs (so simply resuming from a SEGV will never crash the app immediately). For signals the runtime can't handle it will invoke a special art_sigsegv_fault function, which the debugger can hook to catch "real" segfaults. Unfortunately, in the past this mechanism was unreliable (the function could end up inlined), so we have to do this dance instead. Once android versions with the fixed "fault" function become more prevalent, we can skip this and just automatically reinject all SIGSEGVs. This is particularly important as each new version of android finds new creative ways to "optimize" things via SIGSEGVs, and going things this way means constantly playing catch-up.

So much for background. I think Jim's suggestion on having all of this this controllable by a setting makes sense, and it would be consistent with how we handle other kinds of "magical" under-the-hood stops (target.process.stop-on-sharedlibrary-events). I'm not sure how much use would it get, but I can imagine it being useful for debugging the segv handling code itself. I'm a bit sad that we now have two plugins with the OverrideStopInfo functionality, but I can't think of any better way of arranging things right now.

(*) This means "real" GUI apps. command line executables will not have the android runtime inside them, nor the special segv handler, but that means they will not contain any "dex" files either.

source/Plugins/Platform/Android/PlatformAndroid.cpp

399

I'm not sure if SEGV is one of them, but numbers of some signals vary between architectures. You should be able to get the real value via process->GetUnixSignals()

424

Agreed, matching on the exact extension looks safer and more obvious. The most llvm-y way of writing that would be StringSwitch<bool>(ext.GetStringRef()).Cases("dex", "oat", "odex", true).Default(false)

[jingham]

Thanks for the explanation!

Jim

[jingham]

Another thing that slightly bugs me about this patch is now we have the Architecture with special purpose code to modify the stop reason, and the Platform ditto. I wonder if it wouldn't be better to have a way to register interest in modifying stop infos, and then let the target & architecture sign up for that. That way the next time somebody else needs to do this we won't have to add more special purpose code to Thread.cpp.

include/lldb/Target/Platform.h

public:
   ///     contain the error message.
   ///
   /// @return
   ///     The number of processes we are successfully connected to.
   //------------------------------------------------------------------
   virtual size_t ConnectToWaitingProcesses(lldb_private::Debugger &debugger,
                                            lldb_private::Status &error);
 
+  //------------------------------------------------------------------
+  /// Allow platforms to modify thread stop info.
+  ///
+  /// Platforms might have specific signals or stop reasons that are
+  /// overloaded and might not need to be reported. Platform
+  /// subclasses can override this function and modify the stop reason
+  /// when needed.
+  //------------------------------------------------------------------
+  virtual void OverrideStopInfo(Thread &thread) {}
+
 protected:
   bool m_is_host;
   // Set to true when we are able to actually set the OS version while being
   // connected. For remote platforms, we might set the version ahead of time
   // before we actually connect and this version might change when we actually
   // connect to a remote platform. For the host platform this will be set to
   // the once we call HostInfo::GetOSVersion().
   bool m_os_version_set_while_connected;

source/Plugins/Platform/Android/PlatformAndroid.h

public:
   uint32_t GetSdkVersion();
 
   bool GetRemoteOSVersion() override;
 
   Status DisconnectRemote() override;
 
   uint32_t GetDefaultMemoryCacheLineSize() override;
 
+  void OverrideStopInfo(Thread &thread) override;
+
 protected:
   const char *GetCacheHostname() override;
 
   Status DownloadModuleSlice(const FileSpec &src_file_spec,
                              const uint64_t src_offset, const uint64_t src_size,
                              const FileSpec &dst_file_spec) override;
 
   Status DownloadSymbolFile(const lldb::ModuleSP &module_sp,

source/Plugins/Platform/Android/PlatformAndroid.cpp

 //===-- PlatformAndroid.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Scalar.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/StringConvert.h"
+#include "lldb/Target/StackFrame.h"
+#include "lldb/Target/StopInfo.h"
+#include "lldb/Target/Thread.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/UriParser.h"
 
 // Project includes
 #include "AdbClient.h"
 #include "PlatformAndroid.h"
 #include "PlatformAndroidRemoteGDBServer.h"
 #include "lldb/Target/Target.h"
return R"(
               extern "C" void* dlsym(void*, const char*) asm("__dl_dlsym");
               extern "C" int   dlclose(void*) asm("__dl_dlclose");
               extern "C" char* dlerror(void) asm("__dl_dlerror");
              )";
 
   return PlatformPOSIX::GetLibdlFunctionDeclarations(process);
 }
 
+// Define a SIGSEGV that doesn't require any headers
+#define ANDROID_SIGSEGV 11
+
+void PlatformAndroid::OverrideStopInfo(Thread &thread) {
+  auto stop_info_sp = thread.GetStopInfo();
+  if (!stop_info_sp)
+    return;
+  // Check for SIGSEGV that is called from a .dex, .odex or .oat file.
+  // These are going to be dealt with by the runtime so we can just erase
+  // the stop reason.
+  const auto reason = stop_info_sp->GetStopReason();
+  if (reason != eStopReasonSignal)
+    return;
+  if (stop_info_sp->GetValue() != ANDROID_SIGSEGV)
+    return;
+  auto frame_sp = thread.GetStackFrameAtIndex(0);
+  if (!frame_sp)
+    return;
+  auto module_sp = frame_sp->GetSymbolContext(eSymbolContextModule).module_sp;
+  if (!module_sp)
+    return;
+  auto ext = module_sp->GetFileSpec().GetFileNameExtension();
+  if (!ext)
+    return;
+  llvm::StringRef ext_ref(ext.GetCString(), ext.GetLength());
+  // We are lookking for .dex, .odex, and .oat files.
+  if (ext_ref.endswith("dex") || ext_ref.endswith("oat")) {
+    // We have a SIGSEGV we need to mute
+    thread.SetStopInfo(lldb::StopInfoSP());
+  }
+}
+
 AdbClient::SyncService *PlatformAndroid::GetSyncService(Status &error) {
   if (m_adb_sync_svc && m_adb_sync_svc->IsConnected())
     return m_adb_sync_svc.get();
 
   AdbClient adb(m_device_id);
   m_adb_sync_svc = adb.GetSyncService(error);
   return (error.Success()) ? m_adb_sync_svc.get() : nullptr;
 }
+

source/Target/Thread.cpp

if (process_sp) {
     // "m_stop_info_stop_id != process_stop_id" as the condition for the if
     // statement below, we must also check the stop info to see if we need to
     // override it. See the header documentation in
     // Process::GetStopInfoOverrideCallback() for more information on the stop
     // info override callback.
     if (m_stop_info_override_stop_id != process_stop_id) {
       m_stop_info_override_stop_id = process_stop_id;
       if (m_stop_info_sp) {
+        // If there is an architecture plug-in for this target architecture,
+        // let it possibly modify the stop reason.
         if (Architecture *arch =
                 process_sp->GetTarget().GetArchitecturePlugin())
           arch->OverrideStopInfo(*this);
+        // Let the platform get a chance to modify the stop reason.
+        auto platform_sp = GetProcess()->GetTarget().GetPlatform();
+        if (platform_sp) {
+          platform_sp->OverrideStopInfo(*this);
+        }
       }
     }
   }
   return m_stop_info_sp;
 }
 
 lldb::StopReason Thread::GetStopReason() {
   lldb::StopInfoSP stop_info_sp(GetStopInfo());