[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

This revision was automatically updated to reflect the committed changes. Closed by commit rG78cb4562faa7: Make offset field optional in RegisterInfo packet for Arm64 (authored by omjavaid). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. In D91241#2422571 , @omjavaid wrote: > In D91241#2422517 , @labath wrote: > >> Yes, but there's no reason that

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid updated this revision to Diff 308339. omjavaid added a comment. Updated after incorporating suggested changes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91241/new/ https://reviews.llvm.org/D91241 Files: lldb/include/lldb/Host/common/NativeRegisterContext.h

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid added a comment. In D91241#2422517 , @labath wrote: > In D91241#2422403 , @omjavaid wrote: > >> In D91241#2422100 , @labath wrote: >> >>> Looks great. One question

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

labath added a comment. In D91241#2422403 , @omjavaid wrote: > In D91241#2422100 , @labath wrote: > >> Looks great. One question about the member variable... > > m_remote_to_local_regnum_map is used by AddRegister

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid added a comment. In D91241#2422100 , @labath wrote: > Looks great. One question about the member variable... m_remote_to_local_regnum_map is used by AddRegister where we add a (key,value) pair for each of registers. We are doing this to

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

labath added a comment. Looks great. One question about the member variable... Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h:90 dynamic_reg_size_map m_dynamic_reg_size_map; + reg_to_reg_map m_remote_to_local_regnum_map; size_t

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid updated this revision to Diff 307954. omjavaid added a comment. This update improves on last patch by adding support for register ordering in increasing order of register number even if register numbers are used randomly in target xml. Also we set offset to LLDB_INVALID_INDEX32 while

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid added a comment. In D91241#2402491 , @labath wrote: > Yes, this is pretty much what I hoped for. Thanks. > > Besides relaxing existing tests, it would be nice to have a positive test > that checks for the behavior that we want. Maybe a

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

labath added a comment. Yes, this is pretty much what I hoped for. Thanks. Besides relaxing existing tests, it would be nice to have a positive test that checks for the behavior that we want. Maybe a gdb-client test which checks that the register values are extracted from the appropriate place

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid added a comment. In D91241#2397083 , @labath wrote: > I disagree, and my main reason for that is that this approach requires > hardcoding the "supported architectures" for which we do this fixup in the > DynamicRegisterInfo class. Besides being

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid updated this revision to Diff 305680. omjavaid added a comment. This update tries to fix concerns raised by removing AArch64 specific code and makes offset assignment more generic. We now have a new flag which is set to true if offset field was present and on basis of that we assign

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

labath added a comment. I disagree, and my main reason for that is that this approach requires hardcoding the "supported architectures" for which we do this fixup in the DynamicRegisterInfo class. Besides being wrong as a matter of priniciple, I suspect this will also be a problem in practice

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid added a comment. In D91241#2391245 , @labath wrote: > In D91241#2391156 , @omjavaid wrote: > >> I guess GDB standard does enforce ascending order, here is what it says >> about regnum: >> >> "The

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

labath added a comment. In D91241#2391156 , @omjavaid wrote: > I guess GDB standard does enforce ascending order, here is what it says about > regnum: > > "The register’s number. If omitted, a register’s number is one greater than > that of the previous

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid added a comment. In D91241#2391126 , @labath wrote: > That's a good point. Maybe this does need to be a two-pass algorithm (first > compute the offsets of primary registers, then fill out subregs). But that > doesn't mean the two passes should

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

labath added a comment. That's a good point. Maybe this does need to be a two-pass algorithm (first compute the offsets of primary registers, then fill out subregs). But that doesn't mean the two passes should be in two completely separate files. It would still be better if that was done in a

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid added a comment. In D91241#2391072 , @labath wrote: > This looks pretty straightforward, modulo the inline comment about the > centralization of the offset computations So the current approach of finalizing offsets of pseudo registers after

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

labath added a comment. This looks pretty straightforward, modulo the inline comment about the centralization of the offset computations Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:534 + // On AArch64 architecture we set offsets on client

[Lldb-commits] [PATCH] D91241: [LLDB] Make offset field optional in RegisterInfo packet for Arm64

omjavaid created this revision. omjavaid added a reviewer: labath. Herald added a subscriber: kristof.beyls. omjavaid requested review of this revision. This patch carries forward our aim to remove offset field from qRegisterInfo packets and XML register description. I have created a new