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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
