[lldb][AArch64] Refactor memory tag range handling

[DavidSpickett]

Summary

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.

[DavidSpickett]

This is a response to feedback on https://reviews.llvm.org/D105181 but would be intended to come before any of the changes in that series if it looks good.

I'm yet to refactor the tag writing changes on to this, wanted to get some feedback first. Roughly tag writing would instead of adding GetMemoryTagManagerForGranules to process, it would add MakeTaggedRangeForGranules to the tag manager. Then the memory tag write command calls that instead of relying on GetMemoryTagManager to implement that logic.

Then Process simply has GetMemoryTagManager, ReadMemoryTags and WriteMemoryTags. Instead of growing all these variants of GetMemoryTagManager.

Side note: I actually found a bug when writing the unit tests, so I think that's a big plus to this approach.

[DavidSpickett]

Similarly, I have a patch locally that has a "best effort" read where it returns a sparse set of results (for annotating memory reads). For that there would be a MakeTaggedRange<...> that returns a list of ranges that you can then read in a loop.

[omjavaid]

I think this approach is a lot better given that the process of tagged ranged calculation deserved a separation and may be more explanation too. I guess somewhere in the comments of same function you can explain interaction between memory regions and tagged ranges. It will become a bit more complicated for a reader who have no knowledge about memory tag range and disjoint region.

lldb/include/lldb/Target/MemoryTagManager.h

66

I couldnt understand this point in comment "have an untagged base address"

lldb/source/Commands/CommandObjectMemoryTag.cpp

70

If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we can directly return a tag manager pointer here.

Also SupportsMemoryTagging may also run once for the life time of a process? We can store this information is a flag or something.

71

should we process pointer validity check before this call?

[DavidSpickett]

Update various comments.

[DavidSpickett]

It will become a bit more complicated for a reader who have no knowledge about memory tag range and disjoint region.

I've added/updated some of the comments in MakeTaggedRange. Maybe you can highlight any bits that aren't clear and I'll make sure they get comments too.

lldb/include/lldb/Target/MemoryTagManager.h

66

This is noted for 2 reasons:

• Callers don't need to remove tags before displaying the range (memory tag read).
• There's potentially 2 different tags, begin/end, so removing them is the easiest way to make (or rather not make) that choice.

You could keep the base tagged the same as addr but the code works out simpler if you don't and all it means is that memory tag read has to remove the tags itself. If it turns out that most callers want it tagged then sure I'll change the behaviour.

lldb/source/Commands/CommandObjectMemoryTag.cpp

70

If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we can directly return a tag manager pointer here.

Sure, that way GetMemoryTagManager does exactly what it says on the tin. I can just make a utility function in CommandObjectMemoryTag.cpp to reduce the duplication of checking both, when adding the write command.

Will do that in the next update.

Also SupportsMemoryTagging may also run once for the life time of a process? We can store this information is a flag or something.

It already is in that for the GDB remote it'll only send a qSupported the first time it (or any other property) is asked for. So we go through a few function calls but that's it.

71

Covered by eCommandRequiresProcess in the constructor.

[DavidSpickett]

lldb/source/Commands/CommandObjectMemoryTag.cpp

70

On second thought I'd rather leave GetMemoryTagManager as it is.

If we split the checks up we'll have the same 2 checks repeated in:

• memory tag read, memory tag write (same file so you could make a helper function)
• Process::ReadMemoryTags, Process::WriteMemoryTags (again you could make a helper but...)

That helper would be what GetMemoryTagManager is now. Leaving it as is saves duplicating it.

I thought of simply only checking SupportsMemoryTagging in Process::ReadMemoryTags. Problem with that is that you would never get this error if your remote wasn't able to show you MTE memory region flags. And you'd hope the most obvious checks would come first.

The other reason to split the 2 checks is so GetMemoryTagManager could be called from the API to get a manager even if the remote didn't support MTE. Which seems like a very niche use case, perhaps you've got some very old remote but are somehow running an MTE enabled program on it? Seems unlikely.

If we find a use case later when I work on the API then it'll be easy to switch it around anyway.

[DavidSpickett]

Correct // to /// in comment block for ReadMemoryTags.

[omjavaid]

Is this final version or you are still doing refactoring. Also can you kindly order its child and parent revs in current tag-write patch series.

lldb/source/Commands/CommandObjectMemoryTag.cpp

70

Agreed no strong reason to further split GetMemoryTagManager. API use-case may never be utilized and we can always come back and refactor if need arises.

[DavidSpickett]

Is this final version or you are still doing refactoring. Also can you kindly order its child and parent revs in current tag-write patch series.

I'm currently rebasing all the active patches onto this one, I don't expect to change this but I might have to I'll see. Then I'll update it all into one series with the correct relations and you can give a final approval for this.

