omjavaid marked 10 inline comments as done.
omjavaid added inline comments.
================
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:
----------------
labath wrote:
> 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.
I have tried a couple of options but the no of different combinations involved
I feel current implementation should stay untill we incrementally move
remaining architectures to user RegisterInfosAndSet interface.
================
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);
----------------
labath wrote:
> 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.)
Agreed. I will update this in updated revision.
================
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);
}
----------------
labath wrote:
> 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`.
yes this would be a lot cleaner than what I am doing currently. I am going to
update this in next revision.
================
Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:82
switch (arch.GetTriple().getOS()) {
case llvm::Triple::FreeBSD: {
----------------
labath wrote:
> The reg(set)_interface and register_context switches could be merged here too
> in a similar way...
Here also incremental merger will be a better approach IMO.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80105/new/
https://reviews.llvm.org/D80105
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
