Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb] Add SBProcess methods for get/set/use address masks #83095

Merged

Conversation

jasonmolenda
Copy link
Collaborator

I'm reviving a patch from phabracator, https://reviews.llvm.org/D155905 which was approved but I wasn't thrilled with all the API I was adding to SBProcess for all of the address mask types / memory regions. In this update, I added enums to control type address mask type (code, data, any) and address space specifiers (low, high, all) with defaulted arguments for the most common case.

This patch is also fixing a bug in the "addressable bits to address mask" calculation I added in AddressableBits::SetProcessMasks. If lldb were told that 64 bits are valid for addressing, this method would overflow the calculation and set an invalid mask. Added tests to check this specific bug while I was adding these APIs.

rdar://123530562

I'm reviving a patch from phabracator, https://reviews.llvm.org/D155905
which was approved but I wasn't thrilled with all the API I was
adding to SBProcess for all of the address mask types / memory
regions. In this update, I added enums to control type address mask
type (code, data, any) and address space specifiers (low, high,
all) with defaulted arguments for the most common case.

This patch is also fixing a bug in the "addressable bits to address
mask" calculation I added in AddressableBits::SetProcessMasks.
If lldb were told that 64 bits are valid for addressing, this method
would overflow the calculation and set an invalid mask.  Added tests
to check this specific bug while I was adding these APIs.

rdar://123530562
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 27, 2024

@llvm/pr-subscribers-lldb

Author: Jason Molenda (jasonmolenda)

Changes

I'm reviving a patch from phabracator, https://reviews.llvm.org/D155905 which was approved but I wasn't thrilled with all the API I was adding to SBProcess for all of the address mask types / memory regions. In this update, I added enums to control type address mask type (code, data, any) and address space specifiers (low, high, all) with defaulted arguments for the most common case.

This patch is also fixing a bug in the "addressable bits to address mask" calculation I added in AddressableBits::SetProcessMasks. If lldb were told that 64 bits are valid for addressing, this method would overflow the calculation and set an invalid mask. Added tests to check this specific bug while I was adding these APIs.

rdar://123530562


Full diff: https://github.com/llvm/llvm-project/pull/83095.diff

8 Files Affected:

  • (modified) lldb/include/lldb/API/SBProcess.h (+123)
  • (modified) lldb/include/lldb/Utility/AddressableBits.h (+3)
  • (modified) lldb/include/lldb/lldb-enumerations.h (+14)
  • (modified) lldb/source/API/SBProcess.cpp (+89)
  • (modified) lldb/source/Utility/AddressableBits.cpp (+10-2)
  • (added) lldb/test/API/python_api/process/address-masks/Makefile (+3)
  • (added) lldb/test/API/python_api/process/address-masks/TestAddressMasks.py (+64)
  • (added) lldb/test/API/python_api/process/address-masks/main.c (+5)
