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

Tue Jul 30 17:10:18 PDT 2019

asmith marked 2 inline comments as done.
asmith added a comment.

I don't see anything else to address in this review. Comments?

================
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 =
----------------
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.

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

https://reviews.llvm.org/D63165