labath added a comment. In D109695#2999221 <https://reviews.llvm.org/D109695#2999221>, @mgorny wrote:
> In D109695#2999171 <https://reviews.llvm.org/D109695#2999171>, @labath wrote: > >> Let's take a step back. First, I'd like to understand why are you adding a >> whole new register, instead of just an alias (alt_name) for an existing >> register. > > Well, I was thinking 'what if the register has already another alt_name?' > While it's unlikely that such a thing would happen with gdbserver, it's valid > input in the end, and adding a new register seemed a more reliable solution > for arbitrary input. While we shouldn't crash on such an input, I am not particularly worried about everything still functioning perfectly. And adding a new register has user-visible consequences (two entries in `register read`). Overall, I think I'd be fine both with overwriting the alt-name in this case (this is an lldb construct anyway) and with just doing nothing. >> And second, have you considered putting this into the ABI plugin, as that's >> where the rest of the "augmentation" code lives (or most of it, anyway). > > I initially did, yes, and I've figured out that it doesn't belong there since > 1) these assignments aren't really specific to a single ABI but are general > architecture characteristics, and 2) the logic is really specific to the > gdb-remote plugin. > > I mean, putting things like generic regno assignments into ABI makes sense > because different ABIs could use different registers for given purpose. > However, things like ESP being derived from RSP or aliasing sp to x31 seem > ABI-independent. That is true, and it may actually make more sense to put this into the Architecture plugin. However, I am not sure that this architectural purity is worth the hassle of having RegisterInfo knowledge in two places, especially since the ABI plugins are nowadays structured in a way that the all ABIs for a given architecture share a common base class, and this common logic could easily go there. (For that reason, I am also not sure we really need a separate Architecture hierarchy, but anyway...) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109695/new/ https://reviews.llvm.org/D109695 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