diff --git a/lldb/include/lldb/API/SBProcess.h b/lldb/include/lldb/API/SBProcess.h
index 4f92a41f3028a2..7e9ad7d9a274f2 100644
--- a/lldb/include/lldb/API/SBProcess.h
+++ b/lldb/include/lldb/API/SBProcess.h
@@ -407,6 +407,129 @@ class LLDB_API SBProcess {
   ///     the process isn't loaded from a core file.
   lldb::SBFileSpec GetCoreFile();
 
+  /// Get the current address mask that can be applied to addresses
+  /// before reading from memory.
+  ///
+  /// \param[in] type
+  ///     lldb may have different address masks for code and data
+  ///     addresses.  Either can be requested, or most commonly,
+  ///     eAddressMaskTypeAny can be requested and the least specific
+  ///     mask will be fetched.  e.g. on a target where instructions
+  ///     are word aligned, the Code mask might clear the low 2 bits.
+  ///
+  /// \param[in] addr_range
+  ///     Specify whether the address mask for high or low address spaces
+  ///     is requested.
+  ///     It is highly unusual to have different address masks in high
+  ///     or low memory, and by default the eAddressMaskRangeLow is the
+  ///     only one used for both types of addresses, the default value for
+  ///     this argument is the correct one.
+  ///
+  ///     On some architectures like AArch64, it is possible to have
+  ///     different page table setups for low and high memory, so different
+  ///     numbers of bits relevant to addressing, and it is possible to have
+  ///     a program running in one half of memory and accessing the other
+  ///     as heap, etc.  In that case the eAddressMaskRangeLow and
+  ///     eAddressMaskRangeHigh will have different masks that must be handled.
+  ///
+  /// \return
+  ///     The address mask currently in use.  Bits which are not used
+  ///     for addressing will be set to 1 in the mask.
+  lldb::addr_t GetAddressMask(
+      lldb::AddressMaskType type,
+      lldb::AddressMaskRange addr_range = lldb::eAddressMaskRangeLow);
+
+  /// Set the current address mask that can be applied to addresses
+  /// before reading from memory.
+  ///
+  /// \param[in] type
+  ///     lldb may have different address masks for code and data
+  ///     addresses.  Either can be set, or most commonly,
+  ///     eAddressMaskTypeAll can be set for both types of addresses.
+  ///     An example where they could be different is a target where
+  ///     instructions are word aligned, so the low 2 bits are always
+  ///     zero.
+  ///
+  /// \param[in] mask
+  ///     The address mask to set.  Bits which are not used for addressing
+  ///     should be set to 1 in the mask.
+  ///
+  /// \param[in] addr_range
+  ///     Specify whether the address mask for high or low address spaces
+  ///     is being set.
+  ///     It is highly unusual to have different address masks in high
+  ///     or low memory, and by default the eAddressMaskRangeLow is the
+  ///     only one used for both types of addresses, the default value for
+  ///     this argument is the correct one.
+  ///
+  ///     On some architectures like AArch64, it is possible to have
+  ///     different page table setups for low and high memory, so different
+  ///     numbers of bits relevant to addressing, and it is possible to have
+  ///     a program running in one half of memory and accessing the other
+  ///     as heap, etc.  In that case the eAddressMaskRangeLow and
+  ///     eAddressMaskRangeHigh will have different masks that must be
+  ///     specified.
+  void SetAddressMask(
+      lldb::AddressMaskType type, lldb::addr_t mask,
+      lldb::AddressMaskRange addr_range = lldb::eAddressMaskRangeLow);
+
+  /// Set the number of bits used for addressing in this Process.
+  ///
+  /// In some environments, the number of bits that are used for addressing
+  /// is the natural representation insted of a mask; this method calculates
+  /// the addressing mask that lldb uses internally from that number.
+  ///
+  /// \param[in] type
+  ///     lldb may have different address masks for code and data
+  ///     addresses.  Either can be set, or most commonly,
+  ///     eAddressMaskTypeAll can be set for both types of addresses.
+  ///     An example where they could be different is a target where
+  ///     instructions are word aligned, so the low 2 bits are always
+  ///     zero.
+  ///
+  /// \param[in] num_bits
+  ///     Number of bits that are used for addressing.  e.g. the low 42
+  ///     bits may be the only ones used for addressing, and high bits may
+  ///     store metadata and should be ignored by lldb.
+  ///
+  /// \param[in] addr_range
+  ///     Specify whether the address mask for high or low address spaces
+  ///     is being set.
+  ///     It is highly unusual to have different address masks in high
+  ///     or low memory, and by default the eAddressMaskRangeLow is the
+  ///     only one used for both types of addresses, the default value for
+  ///     this argument is the correct one.
+  ///
+  ///     On some architectures like AArch64, it is possible to have
+  ///     different page table setups for low and high memory, so different
+  ///     numbers of bits relevant to addressing, and it is possible to have
+  ///     a program running in one half of memory and accessing the other
+  ///     as heap, etc.  In that case the eAddressMaskRangeLow and
+  ///     eAddressMaskRangeHigh will have different masks that must be
+  ///     specified.
+  void
+  SetAddressableBits(AddressMaskType type, uint32_t num_bits,
+                     AddressMaskRange addr_range = lldb::eAddressMaskRangeLow);
+
+  /// Clear the non-addressable bits of an \a addr value and return a
+  /// virtual address in memory.
+  ///
+  /// Bits that are not used in addressing may be used for other purposes;
+  /// pointer authentication, or metadata in the top byte, or the 0th bit
+  /// of armv7 code addresses to indicate arm/thumb are common examples.
+  ///
+  /// \param[in] addr
+  ///     The address that should be cleared of non-addressable bits.
+  ///
+  /// \param[in] type
+  ///     If the address is known to be a code address (address of a function,
+  ///     for example), eAddressMaskTypeCode may be passed, which may have
+  ///     stricter address clearing than data addresses e.g. the low 2 bits
+  ///     being unused for code addresses on AArch64.
+  lldb::addr_t
+  FixAddress(lldb::addr_t addr,
+             lldb::AddressMaskType type = lldb::eAddressMaskTypeAny);
+
   /// Allocate memory within the process.
   ///
   /// This function will allocate memory in the process's address space.
diff --git a/lldb/include/lldb/Utility/AddressableBits.h b/lldb/include/lldb/Utility/AddressableBits.h
index 13c21329a8c617..75752fcf840a44 100644
--- a/lldb/include/lldb/Utility/AddressableBits.h
+++ b/lldb/include/lldb/Utility/AddressableBits.h
@@ -10,6 +10,7 @@
 #define LLDB_UTILITY_ADDRESSABLEBITS_H
 
 #include "lldb/lldb-forward.h"
+#include "lldb/lldb-public.h"
 
 namespace lldb_private {
 
@@ -33,6 +34,8 @@ class AddressableBits {
 
   void SetHighmemAddressableBits(uint32_t highmem_addressing_bits);
 
+  static lldb::addr_t AddressableBitToMask(uint32_t addressable_bits);
+
   void SetProcessMasks(lldb_private::Process &process);
 
 private:
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 85769071dae785..853370bf5eb515 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -1323,6 +1323,20 @@ enum SymbolDownload {
   eSymbolDownloadForeground = 2,
 };
 
+enum AddressMaskType {
+  eAddressMaskTypeCode = 0,
+  eAddressMaskTypeData,
+  eAddressMaskTypeAny,
+  eAddressMaskTypeAll = eAddressMaskTypeAny
+};
+
+enum AddressMaskRange {
+  eAddressMaskRangeLow = 0,
+  eAddressMaskRangeHigh,
+  eAddressMaskRangeAny,
+  eAddressMaskRangeAll = eAddressMaskRangeAny,
+};
+
 } // namespace lldb
 
 #endif // LLDB_LLDB_ENUMERATIONS_H
diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp
index a9fe915324683e..7edaa6b84fd7d7 100644
--- a/lldb/source/API/SBProcess.cpp
+++ b/lldb/source/API/SBProcess.cpp
@@ -1255,6 +1255,95 @@ lldb::SBFileSpec SBProcess::GetCoreFile() {
   return SBFileSpec(core_file);
 }
 
+addr_t SBProcess::GetAddressMask(AddressMaskType type,
+                                 AddressMaskRange addr_range) {
+  LLDB_INSTRUMENT_VA(this, type, addr_range);
+  addr_t default_mask = 0;
+  if (ProcessSP process_sp = GetSP()) {
+    switch (type) {
+    case eAddressMaskTypeCode:
+      if (addr_range == eAddressMaskRangeHigh)
+        return process_sp->GetHighmemCodeAddressMask();
+      else
+        return process_sp->GetCodeAddressMask();
+    case eAddressMaskTypeData:
+      if (addr_range == eAddressMaskRangeHigh)
+        return process_sp->GetHighmemDataAddressMask();
+      else
+        return process_sp->GetDataAddressMask();
+    case eAddressMaskTypeAny:
+      if (addr_range == eAddressMaskRangeHigh)
+        return process_sp->GetHighmemDataAddressMask();
+      else
+        return process_sp->GetDataAddressMask();
+    }
+  }
+  return default_mask;
+}
+
+void SBProcess::SetAddressMask(AddressMaskType type, addr_t mask,
+                               AddressMaskRange addr_range) {
+  LLDB_INSTRUMENT_VA(this, type, mask, addr_range);
+  if (ProcessSP process_sp = GetSP()) {
+    switch (type) {
+    case eAddressMaskTypeCode:
+      if (addr_range == eAddressMaskRangeAll) {
+        process_sp->SetCodeAddressMask(mask);
+        process_sp->SetHighmemCodeAddressMask(mask);
+      } else if (addr_range == eAddressMaskRangeHigh) {
+        process_sp->SetHighmemCodeAddressMask(mask);
+      } else {
+        process_sp->SetCodeAddressMask(mask);
+      }
+      break;
+    case eAddressMaskTypeData:
+      if (addr_range == eAddressMaskRangeAll) {
+        process_sp->SetDataAddressMask(mask);
+        process_sp->SetHighmemDataAddressMask(mask);
+      } else if (addr_range == eAddressMaskRangeHigh) {
+        process_sp->SetHighmemDataAddressMask(mask);
+      } else {
+        process_sp->SetDataAddressMask(mask);
+      }
+      break;
+    case eAddressMaskTypeAll:
+      if (addr_range == eAddressMaskRangeAll) {
+        process_sp->SetCodeAddressMask(mask);
+        process_sp->SetDataAddressMask(mask);
+        process_sp->SetHighmemCodeAddressMask(mask);
+        process_sp->SetHighmemDataAddressMask(mask);
+      } else if (addr_range == eAddressMaskRangeHigh) {
+        process_sp->SetHighmemCodeAddressMask(mask);
+        process_sp->SetHighmemDataAddressMask(mask);
+      } else {
+        process_sp->SetCodeAddressMask(mask);
+        process_sp->SetDataAddressMask(mask);
+      }
+      break;
+    }
+  }
+}
+
+void SBProcess::SetAddressableBits(AddressMaskType type, uint32_t num_bits,
+                                   AddressMaskRange addr_range) {
+  LLDB_INSTRUMENT_VA(this, type, num_bits, addr_range);
+  SetAddressMask(type, AddressableBits::AddressableBitToMask(num_bits),
+                 addr_range);
+}
+
+addr_t SBProcess::FixAddress(addr_t addr, AddressMaskType type) {
+  LLDB_INSTRUMENT_VA(this, addr, type);
+  if (ProcessSP process_sp = GetSP()) {
+    if (type == eAddressMaskTypeAny)
+      return process_sp->FixAnyAddress(addr);
+    else if (type == eAddressMaskTypeData)
+      return process_sp->FixDataAddress(addr);
+    else if (type == eAddressMaskTypeCode)
+      return process_sp->FixCodeAddress(addr);
+  }
+  return addr;
+}
+
 lldb::addr_t SBProcess::AllocateMemory(size_t size, uint32_t permissions,
                                        lldb::SBError &sb_error) {
   LLDB_INSTRUMENT_VA(this, size, permissions, sb_error);
diff --git a/lldb/source/Utility/AddressableBits.cpp b/lldb/source/Utility/AddressableBits.cpp
index c6e25f608da73d..7f9d7ec6c1349c 100644
--- a/lldb/source/Utility/AddressableBits.cpp
+++ b/lldb/source/Utility/AddressableBits.cpp
@@ -33,18 +33,26 @@ void AddressableBits::SetHighmemAddressableBits(
   m_high_memory_addr_bits = highmem_addressing_bits;
 }
 
+addr_t AddressableBits::AddressableBitToMask(uint32_t addressable_bits) {
+  assert(addressable_bits <= sizeof(addr_t) * 8);
+  if (addressable_bits == 64)
+    return 0; // all bits used for addressing
+  else
+    return ~((1ULL << addressable_bits) - 1);
+}
+
 void AddressableBits::SetProcessMasks(Process &process) {
   if (m_low_memory_addr_bits == 0 && m_high_memory_addr_bits == 0)
     return;
 
   if (m_low_memory_addr_bits != 0) {
-    addr_t low_addr_mask = ~((1ULL << m_low_memory_addr_bits) - 1);
+    addr_t low_addr_mask = AddressableBitToMask(m_low_memory_addr_bits);
     process.SetCodeAddressMask(low_addr_mask);
     process.SetDataAddressMask(low_addr_mask);
   }
 
   if (m_high_memory_addr_bits != 0) {
-    addr_t hi_addr_mask = ~((1ULL << m_high_memory_addr_bits) - 1);
+    addr_t hi_addr_mask = AddressableBitToMask(m_high_memory_addr_bits);
     process.SetHighmemCodeAddressMask(hi_addr_mask);
     process.SetHighmemDataAddressMask(hi_addr_mask);
   }
diff --git a/lldb/test/API/python_api/process/address-masks/Makefile b/lldb/test/API/python_api/process/address-masks/Makefile
new file mode 100644
index 00000000000000..10495940055b63
--- /dev/null
+++ b/lldb/test/API/python_api/process/address-masks/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/python_api/process/address-masks/TestAddressMasks.py b/lldb/test/API/python_api/process/address-masks/TestAddressMasks.py
new file mode 100644
index 00000000000000..4a6a737e94f9d9
--- /dev/null
+++ b/lldb/test/API/python_api/process/address-masks/TestAddressMasks.py
@@ -0,0 +1,64 @@
+"""Test Python APIs for setting, getting, and using address masks."""
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AddressMasksTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_address_masks(self):
+        self.build()
+        (target, process, t, bp) = lldbutil.run_to_source_breakpoint(
+            self, "break here", lldb.SBFileSpec("main.c")
+        )
+
+        process.SetAddressableBits(lldb.eAddressMaskTypeAll, 42)
+        self.assertEqual(0x0000029500003F94, process.FixAddress(0x00265E9500003F94))
+
+        # ~((1ULL<<42)-1) == 0xfffffc0000000000
+        process.SetAddressMask(lldb.eAddressMaskTypeAll, 0xFFFFFC0000000000)
+        self.assertEqual(0x0000029500003F94, process.FixAddress(0x00265E9500003F94))
+
+        # Check that all bits can pass through unmodified
+        process.SetAddressableBits(lldb.eAddressMaskTypeAll, 64)
+        self.assertEqual(0x00265E9500003F94, process.FixAddress(0x00265E9500003F94))
+
+        process.SetAddressableBits(
+            lldb.eAddressMaskTypeAll, 42, lldb.eAddressMaskRangeLow
+        )
+        process.SetAddressableBits(
+            lldb.eAddressMaskTypeAll, 15, lldb.eAddressMaskRangeHigh
+        )
+        self.assertEqual(0x000002950001F694, process.FixAddress(0x00265E950001F694))
+        self.assertEqual(0xFFFFFFFFFFFFF694, process.FixAddress(0xFFA65E950000F694))
+
+        process.SetAddressableBits(
+            lldb.eAddressMaskTypeAll, 42, lldb.eAddressMaskRangeAll
+        )
+        self.assertEqual(0x000002950001F694, process.FixAddress(0x00265E950001F694))
+        self.assertEqual(0xFFFFFE950000F694, process.FixAddress(0xFFA65E950000F694))
+
+        process.SetAddressMask(lldb.eAddressMaskTypeCode, 0xFFFFFC0000000003)
+        self.assertEqual(0x000002950001F697, process.FixAddress(0x00265E950001F697))
+        self.assertEqual(0xFFFFFE950000F697, process.FixAddress(0xFFA65E950000F697))
+        self.assertEqual(
+            0x000002950001F697,
+            process.FixAddress(0x00265E950001F697, lldb.eAddressMaskTypeData),
+        )
+        self.assertEqual(
+            0x000002950001F694,
+            process.FixAddress(0x00265E950001F697, lldb.eAddressMaskTypeCode),
+        )
+
+        # The user can override whatever settings the Process thinks should be used.
+        process.SetAddressableBits(
+            lldb.eAddressMaskTypeAll, 42, lldb.eAddressMaskRangeAll
+        )
+        self.runCmd("settings set target.process.virtual-addressable-bits 15")
+        self.runCmd("settings set target.process.highmem-virtual-addressable-bits 15")
+        self.assertEqual(0x0000000000007694, process.FixAddress(0x00265E950001F694))
+        self.assertEqual(0xFFFFFFFFFFFFF694, process.FixAddress(0xFFA65E950000F694))
diff --git a/lldb/test/API/python_api/process/address-masks/main.c b/lldb/test/API/python_api/process/address-masks/main.c
new file mode 100644
index 00000000000000..f21a10a16d5a75
--- /dev/null
+++ b/lldb/test/API/python_api/process/address-masks/main.c
@@ -0,0 +1,5 @@
+#include <stdio.h>
+
+int main(int argc, char const *argv[]) {
+  puts("Hello address masking world"); // break here
+}

Copy link
Contributor

@hawkinsw hawkinsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate the thorough documentation you wrote for these new functions. Because there is so much overlap in the documentation between the functions, could we refactor it somehow (not sure how?) so that any future change could be more easily tracked?

Just a question. Again, I appreciate your thorough documentation!
Will

lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
for the `type` and `addr_range` arguments that are used by all of
them, and refer back to those longer descriptions in each individual
message.

Fix the descriptions as suggested by David and Will.
@jasonmolenda
Copy link
Collaborator Author

Thanks so much for reading through these @DavidSpickett and @hawkinsw ! @adrian-prantl and @JDevlieghere suggested using a doxygen group for this set of methods and having the long definitions of type and addr_range a single time, referring back to them from the individual methods, I wasn't thrilled with all that duplicated text either. I think I did this well enough?

Copy link
Contributor

@hawkinsw hawkinsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said before, I really appreciate you doing such in-depth documentation. I hope these little suggestions help!

lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
@jasonmolenda
Copy link
Collaborator Author

Thanks for the second round of feedback @hawkinsw . Let me try to read the Doxygen docs a little more closely tonight and see if the references I threw in there might actually do what I hoped they would. I briefly looked at the Doxygen docs to see the Grouping feature and got a little overwhelmed and wasn't sure what the expected formatting would be.

lldb/source/API/SBProcess.cpp Outdated Show resolved Hide resolved
lldb/source/API/SBProcess.cpp Show resolved Hide resolved
@hawkinsw
Copy link
Contributor

Thanks for the second round of feedback @hawkinsw . Let me try to read the Doxygen docs a little more closely tonight and see if the references I threw in there might actually do what I hoped they would. I briefly looked at the Doxygen docs to see the Grouping feature and got a little overwhelmed and wasn't sure what the expected formatting would be.

Not a problem at all! Just trying to help!!

Will

lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/include/lldb/API/SBProcess.h Outdated Show resolved Hide resolved
lldb/source/API/SBProcess.cpp Outdated Show resolved Hide resolved
lldb/source/Utility/AddressableBits.cpp Show resolved Hide resolved
Add Will's good copyedit suggestions.

Co-authored-by: Will Hawkins <whh8b@obs.cr>
Copy link

github-actions bot commented Feb 29, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Have SBProcess::GetAddressMask return LLDB_INVALID_ADDRESS_MASK if
no mask can be retrieved.
David asked if I should assert on >64 addressable bits from
the user settings, and I saw that I failed to use the new
static method in AddressableBits when creating a mask from
the settings, which has the assert it in.

Also make the API test a little easier to read for the
eAddressMaskTypeCode.
I had this set to 0, which means "all bits are used for an address",
i.e. no masking will be perfomed, which is an acceptable state not
an invalid one.
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please fix the formatting issue before merging.

@jasonmolenda jasonmolenda merged commit 9a12b0a into llvm:main Mar 1, 2024
4 checks passed
@jasonmolenda jasonmolenda deleted the add-address-mask-sbprocess-apis branch March 1, 2024 01:02
jasonmolenda added a commit that referenced this pull request Mar 1, 2024
…83095)"

This reverts commit 9a12b0a.

TestAddressMasks fails its first test on lldb-x86_64-debian,
lldb-arm-ubuntu, lldb-aarch64-ubuntu bots.  Reverting while
investigating.
@jasonmolenda
Copy link
Collaborator Author

Temporarily reverted this change while I investigate why the tests failed on all the linux bots (lldb-x86_64-debian, lldb-arm-ubuntu, lldb-aarch64-ubuntu), I'll build up in a VM and debug.

@jasonmolenda
Copy link
Collaborator Author

It does occur to me that I'm going to need to only run this API test on targets which have a FixAddress method in their ABI, the base class own't do it. Maybe it should have a base class impl that can be overridden, and use the Process masks if they are set. (they're all initialized to 0 which is an impossible mask value (no address bits)) The base class impl would need to be overridden for different architectures, e.g. on AArch64 where TBI or MTE are used, in addition to clearing/setting the top byte, we need to use b55 to determine if the non-address bits are set to 1 or 0. On armv7 the 0th bit can be used for metadata on TypeCode fixes, etc.

But still I'd expect the lldb-aarch64-ubuntu bot to have had a FixAddress impl in its ABI. will check out when I build it up.

@jasonmolenda
Copy link
Collaborator Author

I think I'm going open a new PR with the base class address masking added to the patch. I think having these API and the unwritten caveat is "they may be no-ops if you're using an ABI that doesn't do FixAddress" is going to confuse people. I still want to investigate why my API test failed on aarch64 linux because I didn't expect that, it's possible there's something else going on. But the ABI thing is definitely something I think should be addressed.

jasonmolenda added a commit to jasonmolenda/llvm-project that referenced this pull request Mar 2, 2024
I'm reviving a patch from phabracator, https://reviews.llvm.org/D155905
which was approved but I wasn't thrilled with all the API I was adding
to SBProcess for all of the address mask types / memory regions. In this
update, I added enums to control type address mask type (code, data,
any) and address space specifiers (low, high, all) with defaulted
arguments for the most common case.

This patch is also fixing a bug in the "addressable bits to address
mask" calculation I added in AddressableBits::SetProcessMasks. If lldb
were told that 64 bits are valid for addressing, this method would
overflow the calculation and set an invalid mask. Added tests to check
this specific bug while I was adding these APIs.

rdar://123530562
@jasonmolenda
Copy link
Collaborator Author

After debugging, updating, and testing on aarch64-unbuntu, x86_64-macos, and arm64-macos, I have created a new PR with this commit plus an additional commit to fix the issues I found on the different platforms. #83663

jasonmolenda added a commit that referenced this pull request Mar 6, 2024
[lldb] Add SBProcess methods for get/set/use address masks (#83095)

I'm reviving a patch from phabracator, https://reviews.llvm.org/D155905
which was approved but I wasn't thrilled with all the API I was adding
to SBProcess for all of the address mask types / memory regions. In this
update, I added enums to control type address mask type (code, data,
any) and address space specifiers (low, high, all) with defaulted
arguments for the most common case.  I originally landed this via
#83095 but it failed on CIs
outside of arm64 Darwin so I had to debug it on more environments
and update the patch.

This patch is also fixing a bug in the "addressable bits to address
mask" calculation I added in AddressableBits::SetProcessMasks. If lldb
were told that 64 bits are valid for addressing, this method would
overflow the calculation and set an invalid mask. Added tests to check
this specific bug while I was adding these APIs.

This patch changes the value of "no mask set" from 0 to
LLDB_INVALID_ADDRESS_MASK, which is UINT64_MAX. A mask of all 1's
means "no bits are used for addressing" which is an impossible mask,
whereas a mask of 0 means "all bits are used for addressing" which
is possible.

I added a base class implementation of ABI::FixCodeAddress and
ABI::FixDataAddress that will apply the Process mask values if they
are set to a value other than LLDB_INVALID_ADDRESS_MASK.

I updated all the callers/users of the Mask methods which were
handling a value of 0 to mean invalid mask to use
LLDB_INVALID_ADDRESS_MASK.

I added code to the all AArch64 ABI Fix* methods to apply the
Highmem masks if they have been set.  These will not be set on a
Linux environment, but in TestAddressMasks.py I test the highmem
masks feature for any AArch64 target, so all AArch64 ABI  plugins 
must handle it.

rdar://123530562
jasonmolenda added a commit to jasonmolenda/llvm-project that referenced this pull request Mar 7, 2024
…3663)

[lldb] Add SBProcess methods for get/set/use address masks (llvm#83095)

I'm reviving a patch from phabracator, https://reviews.llvm.org/D155905
which was approved but I wasn't thrilled with all the API I was adding
to SBProcess for all of the address mask types / memory regions. In this
update, I added enums to control type address mask type (code, data,
any) and address space specifiers (low, high, all) with defaulted
arguments for the most common case.  I originally landed this via
llvm#83095 but it failed on CIs
outside of arm64 Darwin so I had to debug it on more environments
and update the patch.

This patch is also fixing a bug in the "addressable bits to address
mask" calculation I added in AddressableBits::SetProcessMasks. If lldb
were told that 64 bits are valid for addressing, this method would
overflow the calculation and set an invalid mask. Added tests to check
this specific bug while I was adding these APIs.

This patch changes the value of "no mask set" from 0 to
LLDB_INVALID_ADDRESS_MASK, which is UINT64_MAX. A mask of all 1's
means "no bits are used for addressing" which is an impossible mask,
whereas a mask of 0 means "all bits are used for addressing" which
is possible.

I added a base class implementation of ABI::FixCodeAddress and
ABI::FixDataAddress that will apply the Process mask values if they
are set to a value other than LLDB_INVALID_ADDRESS_MASK.

I updated all the callers/users of the Mask methods which were
handling a value of 0 to mean invalid mask to use
LLDB_INVALID_ADDRESS_MASK.

I added code to the all AArch64 ABI Fix* methods to apply the
Highmem masks if they have been set.  These will not be set on a
Linux environment, but in TestAddressMasks.py I test the highmem
masks feature for any AArch64 target, so all AArch64 ABI  plugins
must handle it.

rdar://123530562
(cherry picked from commit aeaa11a)
jasonmolenda added a commit to jasonmolenda/llvm-project that referenced this pull request Mar 9, 2024
…3663)

[lldb] Add SBProcess methods for get/set/use address masks (llvm#83095)

I'm reviving a patch from phabracator, https://reviews.llvm.org/D155905
which was approved but I wasn't thrilled with all the API I was adding
to SBProcess for all of the address mask types / memory regions. In this
update, I added enums to control type address mask type (code, data,
any) and address space specifiers (low, high, all) with defaulted
arguments for the most common case.  I originally landed this via
llvm#83095 but it failed on CIs
outside of arm64 Darwin so I had to debug it on more environments
and update the patch.

This patch is also fixing a bug in the "addressable bits to address
mask" calculation I added in AddressableBits::SetProcessMasks. If lldb
were told that 64 bits are valid for addressing, this method would
overflow the calculation and set an invalid mask. Added tests to check
this specific bug while I was adding these APIs.

This patch changes the value of "no mask set" from 0 to
LLDB_INVALID_ADDRESS_MASK, which is UINT64_MAX. A mask of all 1's
means "no bits are used for addressing" which is an impossible mask,
whereas a mask of 0 means "all bits are used for addressing" which
is possible.

I added a base class implementation of ABI::FixCodeAddress and
ABI::FixDataAddress that will apply the Process mask values if they
are set to a value other than LLDB_INVALID_ADDRESS_MASK.

I updated all the callers/users of the Mask methods which were
handling a value of 0 to mean invalid mask to use
LLDB_INVALID_ADDRESS_MASK.

I added code to the all AArch64 ABI Fix* methods to apply the
Highmem masks if they have been set.  These will not be set on a
Linux environment, but in TestAddressMasks.py I test the highmem
masks feature for any AArch64 target, so all AArch64 ABI  plugins
must handle it.

rdar://123530562
(cherry picked from commit aeaa11a)
(cherry picked from commit 674487f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants