[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/include/lldb/Core/EmulateInstruction.h:196
+      struct RegisterPlusOffsetStruct {
         RegisterInfo reg;      // base register
         int64_t signed_offset; // signed offset added to base register
----------------
labath wrote:
> I actually think that the simplest solution here would be to store the RegisterInfos as pointers. Copying them around doesn't make sense, particularly if their size is about to grow.
True, but sounds like we're going toward adding a pointer to the info. So that should keep the size constant.


================
Comment at: lldb/include/lldb/Core/EmulateInstruction.h:342-345
+      info.~ContextUnion();
+      info.info_type = eInfoTypeRegisterRegisterOperands;
+      new (&info.RegisterRegisterOperands.operand1) RegisterInfo(op1_reg);
+      new (&info.RegisterRegisterOperands.operand2) RegisterInfo(op2_reg);
----------------
labath wrote:
> I am not a C++ expert, but I have a strong suspicion that this is not correct. You're destroyed the whole object, but then are only recreating its individual submembers. I get that these are the only members which have non-trivial constructors, but I don't think that's how c++ works. I can confirm this with some c++ experts if you want, but I am pretty sure that you need to recreate the ContextUnion object as a whole here to avoid straying into UB territory.
I will check the rules here, it's not too hard to change anyway and it would mean I don't forget one of the members.


================
Comment at: lldb/include/lldb/lldb-private-types.h:67-77
+  RegisterInfo() { kinds.fill(LLDB_INVALID_REGNUM); }
+
+  RegisterInfo(const char *name, const char *alt_name, uint32_t byte_size,
+               uint32_t byte_offset, lldb::Encoding encoding,
+               lldb::Format format,
+               std::array<uint32_t, lldb::kNumRegisterKinds> kinds,
+               uint32_t *value_regs, uint32_t *invalidate_regs)
----------------
labath wrote:
> Is this class still trivially constructible (not requiring dynamic initialization)?
Current state, no. Due to the constructor setting `kinds`. Which is my attempt to remove the `memset` that was used elsewhere to do this, which sort of implies it was never trivially constructable really. Unless you knew you weren't going to read certain fields.

Given that:
```
#define LLDB_INVALID_REGNUM UINT32_MAX
```

And the memsets all used 0. At the very least it's opaque what the state of RegisterInfo is supposed to be.

That said I looked again at kNumRegisterKinds and it only goes up to 5. So I could just write this as:
```
uint32_t kinds[lldb::kNumRegisterKinds] = {LLDB_INVALID_REGNUM ... };
```

To avoid all this (I think I mixed it up with another enum with 100s of members).


================
Comment at: lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp:785-787
 bool EmulateInstructionARM::GetRegisterInfo(lldb::RegisterKind reg_kind,
                                             uint32_t reg_num,
                                             RegisterInfo &reg_info) {
----------------
clayborg wrote:
> Do we want to switch this over to returning a llvm::Optional<RegisterInfo> as well in the base class?
I will try this as a separate change, unconnected to this. In general any time we can change bool+ref to optional, I'll take it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134041