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 ®_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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
