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

[Aaron Smith via Phabricator via lldb-commits]

asmith marked an inline comment as done.
asmith added inline 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:
> labath wrote:
> > 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).
> Unfortunately NativeRegisterContextLinux_x86_64 is not good example here. It turns I misread, and that class actually switches on the *host* architecture instead of *target*, and its implementation of RegisterContextLinux_x86_64 does indeed the i386 copying tricks that are very similar to what you do here. However, that class has the "excuse" of needing to fix up the register offsets in the i386-on-x86_64 case (for those following along: see `UPDATE_REGISTER_INFOS_I386_STRUCT_WITH_X86_64_OFFSETS`). You don't need to do anything like that here, so I would hope that we can do something simpler here.
> 
> And I'll try to figure out a way to do the linux thing in a saner fashion... 
Are you accepting this review? 


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

https://reviews.llvm.org/D63165