[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextWindows_i386.cpp:70-103
+static const RegisterInfo *GetRegisterInfoPtr(const ArchSpec &target_arch) {
+  switch (target_arch.GetMachine()) {
+  case llvm::Triple::x86:
+    return g_register_infos_i386;
+  default:
+    llvm_unreachable("Unhandled target architecture.");
+  }
----------------
I think all of this could be bolied down to:
```
RegisterContextWindows_i386::RegisterContextWindows_i386(
    const ArchSpec &target_arch)
    : lldb_private::RegisterInfoInterface(target_arch),
      m_register_info_p(g_register_infos_i386),
      m_register_info_count(llvm::array_lengthof(g_register_infos_i386)),
      m_user_register_count(llvm::array_lengthof(g_register_infos_i386)) {
  assert(target_arch.GetMachine() == llvm::Triple::x86);
}
```

In fact, I'd probably not even bother with the member variables, but have the public RegisterContextWindows_i386 functions return the values directly (just like GetGPRSize does).


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextWindows_x86_64.cpp:141
+GetRegisterInfo_WoW64(const lldb_private::ArchSpec &arch) {
+  // A WoW64 register info is the same as the i386's.
+  std::vector<lldb_private::RegisterInfo> &g_register_infos =
----------------
asmith wrote:
> labath wrote:
> > Hui wrote:
> > > labath wrote:
> > > > Why is all of this complexity necessary? Couldn't you just switch on the target architecture in `CreateRegisterInfoInterface` in NativeRegisterContextWindows_x86_64.cpp and create either `RegisterContextWindows_WoW64` or `RegisterContextWindows_x86_64` ?
> > > > 
> > > > In fact, if RegisterContextWindows_WoW64 is identical to RegisterContextWindows_i386, then why do you even need the _WoW64 version of the class in the first place? FWIW, linux also does not have a RegisterContextLinux_32_bit_process_on_64_bit_kernel class...
> > > I think WoW64 is i686 that shall deserve a separate arch specific implementation?
> > I am sorry, but I don't follow. Can you repeat the question?
> > 
> > (FWIW, I don't believe that the fact that two things are different from a certain point of view justifies copy-pasting (at least) 150 LOC. If you think it's confusing to use something called "i386" in things dealing with WoW64, you can always typedef the WOW64 context to it, or rename the i386 context to something more generic.)
> I don't want this to block the review and have removed the code.
Thanks. FTR, I'm not opposed to splitting these classes again in the future, if we develop a need for them to diverge (though it would be nice to find a way to factor the common parts in that case).

However, there's one more thing that bothers me here. It's this business with constructing a `RegisterContextWindows_i386` here, copying the entries out of it, and re-using them elsewhere while asserting that things were done in the right order. It seems very complex, and I'm not sure that complexity is needed. It seems to me like all of that could be removed if you just made the decision which set of registers to use one level up (i.e., in `CreateRegisterInfoInterface` in NativeRegisterContextWindows_x86_64.cpp. You could just have that function switch on the process byte size, and return RegisterContextWindows_i386 or RegisterContextWindows_x86_64. This is the same way things are done on x86 linux (see NativeRegisterContextLinux_x86_64.cpp).


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

https://reviews.llvm.org/D63165