[Lldb-commits] [PATCH] D78588: [lldb/Core] Check that ArchSpec is valid.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D78588#1996222 <https://reviews.llvm.org/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`.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78588/new/

https://reviews.llvm.org/D78588