labath 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:
> 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...
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
