[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

This revision was automatically updated to reflect the committed changes. Closed by commit rG7e017de0ad62: AArch64 SVE register infos and core file support (authored by omjavaid). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 279151. omjavaid added a comment. This update makes minor adjustments before merge in light of final comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77047/new/ https://reviews.llvm.org/D77047 Files: lldb/source/Plugins/Process/Utility/Re

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. I think this is as good as we can get right now. Thanks for sticking through. Two quick comments inline. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOS

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid marked an inline comment as done. omjavaid added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:63-66 +m_sve_note_payload.resize(m_sveregset.GetByteSize()); +::memcpy(GetSVEBuffer(), m_sveregset.GetDataStart(

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 278617. omjavaid added a comment. This update plugs the holes susceptible to bugs by endianity variations. I have used DataExtractor GetU16 to read flags and vl from user_sve_header. Also removed the memset and replaced it with SetFromMemoryData function.

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:63-66 +m_sve_note_payload.resize(m_sveregset.GetByteSize()); +::memcpy(GetSVEBuffer(), m_sveregset.GetDataStart(), + m_sveregset.GetByteSize());

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 278435. omjavaid added a comment. In this update I have addressed issues highlighted in last review iteration. @labath Does this now look LGTM? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77047/new/ https://reviews.llvm.org/D77047 Files: lldb

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid marked 9 inline comments as done. omjavaid added inline comments. Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp:289 + +lldb_private::RegisterInfo *new_reg_info_p = reg_info_ref.data(); + labath wrote: > I think all uses

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added a comment. At this point, I think I (finally) have a good understanding of both how this patch works and interacts with the rest of the world. I have one more batch of comments, but hopefully none are too controversial, and I really do hope this is the last iteration. ==

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 278124. omjavaid added a comment. Minor fix removing GetRegNumP0 which is no longer needed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77047/new/ https://reviews.llvm.org/D77047 Files: lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 278113. omjavaid added a comment. This update address issues highlighted in last iteration and also minimizes the use of SVE_PT macros by using RegisterInfo.byte_size and byte_offset where possible. @labath What do you think ? CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid marked an inline comment as done. omjavaid added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106 +uint32_t +RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const { + uint32_t sve_offset =

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106 +uint32_t +RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const { + uint32_t sve_offset = 0; + if (m_sve_state == SVE_STATE::SVE_STATE

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid marked an inline comment as done. omjavaid added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106 +uint32_t +RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const { + uint32_t sve_offset =

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106 +uint32_t +RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const { + uint32_t sve_offset = 0; + if (m_sve_state == SVE_STATE::SVE_STATE

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid marked an inline comment as done. omjavaid added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106 +uint32_t +RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const { + uint32_t sve_offset =

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:82-106 +uint32_t +RegisterContextCorePOSIX_arm64::CalculateSVEOffset(uint32_t reg_num) const { + uint32_t sve_offset = 0; + if (m_sve_state == SVE_STATE::SVE_STATE

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid marked 6 inline comments as done. omjavaid added a comment. In D77047#2125220 , @labath wrote: > There's always a chance for ODR violations, particularly with header files > defining static objects. This looks better though what I really wanted w

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added a comment. Some more comments (and questions) from me. Sorry about the back-and-forth. It's taking me a while to wrap my head around this, and as I start to understand things, new questions arise... Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOS

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 277701. omjavaid added a comment. This update addresses issues raised in last iteration. Also have removed dependence on lldb-arm64-register-enums.h. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77047/new/ https://reviews.llvm.org/D77047 Files:

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added a comment. I didn't notice this before, but I see now that there's more register number overlap in `lldb-arm64-register-enums.h`. Having one overlapping enum is bad enough, but two seems really too much? Can we avoid the second enum somehow, at least? Perhaps by switching `Register

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 276963. omjavaid added a comment. This update incorporates suggestion from last iteration and also rebases after merger to registersets and register infos in RegisterInfosPOSIX_arm64. Also I have posted a separate patch D83541

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath marked an inline comment as done. labath added a comment. Thanks. This looks is starting to look good now. I didn't get a chance to review all of the changes, but here's a few more things that I noticed. Comment at: lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 275037. omjavaid added a comment. @labath I have updated diff after resolving issues highlighted. Also I have removed dependence on generic interfaces. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77047/new/ https://reviews.llvm.org/D77047 Files

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid marked 6 inline comments as done. omjavaid added a comment. In D77047#2125220 , @labath wrote: > There's always a chance for ODR violations, particularly with header files > defining static objects. This looks better though what I really wanted w

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added a comment. There's always a chance for ODR violations, particularly with header files defining static objects. This looks better though what I really wanted was to keep the non-sve register info array (`g_register_infos_arm64_le`) completely out of the path of the sve header. Like,

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 274390. omjavaid added a comment. In this updated I have removed overlapping parts of RegisterInfos_arm64.h and RegisterInfos_arm64_sve.h which in turn removes any possibility of duplicate definitions. Both register infos define separate static register in

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added a comment. I'm worried about the confusing relationship between `RegisterInfos_arm64_sve.h` and `RegisterInfos_arm64.h` and the overlap in the register numbers it enforces. when we discussed this last time, my understanding was that the sve registers could be inserted _before_ the

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 271532. omjavaid added a comment. This update remove skipping over register infos requirement by overlapping sve register numbers with debug register numbers which are not required by Linux register context. CHANGES SINCE LAST ACTION https://reviews.llv

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

omjavaid updated this revision to Diff 263934. omjavaid added a comment. New update contains minor update needed to accommodate SVE PTrace macros header from ARM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77047/new/ https://reviews.llvm.org/D77047 Files: lldb/include/lldb/Target

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and core file support

labath added a comment. (sorry about all the editing -- I'm trying to figure out the order of these patches) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77047/new/ https://reviews.llvm.org/D77047 ___ lldb-commits mailing list lldb-commit