[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

[David Spickett via Phabricator via lldb-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.

This LGTM.



================
Comment at: lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h:58
+                                               uint32_t reg_num) override;
+  lldb::addr_t ReadPC(bool *success);
+  bool WritePC(lldb::addr_t pc);
----------------
seehearfeel wrote:
> lh03061238 wrote:
> > DavidSpickett wrote:
> > > I think the older targets use this form but for riscv they went with `llvm::Optional<lldb::addr_t> ReadPC();` which I prefer over pointer plus addr_t.
> > > I think the older targets use this form but for riscv they went with `llvm::Optional<lldb::addr_t> ReadPC();` which I prefer over pointer plus addr_t.
> > 
> > EmulateInstructionLoongArch is relatively simple Compared with riscv. If use llvm::Optional<lldb::addr_t> ReadPC(), There will be more type 
> > conversions here. I prefer to keep that definition for now, Considering the complexity of the code at this stage.
> > 
> > Thank you.
> @DavidSpickett Are you OK with the current code?
> I prefer to keep that definition for now, Considering the complexity of the code at this stage.

Totally fine, this is a lot of duplicating existing code to begin with so I appreciate why you'd keep the differences minimal.


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

https://reviews.llvm.org/D139158