[Lldb-commits] [lldb] [lldb] Step over non-lldb breakpoints (PR #174348)

[David Spickett via lldb-commits]

================
@@ -336,3 +338,32 @@ UnwindPlanSP ArchitectureArm::GetArchitectureUnwindPlan(
   }
   return new_plan;
 }
+
+bool ArchitectureArm::IsValidTrapInstruction(
----------------
DavidSpickett wrote:

There's not that much difference so it's likely just personal preference but here's my version:
```
bool ArchitectureArm::IsValidTrapInstruction(
    llvm::ArrayRef<uint8_t> reference, llvm::ArrayRef<uint8_t> observed) const {
  if (reference.size() > observed.size())
    return false;

  if (reference.size() == 2) {
    auto ref_bytes = llvm::support::endian::read16le(reference.data());
    auto obs_bytes = llvm::support::endian::read16le(observed.data());
    if (ref_bytes == obs_bytes)
      return true;

    // LLDB uses an undef instruction encoding for breakpoints
    // perhaps we have an actual BKPT in the inferior
    uint16_t mask = 0xF0;
    if ((obs_bytes & mask) == 0xBE00)
      return true;
  } else if (reference.size() == 4) {
    auto ref_bytes = llvm::support::endian::read32le(reference.data());
    auto obs_bytes = llvm::support::endian::read32le(observed.data());
    if (ref_bytes == obs_bytes)
      return true;

    uint32_t mask = 0x0FF000F0;
    uint32_t bkpt_pattern = 0xE1200070;
    if ((obs_bytes & mask) == bkpt_pattern)
      return true;
  }

  return false;
}
```
I also wonder if you could compare the array refs up front, like:
```
  if (reference != observed)
    return false;
```
This compares size and values (I assume, please check), then later you don't have to have the:
```
    if (ref_bytes == obs_bytes)
      return true;
```
In each branch of the if. Endian shouldn't be a problem here as it's a byte by byte comparison. Same result in either order.

Then the code becomes:
* Try the most straightforward comparison
* If it didn't work, then do the fiddly size specific mask part

https://github.com/llvm/llvm-project/pull/174348