labath added a comment.
I like this.
================
Comment at: lldb/source/Plugins/Process/FreeBSD/FreeBSDThread.cpp:167
assert(target_arch.GetTriple().getOS() == llvm::Triple::FreeBSD);
switch (target_arch.GetMachine()) {
case llvm::Triple::aarch64:
----------------
It would be good to merge these two switches. Then the reg(set)_interface
variables would be local to each case label and it would not be so weird that
we sometimes use one and sometimes the other.
================
Comment at:
lldb/source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_arm64.h:21
lldb_private::Thread &thread, uint32_t concrete_frame_idx,
- lldb_private::RegisterInfoInterface *register_info);
+ lldb_private::RegisterInfoAndSetInterface *register_info);
----------------
While we're changing interfaces, let's also make this
unique_ptr<RegisterInfoAndSetInterface> to document the ownership transfer. (I
might also drop the concrete_frame_idx argument, as that is always zero.)
================
Comment at:
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:71-72
+
+ m_regset_interface_up =
std::static_pointer_cast<RegisterInfoAndSetInterface>(
+ m_register_info_interface_up);
}
----------------
The way I'd recommend dealing with this is to have a `RegisterInfoPOSIX_arm64&
GetRegisterInfo() const` method which performs a cast on the
`m_register_info_interface_up` pointer, and then have everything call that. If
you place that method in close proximity to this constructor, then it should be
clear that this operation is always safe. There's already something similar
being done in e.g. `NativeThreadLinux::GetProcess`.
================
Comment at:
lldb/source/Plugins/Process/Utility/NativeRegisterContextRegisterInfo.h:36
-private:
- std::unique_ptr<RegisterInfoInterface> m_register_info_interface_up;
+ std::shared_ptr<RegisterInfoInterface> m_register_info_interface_up;
};
----------------
With the above idea, it should no longer be needed to change this into a
shared_ptr.
================
Comment at:
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h:23-24
RegisterContextPOSIX_arm64(
lldb_private::Thread &thread, uint32_t concrete_frame_idx,
- lldb_private::RegisterInfoInterface *register_info);
+ lldb_private::RegisterInfoAndSetInterface *register_info);
----------------
similar thoughts about unique_ptr and concrete_frame_idx. In fact, thinking
ahead to the next patch, I think we could make this an
`unique_ptr<RegisterInfoPOSIX_arm64>` even.
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoAndSetInterface.h:24
+ : RegisterInfoInterface(target_arch) {}
+ virtual ~RegisterInfoAndSetInterface() {}
+
----------------
s/virtual/override (or just omit the destructor completely)
================
Comment at:
lldb/source/Plugins/Process/Utility/RegisterInfoAndSetInterface.h:35-37
+ virtual uint32_t GetRegisterOffset(uint32_t reg_index) const = 0;
+
+ virtual uint32_t GetRegisterSize(uint32_t reg_index) const = 0;
----------------
These aren't really register *set* related, are they? Could they go on the base
interface instead?
Although, THB, the way I'd handle this is by having something a
`ArrayRef<RegisterInfo> RegInfos()` interface, and then having the callers do
`reg_info_interface->RegInfos()[reg_index].offset`
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:16
-class RegisterInfoPOSIX_arm64 : public lldb_private::RegisterInfoInterface {
+enum { RegisterSetPOSIX_ARM64GPR = 0, RegisterSetPOSIX_ARM64FPR };
+
----------------
How about we put this inside the `RegisterInfoPOSIX_arm64` class, and drop
`RegisterSetPOSIX_ARM64` from the name. (`RegisterInfoPOSIX_arm64::GPRegSet` ?)
(there's already something similar inside `NativeRegisterContextNetBSD_x86_64`).
================
Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h:21-29
+ struct RegInfo {
+ uint32_t num_registers;
+ uint32_t num_gpr_registers;
+ uint32_t num_fpr_registers;
+
+ uint32_t last_gpr;
+ uint32_t first_fpr;
----------------
I'm not sure what's the advantage of making this a struct vs. just splating it
into the containing class. Maybe that made more sense when originally when the
class was doing a lot more work, but here I'd just replace this by a list of
members.
================
Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:82
switch (arch.GetTriple().getOS()) {
case llvm::Triple::FreeBSD: {
----------------
The reg(set)_interface and register_context switches could be merged here too
in a similar way...
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80105/new/
https://reviews.llvm.org/D80105
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