[DavidSpickett]

All the follow ups are now based on this change. I didn't need to make an updates to this particular patch after all.

[omjavaid]

In D105630#2876615, @DavidSpickett wrote:

All the follow ups are now based on this change. I didn't need to make an updates to this particular patch after all.

This looks good but the test fails on arm 32 bit.

lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp

114

less than?

[DavidSpickett]

This looks good but the test fails on arm 32 bit.

I had a few of those last time, I will build the whole series on 32 bit to check for more.

[DavidSpickett]

Rebase and correct printf formatting to fix 32 bit test issues.

[DavidSpickett]

Update comment about save/restoring end of range.

lldb/include/lldb/Target/MemoryTagManager.h

 //===-- MemoryTagManager.h --------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 
 namespace lldb_private {
 
 // This interface allows high level commands to handle memory tags
 // in a generic way.
public:
   //
   // Say your granules are 16 bytes and you want
   // tags for 16 bytes of memory starting from address 8.
   // 1 granule isn't enough because it only covers addresses
   // 0-16, we want addresses 8-24. So the range must be
   // 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.
   //
   // This value is unique within a given architecture. Meaning that different
   // tagging schemes within the same architecture should use unique values,
   // but other architectures can overlap those values.
   virtual int32_t GetAllocationTagType() const = 0;

lldb/include/lldb/Target/Process.h

/// \see Process::Notifications
   ///
   /// \return
   ///     The address of the allocated buffer in the process, or
   ///     LLDB_INVALID_ADDRESS if the allocation failed.
 
   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.
   ///
   /// \param[in] len
   ///     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,
+  ///     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.
   ///
   /// \param[in] address
   ///     The load address of the indirect function to resolve.
   ///
   /// \param[out] error
   ///     An error value in case the resolve fails.

lldb/source/Commands/CommandObjectMemoryTag.cpp

if (command.GetArgumentCount() > 1) {
       if (end_addr == LLDB_INVALID_ADDRESS) {
         result.AppendErrorWithFormatv("Invalid end address expression, {0}",
                                       error.AsCString());
         return false;
       }
     }
 
     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()));
       return false;
     }
 
     const MemoryTagManager *tag_manager = *tag_manager_or_err;
-    ptrdiff_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()));
       return false;
     }
 
     result.AppendMessageWithFormatv("Logical tag: {0:x}",
                                     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
       result.AppendMessageWithFormatv("[{0:x}, {1:x}): {2:x}", addr, next_addr,
                                       tag);
       addr = next_addr;
     }
 

lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h

public:
   size_t GetTagSizeInBytes() const override;
 
   lldb::addr_t GetLogicalTag(lldb::addr_t addr) const override;
   lldb::addr_t RemoveNonAddressBits(lldb::addr_t addr) const override;
   ptrdiff_t AddressDiff(lldb::addr_t addr1, lldb::addr_t addr2) const override;
 
   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;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_UTILITY_MEMORYTAGMANAGERAARCH64MTE_H

lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp

MemoryTagManagerAArch64MTE::ExpandToGranule(TagRange range) const {
   // Then align up to the end of the granule
   size_t align_up_amount = granule - (new_len % granule);
   if (align_up_amount != granule)
     new_len += align_up_amount;
 
   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.
+  ptrdiff_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 &region) {
+          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 {
   size_t num_tags = tags.size() / GetTagSizeInBytes();
   if (num_tags != granules) {
     return llvm::createStringError(
         llvm::inconvertibleErrorCode(),
         "Packed tag data size does not match expected number of tags. "

lldb/source/Target/Process.cpp

if (frame) {
         return true;
       }
     }
   }
 
   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;
   if (!arch || !tag_manager) {
     return llvm::createStringError(
         llvm::inconvertibleErrorCode(),
         "This architecture does not support memory tagging",
         GetPluginName().GetCString());
   }
 
   if (!SupportsMemoryTagging()) {
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "Process does not support memory tagging");
   }
 
-  ptrdiff_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());
 }

lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp

ASSERT_EQ(
       manager.ExpandToGranule(MemoryTagManagerAArch64MTE::TagRange(16, 24)));
 
   // Start and size need aligning here but we only need 1 granule to cover it
   ASSERT_EQ(
       MemoryTagManagerAArch64MTE::TagRange(16, 16),
       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 different 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;
 
   ASSERT_EQ(0, 0);
   ASSERT_EQ((lldb::addr_t)0x00ffeedd11223344,
             manager.RemoveNonAddressBits(0x00ffeedd11223344));
   ASSERT_EQ((lldb::addr_t)0x0000000000000000,
             manager.RemoveNonAddressBits(0xFF00000000000000));