[lldb/Core] Don't crash in GetSoftwareBreakpointTrapOpcode for unknown triples

[JDevlieghere]

Summary

A reproducer replay hit an llvm_unreachable in Target.cpp because the architecture was not set and therefore the ArchSpec was invalid.

Unhandled architecture in Platform::GetSoftwareBreakpointTrapOpcode
UNREACHABLE executed at /Users/jonas/llvm/llvm-project/lldb/source/Target/Platform.cpp:1922!

The unhandled case is llvm::Triple::UnknownArch. Alternatively we can add a case for it to the switch statement, but checking IsValid seemed more canonical.

[davide]

What's exactly the testcase doing?

[clayborg]

None of this is exposed through SBPlatform right? No way to test this?

lldb/source/Target/Platform.cpp

1825–1826

would be nice to log something here on the "lldb break" log channel?

[labath]

The presence of llvm_unreachable here is questionable, but I am surprised that this comes up in the context of reproducers. If the reproducers cause this function to be called with a different ArchSpec, then it sounds like there are bigger problems that need to be solved..

If we do want to do something about the crash, then I think we ought to just remove the llvm_unreachable. I don't think it makes sense to bail out on invalid ArchSpecs, but blow up on not-yet-supported architectures. If anything, I would say it should be the opposite -- I don't think it makes sense for anyone to call this function with an invalid/empty ArchSpec, but it may be reasonable to enable some degraded behavior for architectures which are not fully supported.

[JDevlieghere]

In D78588#1996222, @labath wrote:

The presence of llvm_unreachable here is questionable, but I am surprised that this comes up in the context of reproducers. If the reproducers cause this function to be called with a different ArchSpec, then it sounds like there are bigger problems that need to be solved..

I only mentioned the reproducers to show that we can reach this code with an invalid ArchSpec and that this is not just a speculative fix. Since you're interested in what triggered this: TestRecognizeBreakpoint.py is a GDBRemoteTestBase, which starts with an empty Target (hence the invalid ArchSpec later on) and then connects to the test's GDB remote. It does so through ConnectRemote which for ProcessGDBRemote calls ConnectToDebugserver, which bypasses redirecting the connection to the GDB replay server. Fixing the reproducers issue is just a matter of moving that code down, so we connect to the replay server instead of the test's gdb server (which isn't there during active replay).

If we do want to do something about the crash, then I think we ought to just remove the llvm_unreachable. I don't think it makes sense to bail out on invalid ArchSpecs, but blow up on not-yet-supported architectures. If anything, I would say it should be the opposite -- I don't think it makes sense for anyone to call this function with an invalid/empty ArchSpec, but it may be reasonable to enable some degraded behavior for architectures which are not fully supported.

Sounds reasonable to me. We can add an assert arch.IsValid() to enforce that precondition and have the default case return 0.

[labath]

This makes sense to me.

(If we are able to reach this deep into the code with an invalid archspec by just issuing (sb or cli) commands, it sounds like there should be some check (and a bail out) very early on in the call stack.)

lldb/source/Target/Platform.cpp

size_t Platform::ConnectToWaitingProcesses(lldb_private::Debugger &debugger,
                                            lldb_private::Status &error) {
   error.Clear();
   return 0;
 }
 
 size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target,
                                                  BreakpointSite *bp_site) {
   ArchSpec arch = target.GetArchitecture();
+  assert(arch.IsValid());
   const uint8_t *trap_opcode = nullptr;
   size_t trap_opcode_size = 0;
 
   switch (arch.GetMachine()) {
   case llvm::Triple::aarch64_32:
   case llvm::Triple::aarch64: {
     static const uint8_t g_aarch64_opcode[] = {0x00, 0x00, 0x20, 0xd4};
     trap_opcode = g_aarch64_opcode;
     trap_opcode_size = sizeof(g_aarch64_opcode);
size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target,
   case llvm::Triple::x86:
   case llvm::Triple::x86_64: {
     static const uint8_t g_i386_opcode[] = {0xCC};
     trap_opcode = g_i386_opcode;
     trap_opcode_size = sizeof(g_i386_opcode);
   } break;
 
   default:
-    llvm_unreachable(
-        "Unhandled architecture in Platform::GetSoftwareBreakpointTrapOpcode");
+    return 0;
   }
 
   assert(bp_site);
   if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
     return trap_opcode_size;
 
   return 0;
 }