omjavaid added a comment. In D82863#2330122 <https://reviews.llvm.org/D82863#2330122>, @labath wrote:
> In D82863#2324560 <https://reviews.llvm.org/D82863#2324560>, @omjavaid wrote: > >> In D82863#2321647 <https://reviews.llvm.org/D82863#2321647>, @labath wrote: >> >>> In D82863#2321370 <https://reviews.llvm.org/D82863#2321370>, @omjavaid >>> wrote: >>> >>>> In D82863#2320342 <https://reviews.llvm.org/D82863#2320342>, @jasonmolenda >>>> wrote: >>>> >>>>> The original g/G packets were designed for little embedded systems where >>>>> the stub had to be very small and dumb, and could just memcpy the payload >>>>> in the packet into the register context & back out again. Any sensible >>>>> design today would, at least, have some form of an array of regnum:regval >>>>> to eliminate many of the problems. >>>>> >>>>>> Unless of course, we make sure SVE regs come last, but that imposes some >>>>>> constraints on future registers sets. I suppose we might be able to say >>>>>> that all variable-length or otherwise-funny registers must come last. >>>>> >>>>> Yeah, Omair's patch currently assumes that the SVE regs come last already >>>>> when they copy & truncate the registers context to heap. I fear that >>>>> we'll get to armv12 and we'll be adding registers after the SVE and >>>>> wonder why they're being truncated somewhere in lldb. :) >>> >>> Well.. we could design it such that the SVE registers (and any other >>> dynamically-sized regs) always come last. Then they wouldn't impact the >>> offsets of static registers. >>> >>> I don't think we have a requirement that newer register sets must be >>> appended. Or at least, we shouldn't have, as now both intel and arm have >>> cpu features (and registers) that may be arbitrarily combined in future >>> processors. >> >> For now I am considering variable sized registers i-e SVE Z and P registers >> to always come last which is currently the case and will be in future >> patches which add support for Pointer Authentication and MTE registers. >> >>>>> @omjavaid , what do you think about disabling g/G when we're working with >>>>> SVE registers (GDBRemoteCommunicationClient::AvoidGPackets)? There are >>>>> some gdb-remote stubs that can *only* read/write registers with g/G, but >>>>> I think it's reasonable to say "you must implement p/P for a target with >>>>> SVE", that's a generic packet shared by both lldb and gdb. We could add >>>>> a more-modern g/G replacement packet, but no one would have that >>>>> implemented, and if they were going to add anything, I'd have them >>>>> implement p/P unless it's perf problems and they need a >>>>> read-all-registers / write-all-registers packet that works with SVE. >> >> Just scrolling through GDB log one of the answers about target XML is that >> target XML is not exchanged for native connection where gdbserver is not >> being used. For gdbserver connection, target xml is exchanged onces per >> connection and register sizes are updated based on vg after that internally. >> However testing gdb with same executable as the one I wrote for LLDB was >> having gdb remote connection crash which could be something to do with SVE >> or some other bug. > > Interesting. > > I started drafting an email to the gdb mailing list to try to clarify this > thing. For that, I was re-reading the gdb docs for the qXfer:target.xml > packet, I realized that the gdb interpretation is mostly unambiguous about > this thing: > > - **regnum**: The register’s number. If omitted, <default value > algorithm...>. This register number is used to read or write the register; > e.g. it is used in the remote p and P packets, and **registers appear in the > g and G packets in order of increasing register number**. > > I'm sure that whoever wrote this did not have variable-length registers in > mind. However, it's interpretation for variable-length registers is pretty > clear: take each register, and dump its bytes in sequence (no matter many of > them are in the register at this particular time). The reason this is > unambiguous is because gdb does not have the "offset" attribute of the "reg" > element, and so the only way to find its 'g' offset is via this algorithm. > And given your explanation about "switching" target.xml descriptions at > runtime, I'm would expect this is what is happening within gdb. > > The problem is that he "offset" attribute (added by lldb) makes things > ambiguous, as it provides another way to find a register in the 'g' blob. So > how about we "fix" that ambiguity by having lldb-server omit the "offset" > attribute for variable-length registers? > > Sending it over is actively harmful, as the value is very likely to be wrong, > and I believe that in your current implementation these numbers are pretty > much ignored by the client anyway. Lldb should already have code to > automatically compute the regiser offsets when the "offset" attribute is > missing (though it's possible it may not handle partially missing values > correctly). This code would need to be extended to recompute those offsets > whenever register sizes change (which is roughly what this patch does). We > could also extend the parser with a check to ensure that all registers with > explicit offsets come before offset-less registers. That would ensure that > the register offsets dynamically computed by the client never overlap with > any registers whose offsets are explicitly given by the server. > > How does that sound? I think the algorithm you are suggestion is quite workable. Summarizing our discussion above: 1. registers should be sent over by lldb-server (via qRegisterInfo packet or target XML packet) in order of increasing register numbers 2. If its a non-pseudo register that is not contained by any other register then its place is decided on the bases of its sequence via register number This makes a register's name, size , type encoding and whether it is contained by any other register, the most important information while the remaining information including (dwarf and eh_frame reg numbers can be avoided somehow not sure exactly how??) Also there are two fields that are sent over invalidate-regs and container-regs I think they are one and same thing. Moreover the encoding and format are also two difference field where I think we need only one of these. So now comes the point where we want to decided how much information we safely can skip and do we want to skip that information as part of this patch blocking it or let it go ahead and make changes in a follow up. For Arm/AArch64 I see no trouble sending no offset and calculating an offset locally when register infos are parsed. I dont think other architectures should have any trouble as far as process gdb-remote is concerned as most of these architecture must be following the same guidelines. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82863/new/ https://reviews.llvm.org/D82863 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
