[Lldb-commits] [lldb] d046fb6 - [lldb][AArch64] Refactor memory tag range handling
David Spickett via lldb-commits
lldb-commits at lists.llvm.org
Fri Jul 16 03:02:13 PDT 2021
Author: David Spickett
Date: 2021-07-16T11:02:06+01:00
New Revision: d046fb62b7e7cd273b104fee0162725003411c00
URL: https://github.com/llvm/llvm-project/commit/d046fb62b7e7cd273b104fee0162725003411c00
DIFF: https://github.com/llvm/llvm-project/commit/d046fb62b7e7cd273b104fee0162725003411c00.diff
LOG: [lldb][AArch64] Refactor memory tag range handling
Previously GetMemoryTagManager checked many things in one:
* architecture supports memory tagging
* process supports memory tagging
* memory range isn't inverted
* memory range is all tagged
Since writing follow up patches for tag writing (in review
at the moment) it has become clear that this gets unwieldy
once we add the features needed for that.
It also implies that the memory tag manager is tied to the
range you used to request it with but it is not. It's a per
process object.
Instead:
* GetMemoryTagManager just checks architecture and process.
* Then the MemoryTagManager can later be asked to check a
memory range.
This is better because:
* We don't imply that range and manager are tied together.
* A slightly diferent range calculation for tag writing
doesn't add more code to Process.
* Range checking code can now be unit tested.
Reviewed By: omjavaid
Differential Revision: https://reviews.llvm.org/D105630
Added:
Modified:
lldb/include/lldb/Target/MemoryTagManager.h
lldb/include/lldb/Target/Process.h
lldb/source/Commands/CommandObjectMemoryTag.cpp
lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
lldb/source/Target/Process.cpp
lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/MemoryTagManager.h b/lldb/include/lldb/Target/MemoryTagManager.h
index f6383d82efcd7..65abfcd763827 100644
--- a/lldb/include/lldb/Target/MemoryTagManager.h
+++ b/lldb/include/lldb/Target/MemoryTagManager.h
@@ -9,6 +9,7 @@
#ifndef LLDB_TARGET_MEMORYTAGMANAGER_H
#define LLDB_TARGET_MEMORYTAGMANAGER_H
+#include "lldb/Target/MemoryRegionInfo.h"
#include "lldb/Utility/RangeMap.h"
#include "lldb/lldb-private.h"
#include "llvm/Support/Error.h"
@@ -57,6 +58,20 @@ class MemoryTagManager {
// expanded to 2 granules.
virtual TagRange ExpandToGranule(TagRange range) const = 0;
+ // Given a range addr to end_addr, check that:
+ // * end_addr >= addr (when memory tags are removed)
+ // * the granule aligned range is completely covered by tagged memory
+ // (which may include one or more memory regions)
+ //
+ // If so, return a modified range which will have been expanded
+ // to be granule aligned.
+ //
+ // Tags in the input addresses are ignored and not present
+ // in the returned range.
+ virtual llvm::Expected<TagRange> MakeTaggedRange(
+ lldb::addr_t addr, lldb::addr_t end_addr,
+ const lldb_private::MemoryRegionInfos &memory_regions) const = 0;
+
// Return the type value to use in GDB protocol qMemTags packets to read
// allocation tags. This is named "Allocation" specifically because the spec
// allows for logical tags to be read the same way, though we do not use that.
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index b125a38636c8b..2457ac31c19db 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -1709,30 +1709,19 @@ class Process : public std::enable_shared_from_this<Process>,
lldb::addr_t CallocateMemory(size_t size, uint32_t permissions,
Status &error);
- /// If the address range given is in a memory tagged range and this
- /// architecture and process supports memory tagging, return a tag
+ /// If this architecture and process supports memory tagging, return a tag
/// manager that can be used to maniupulate those memory tags.
- /// Tags present in the addresses given are ignored.
- ///
- /// \param[in] addr
- /// Start of memory range.
- ///
- /// \param[in] end_addr
- /// End of the memory range. Where end is one beyond the last byte to be
- /// included.
///
/// \return
/// Either a valid pointer to a tag manager or an error describing why one
/// could not be provided.
- llvm::Expected<const MemoryTagManager *>
- GetMemoryTagManager(lldb::addr_t addr, lldb::addr_t end_addr);
+ llvm::Expected<const MemoryTagManager *> GetMemoryTagManager();
- /// Expands the range addr to addr+len to align with granule boundaries and
- /// then calls DoReadMemoryTags to do the target specific operations.
- /// Tags are returned unpacked so can be used without conversion.
+ /// Read memory tags for the range addr to addr+len. It is assumed
+ /// that this range has already been granule aligned.
+ /// (see MemoryTagManager::MakeTaggedRange)
///
- /// \param[in] tag_manager
- /// The tag manager to get memory tagging information from.
+ /// This calls DoReadMemoryTags to do the target specific operations.
///
/// \param[in] addr
/// Start of memory range to read tags for.
@@ -1741,11 +1730,12 @@ class Process : public std::enable_shared_from_this<Process>,
/// Length of memory range to read tags for (in bytes).
///
/// \return
- /// Either the unpacked tags or an error describing a failure to read
- /// or unpack them.
- llvm::Expected<std::vector<lldb::addr_t>>
- ReadMemoryTags(const MemoryTagManager *tag_manager, lldb::addr_t addr,
- size_t len);
+ /// If this architecture or process does not support memory tagging,
+ /// an error saying so.
+ /// If it does, either the memory tags or an error describing a
+ /// failure to read or unpack them.
+ llvm::Expected<std::vector<lldb::addr_t>> ReadMemoryTags(lldb::addr_t addr,
+ size_t len);
/// Resolve dynamically loaded indirect functions.
///
diff --git a/lldb/source/Commands/CommandObjectMemoryTag.cpp b/lldb/source/Commands/CommandObjectMemoryTag.cpp
index 07dccf5c16fb0..1dfb32a92f3bb 100644
--- a/lldb/source/Commands/CommandObjectMemoryTag.cpp
+++ b/lldb/source/Commands/CommandObjectMemoryTag.cpp
@@ -68,7 +68,7 @@ class CommandObjectMemoryTagRead : public CommandObjectParsed {
Process *process = m_exe_ctx.GetProcessPtr();
llvm::Expected<const MemoryTagManager *> tag_manager_or_err =
- process->GetMemoryTagManager(start_addr, end_addr);
+ process->GetMemoryTagManager();
if (!tag_manager_or_err) {
result.SetError(Status(tag_manager_or_err.takeError()));
@@ -76,9 +76,21 @@ class CommandObjectMemoryTagRead : public CommandObjectParsed {
}
const MemoryTagManager *tag_manager = *tag_manager_or_err;
- ptr
diff _t len = tag_manager->AddressDiff(end_addr, start_addr);
- llvm::Expected<std::vector<lldb::addr_t>> tags =
- process->ReadMemoryTags(tag_manager, start_addr, len);
+
+ MemoryRegionInfos memory_regions;
+ // If this fails the list of regions is cleared, so we don't need to read
+ // the return status here.
+ process->GetMemoryRegions(memory_regions);
+ llvm::Expected<MemoryTagManager::TagRange> tagged_range =
+ tag_manager->MakeTaggedRange(start_addr, end_addr, memory_regions);
+
+ if (!tagged_range) {
+ result.SetError(Status(tagged_range.takeError()));
+ return false;
+ }
+
+ llvm::Expected<std::vector<lldb::addr_t>> tags = process->ReadMemoryTags(
+ tagged_range->GetRangeBase(), tagged_range->GetByteSize());
if (!tags) {
result.SetError(Status(tags.takeError()));
@@ -89,8 +101,7 @@ class CommandObjectMemoryTagRead : public CommandObjectParsed {
tag_manager->GetLogicalTag(start_addr));
result.AppendMessage("Allocation tags:");
- MemoryTagManager::TagRange initial_range(start_addr, len);
- addr_t addr = tag_manager->ExpandToGranule(initial_range).GetRangeBase();
+ addr_t addr = tagged_range->GetRangeBase();
for (auto tag : *tags) {
addr_t next_addr = addr + tag_manager->GetGranuleSize();
// Showing tagged adresses here until we have non address bit handling
diff --git a/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp b/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
index 987a55e7e8575..e44a10da73afe 100644
--- a/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
+++ b/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
@@ -66,6 +66,63 @@ MemoryTagManagerAArch64MTE::ExpandToGranule(TagRange range) const {
return TagRange(new_start, new_len);
}
+llvm::Expected<MemoryTagManager::TagRange>
+MemoryTagManagerAArch64MTE::MakeTaggedRange(
+ lldb::addr_t addr, lldb::addr_t end_addr,
+ const lldb_private::MemoryRegionInfos &memory_regions) const {
+ // First check that the range is not inverted.
+ // We must remove tags here otherwise an address with a higher
+ // tag value will always be > the other.
+ ptr
diff _t len = AddressDiff(end_addr, addr);
+ if (len <= 0) {
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "End address (0x%" PRIx64
+ ") must be greater than the start address (0x%" PRIx64 ")",
+ end_addr, addr);
+ }
+
+ // Region addresses will not have memory tags. So when searching
+ // we must use an untagged address.
+ MemoryRegionInfo::RangeType tag_range(RemoveNonAddressBits(addr), len);
+ tag_range = ExpandToGranule(tag_range);
+
+ // Make a copy so we can use the original for errors and the final return.
+ MemoryRegionInfo::RangeType remaining_range(tag_range);
+
+ // While there are parts of the range that don't have a matching tagged memory
+ // region
+ while (remaining_range.IsValid()) {
+ // Search for a region that contains the start of the range
+ MemoryRegionInfos::const_iterator region = std::find_if(
+ memory_regions.cbegin(), memory_regions.cend(),
+ [&remaining_range](const MemoryRegionInfo ®ion) {
+ return region.GetRange().Contains(remaining_range.GetRangeBase());
+ });
+
+ if (region == memory_regions.cend() ||
+ region->GetMemoryTagged() != MemoryRegionInfo::eYes) {
+ // Some part of this range is untagged (or unmapped) so error
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Address range 0x%" PRIx64 ":0x%" PRIx64
+ " is not in a memory tagged region",
+ tag_range.GetRangeBase(),
+ tag_range.GetRangeEnd());
+ }
+
+ // We've found some part of the range so remove that part and continue
+ // searching for the rest. Moving the base "slides" the range so we need to
+ // save/restore the original end. If old_end is less than the new base, the
+ // range will be set to have 0 size and we'll exit the while.
+ lldb::addr_t old_end = remaining_range.GetRangeEnd();
+ remaining_range.SetRangeBase(region->GetRange().GetRangeEnd());
+ remaining_range.SetRangeEnd(old_end);
+ }
+
+ // Every part of the range is contained within a tagged memory region.
+ return tag_range;
+}
+
llvm::Expected<std::vector<lldb::addr_t>>
MemoryTagManagerAArch64MTE::UnpackTagsData(const std::vector<uint8_t> &tags,
size_t granules) const {
diff --git a/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h b/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
index ef1d0791bcd09..55d80c8ead53e 100644
--- a/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
+++ b/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
@@ -32,6 +32,10 @@ class MemoryTagManagerAArch64MTE : public MemoryTagManager {
TagRange ExpandToGranule(TagRange range) const override;
+ llvm::Expected<TagRange> MakeTaggedRange(
+ lldb::addr_t addr, lldb::addr_t end_addr,
+ const lldb_private::MemoryRegionInfos &memory_regions) const override;
+
llvm::Expected<std::vector<lldb::addr_t>>
UnpackTagsData(const std::vector<uint8_t> &tags,
size_t granules) const override;
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index a169b4623f3ee..2907c71054f08 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6067,8 +6067,7 @@ bool Process::CallVoidArgVoidPtrReturn(const Address *address,
return false;
}
-llvm::Expected<const MemoryTagManager *>
-Process::GetMemoryTagManager(lldb::addr_t addr, lldb::addr_t end_addr) {
+llvm::Expected<const MemoryTagManager *> Process::GetMemoryTagManager() {
Architecture *arch = GetTarget().GetArchitecturePlugin();
const MemoryTagManager *tag_manager =
arch ? arch->GetMemoryTagManager() : nullptr;
@@ -6084,66 +6083,22 @@ Process::GetMemoryTagManager(lldb::addr_t addr, lldb::addr_t end_addr) {
"Process does not support memory tagging");
}
- ptr
diff _t len = tag_manager->AddressDiff(end_addr, addr);
- if (len <= 0) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "End address (0x%" PRIx64
- ") must be greater than the start address (0x%" PRIx64 ")",
- end_addr, addr);
- }
-
- // Region lookup is not address size aware so mask the address
- MemoryRegionInfo::RangeType tag_range(tag_manager->RemoveNonAddressBits(addr),
- len);
- tag_range = tag_manager->ExpandToGranule(tag_range);
-
- // Make a copy so we can use the original range in errors
- MemoryRegionInfo::RangeType remaining_range(tag_range);
-
- // While we haven't found a matching memory region for some of the range
- while (remaining_range.IsValid()) {
- MemoryRegionInfo region;
- Status status = GetMemoryRegionInfo(remaining_range.GetRangeBase(), region);
-
- if (status.Fail() || region.GetMemoryTagged() != MemoryRegionInfo::eYes) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "Address range 0x%lx:0x%lx is not in a memory tagged region",
- tag_range.GetRangeBase(), tag_range.GetRangeEnd());
- }
-
- if (region.GetRange().GetRangeEnd() >= remaining_range.GetRangeEnd()) {
- // We've found a region for the whole range or the last piece of a range
- remaining_range.SetByteSize(0);
- } else {
- // We've found some part of the range, look for the rest
- remaining_range.SetRangeBase(region.GetRange().GetRangeEnd());
- }
- }
-
return tag_manager;
}
llvm::Expected<std::vector<lldb::addr_t>>
-Process::ReadMemoryTags(const MemoryTagManager *tag_manager, lldb::addr_t addr,
- size_t len) {
- if (!tag_manager) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "A memory tag manager is required for reading memory tags.");
- }
-
- MemoryTagManager::TagRange range(tag_manager->RemoveNonAddressBits(addr),
- len);
- range = tag_manager->ExpandToGranule(range);
+Process::ReadMemoryTags(lldb::addr_t addr, size_t len) {
+ llvm::Expected<const MemoryTagManager *> tag_manager_or_err =
+ GetMemoryTagManager();
+ if (!tag_manager_or_err)
+ return tag_manager_or_err.takeError();
+ const MemoryTagManager *tag_manager = *tag_manager_or_err;
llvm::Expected<std::vector<uint8_t>> tag_data =
- DoReadMemoryTags(range.GetRangeBase(), range.GetByteSize(),
- tag_manager->GetAllocationTagType());
+ DoReadMemoryTags(addr, len, tag_manager->GetAllocationTagType());
if (!tag_data)
return tag_data.takeError();
- return tag_manager->UnpackTagsData(
- *tag_data, range.GetByteSize() / tag_manager->GetGranuleSize());
+ return tag_manager->UnpackTagsData(*tag_data,
+ len / tag_manager->GetGranuleSize());
}
diff --git a/lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp b/lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
index efd6514dc6c05..eec952943d79c 100644
--- a/lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
+++ b/lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
@@ -94,6 +94,121 @@ TEST(MemoryTagManagerAArch64MTETest, ExpandToGranule) {
manager.ExpandToGranule(MemoryTagManagerAArch64MTE::TagRange(18, 4)));
}
+static MemoryRegionInfo MakeRegionInfo(lldb::addr_t base, lldb::addr_t size,
+ bool tagged) {
+ return MemoryRegionInfo(
+ MemoryRegionInfo::RangeType(base, size), MemoryRegionInfo::eYes,
+ MemoryRegionInfo::eYes, MemoryRegionInfo::eYes, MemoryRegionInfo::eYes,
+ ConstString(), MemoryRegionInfo::eNo, 0,
+ /*memory_tagged=*/
+ tagged ? MemoryRegionInfo::eYes : MemoryRegionInfo::eNo);
+}
+
+TEST(MemoryTagManagerAArch64MTETest, MakeTaggedRange) {
+ MemoryTagManagerAArch64MTE manager;
+ MemoryRegionInfos memory_regions;
+
+ // No regions means no tagged regions, error
+ ASSERT_THAT_EXPECTED(
+ manager.MakeTaggedRange(0, 0x10, memory_regions),
+ llvm::FailedWithMessage(
+ "Address range 0x0:0x10 is not in a memory tagged region"));
+
+ // Alignment is done before checking regions.
+ // Here 1 is rounded up to the granule size of 0x10.
+ ASSERT_THAT_EXPECTED(
+ manager.MakeTaggedRange(0, 1, memory_regions),
+ llvm::FailedWithMessage(
+ "Address range 0x0:0x10 is not in a memory tagged region"));
+
+ // Range must not be inverted
+ ASSERT_THAT_EXPECTED(
+ manager.MakeTaggedRange(1, 0, memory_regions),
+ llvm::FailedWithMessage(
+ "End address (0x0) must be greater than the start address (0x1)"));
+
+ // Adding a single region to cover the whole range
+ memory_regions.push_back(MakeRegionInfo(0, 0x1000, true));
+
+ // Range can have
diff erent tags for begin and end
+ // (which would make it look inverted if we didn't remove them)
+ // Note that range comes back with an untagged base and alginment
+ // applied.
+ MemoryTagManagerAArch64MTE::TagRange expected_range(0x0, 0x10);
+ llvm::Expected<MemoryTagManagerAArch64MTE::TagRange> got =
+ manager.MakeTaggedRange(0x0f00000000000000, 0x0e00000000000001,
+ memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, expected_range);
+
+ // Error if the range isn't within any region
+ ASSERT_THAT_EXPECTED(
+ manager.MakeTaggedRange(0x1000, 0x1010, memory_regions),
+ llvm::FailedWithMessage(
+ "Address range 0x1000:0x1010 is not in a memory tagged region"));
+
+ // Error if the first part of a range isn't tagged
+ memory_regions.clear();
+ const char *err_msg =
+ "Address range 0x0:0x1000 is not in a memory tagged region";
+
+ // First because it has no region entry
+ memory_regions.push_back(MakeRegionInfo(0x10, 0x1000, true));
+ ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+ llvm::FailedWithMessage(err_msg));
+
+ // Then because the first region is untagged
+ memory_regions.push_back(MakeRegionInfo(0, 0x10, false));
+ ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+ llvm::FailedWithMessage(err_msg));
+
+ // If we tag that first part it succeeds
+ memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+ expected_range = MemoryTagManagerAArch64MTE::TagRange(0x0, 0x1000);
+ got = manager.MakeTaggedRange(0, 0x1000, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, expected_range);
+
+ // Error if the end of a range is untagged
+ memory_regions.clear();
+
+ // First because it has no region entry
+ memory_regions.push_back(MakeRegionInfo(0, 0xF00, true));
+ ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+ llvm::FailedWithMessage(err_msg));
+
+ // Then because the last region is untagged
+ memory_regions.push_back(MakeRegionInfo(0xF00, 0x100, false));
+ ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+ llvm::FailedWithMessage(err_msg));
+
+ // If we tag the last part it succeeds
+ memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+ got = manager.MakeTaggedRange(0, 0x1000, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, expected_range);
+
+ // Error if the middle of a range is untagged
+ memory_regions.clear();
+
+ // First because it has no entry
+ memory_regions.push_back(MakeRegionInfo(0, 0x500, true));
+ memory_regions.push_back(MakeRegionInfo(0x900, 0x700, true));
+ ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+ llvm::FailedWithMessage(err_msg));
+
+ // Then because it's untagged
+ memory_regions.push_back(MakeRegionInfo(0x500, 0x400, false));
+ ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+ llvm::FailedWithMessage(err_msg));
+
+ // If we tag the middle part it succeeds
+ memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+ got = manager.MakeTaggedRange(0, 0x1000, memory_regions);
+ ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+ ASSERT_EQ(*got, expected_range);
+}
+
TEST(MemoryTagManagerAArch64MTETest, RemoveNonAddressBits) {
MemoryTagManagerAArch64MTE manager;
More information about the lldb-commits
mailing list