omjavaid added a comment. This looks ok to me apart from some cosmetic nits. Also you may figure out a way to clean up the whole address including pauth now after D99944 <https://reviews.llvm.org/D99944> is merged.
================ Comment at: lldb/include/lldb/Target/MemoryTagManager.h:72 + // transport. + virtual size_t GetBytesPerTag() const = 0; + ---------------- Bytes per tag conveys a different message like "No of bytes of memory this tag corresponds to" It could GetTagByteSize or something. ================ Comment at: lldb/include/lldb/Target/MemoryTagManager.h:79 + virtual llvm::Expected<std::vector<lldb::addr_t>> + UnpackTags(const std::vector<uint8_t> &tags, size_t granules) const = 0; + ---------------- This might also be renamed to UnpackTagsData because input is not guaranteed to be tags unless their byte size is 1. ================ Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:28 + // user metadata. + return addr & ~((lldb::addr_t)0xFF << MTE_START_BIT); +} ---------------- This function need to take care of Pointer Auth mask which could be living in bits 48:54 ================ Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp:61 + size_t new_len = range.GetByteSize() + align_down_amount; + // Then align up to the end of the granule + if (new_len % granule) ---------------- May be consider doing something like below to avoid calculating mod twice. lldb::addr_t align_up_amount = new_len % granule; if (align_up_amount) .... ================ Comment at: lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h:24 + }; + + lldb::addr_t GetLogicalTag(lldb::addr_t addr) const override; ---------------- All functions below have no spacing between them may be consider adding a few spaces while combining declaration of setter/getter or other similar function in consecutive lines. ================ Comment at: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp:73 + + // Reading less than 1 granule, rounds up to 1 granule + ASSERT_EQ( ---------------- This is confusing // Reading less than 1 granule, rounds up to 1 granule May be reword a little like "No of bytes being read are less than bytes per granule" ================ Comment at: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp:100 + ASSERT_EQ(0, 0); + ASSERT_EQ((lldb::addr_t)0x00ffeedd11223344, + manager.RemoveNonAddressBits(0x00ffeedd11223344)); ---------------- This should be able to remove pointer auth mask so fix this test accordingly. ================ Comment at: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp:116 + // Anything in the top byte is ignored + ASSERT_EQ(0, manager.AddressDiff(0x2211222233334444, 0x3311222233334444)); + ASSERT_EQ(-32, manager.AddressDiff(0x5511222233334400, 0x4411222233334420)); ---------------- likewise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97281/new/ https://reviews.llvm.org/D97281 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
