[Lldb-commits] [PATCH] D139833: [LLDB][LoongArch] Add branch instructions for EmulateInstructionLoongArch

[David Spickett via Phabricator via lldb-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

This is all very repetitive, but that's to be expected. Whatever you can do to reduce boilerplate in future patches would be great. For example, can 32 and 64 bit variants of some instructions be emulated in the same way? Perhaps with a template parameter for the return types.

This LGTM with a few comments you can do/not do/keep in mind for future patches.



================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:276
+    return false;
+}
+
----------------
All these could be of the form:
```
bool EmulateInstructionLoongArch::EmulateBGEU(uint32_t inst) {
  return IsLoongArch64()) ? EmulateBGEU64(inst) : false;
}
```

Generally I'd advise coming up with a different way of doing this, that checks up front that you won't call `EmulateBGEU` and friends when you are emulating 32 bit. Perhaps some table lookup first.

That said perhaps you intend to replace the `else` here with a call to `EmulateBGEU32` in future. Up to you how to structure that.

In any case I'd slim down these functions as suggested for now.


================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp:368
+  bool success = false;
+  uint64_t pc = ReadPC(&success);
+  if (!success)
----------------
This is what I mean about using optionals.

```
  std::optional<uint64_t> pc = ReadPC(&success);
  if (!pc)
    return false;
  EmulateInstruction::Context ctx;
  if (!WriteRegisterUnsigned(ctx, eRegisterKindLLDB, gpr_r1_loongarch, *pc + 4))
    return false;
```

It's a small saving and ok you have to `*` some places but when this stuff gets more complicated it can help.

Anyway, up to you but keep it in mind as things get more complicated.


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

https://reviews.llvm.org/D139833