[lldb] [gdb-remote] Also handle high/low memory addressable bits setting in the stop info packet

[jasonmolenda]

Summary

A small follow on patch to https://reviews.llvm.org/D157667 to handle the new low_mem_addressing_bits and high_mem_addressing_bits keys in the stop reply packet too, not just qHostInfo.

I changed AddressableBits so that the low memory and high memory bits can be set separately, and we can query whether any addressing bits were seen while parsing the packet, and update the Process address masks if they were.

[JDevlieghere]

lldb/source/Utility/AddressableBits.cpp

81–83

Please implement operator bool() instead.

85–87

Where is this called?

[jasonmolenda]

lldb/source/Utility/AddressableBits.cpp

81–83

ok.

85–87

Hm. When I thought of this patch last night, I was sure I needed a Clear method for some reason, but you're right I never ended up using it.

[jasonmolenda]

lldb/source/Utility/AddressableBits.cpp

85–87

AHA I did use it, but it's in the previous patch. It's in the base class method of ObjectFile.

virtual bool GetAddressableBits(lldb_private::AddressableBits &address_bits) {
  address_bits.Clear();
  return false;
}

[jasonmolenda]

Update patch to change the AddressableBits::IsValid to AddressableBits::HasValue which is a clearer description. The IsValid methods are used across the lldb_private classes everywhere. I tried changing this to operator bool but I thought that was less clear in use. I compromised by changing it to HasValue - this is a POD class so "IsValid" seemed a little questionable, but I didn't like operator bool. Let's try this out.

[JDevlieghere]

lldb/source/Utility/AddressableBits.cpp

85–87

Why is this returning a bool and not AddressableBits?

[jasonmolenda]

lldb/source/Utility/AddressableBits.cpp

85–87

The addressable_bits argument is a [out] variable to indicate if it has a value. You're right, now that there's a HasValue method, ObjectFile::GetAddressableBits can return an AddressableBits and the caller can call HasValue.

[JDevlieghere]

In D158041#4593205, @jasonmolenda wrote:

Update patch to change the AddressableBits::IsValid to AddressableBits::HasValue which is a clearer description. The IsValid methods are used across the lldb_private classes everywhere. I tried changing this to operator bool but I thought that was less clear in use. I compromised by changing it to HasValue - this is a POD class so "IsValid" seemed a little questionable, but I didn't like operator bool. Let's try this out.

Yeah that's fine. I don't feel strongly about operator bool(). My main concern is to not create another class that can be in an "invalid state" and requires you to check whether or not it's valid before doing an operation. As you point out, that's not really the case here anyway, and using different terminology helps convey that.

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

2314–2316

I would inline the check for HasValue in SetProcessMasks. You already do some kind of sanity checking there, might as well make that sound and do it unconditionally.

[jasonmolenda]

Update patch to remove the HasValue/IsValue and Clear methods from AddressableBits. Change methods that may fill in an AddressableBits object with values to return an AddressableBits object unconditionally. Change AddressableBits::SetProcessMasks so it can be called unconditionally, and it only sets mask values in Process if values were set.

I wanted to allow for someone to create an AddressableBits object with only the high memory addressing bits specified (for instance) and the low memory address mask will not be changed. I updated AddressableBits::SetProcessMasks to use the current Process mask values if it doesn't have a value specified. This will be more important when I add the SB API to SBProcess to set address masks in https://reviews.llvm.org/D155905 I expect.

[JDevlieghere]

Thanks, this LGTM.

lldb/docs/lldb-gdb-remote.txt

 //                                       qHostInfo packet response, and if the
 //                                       value cannot change during the process
 //                                       lifetime, it does not need to be 
 //                                       duplicated here in the stop packet.
 //                                       For a firmware environment with early
 //                                       start code that may be changing the
 //                                       page table setup, a dynamically set
 //                                       value may be needed.
+//  "low_mem_addressing_bits" unsigned optional, specifies how many bits in 
+//                                       addresses in low memory are significant 
+//                                       for addressing, base 10.  AArch64 can 
+//                                       have different page table setups for low 
+//                                       and high memory, and therefore a different 
+//                                       number of bits used for addressing.
+//  "high_mem_addressing_bits" unsigned optional, specifies how many bits in 
+//                                       addresses in high memory are significant 
+//                                       for addressing, base 10.  AArch64 can have 
+//                                       different page table setups for low and 
+//                                       high memory, and therefore a different 
+//                                       number of bits used for addressing.
 //
 // BEST PRACTICES:
 //  Since register values can be supplied with this packet, it is often useful
 //  to return the PC, SP, FP, LR (if any), and FLAGS registers so that separate
 //  packets don't need to be sent to read each of these registers from each
 //  thread.
 //
 //  If a thread is stopped for no reason (like just because another thread

lldb/include/lldb/Symbol/ObjectFile.h

virtual std::string GetIdentifierString () {
       return std::string();
   }
 
   /// Some object files may have the number of bits used for addressing
   /// embedded in them, e.g. a Mach-O core file using an LC_NOTE.  These
   /// object files can return an AddressableBits object that can can be
   /// used to set the address masks in the Process.
   ///
-  /// \param[out] address_bits
-  ///     Can be used to set the Process address masks.
-  ///
   /// \return
-  ///     Returns true if addressable bits metadata was found.
-  virtual bool GetAddressableBits(lldb_private::AddressableBits &address_bits) {
-    address_bits.Clear();
-    return false;
-  }
+  ///     Returns an AddressableBits object which can be used to set
+  ///     the address masks in the Process.
+  virtual lldb_private::AddressableBits GetAddressableBits() { return {}; }
 
   /// When the ObjectFile is a core file, lldb needs to locate the "binary" in
   /// the core file.  lldb can iterate over the pages looking for a valid
   /// binary, but some core files may have metadata  describing where the main
   /// binary is exactly which removes ambiguity when there are multiple
   /// binaries present in the captured memory pages.
   ///
   /// \param[out] value

lldb/include/lldb/Utility/AddressableBits.h

public:
   /// When a single value is available for the number of bits.
   void SetAddressableBits(uint32_t addressing_bits);
 
   /// When we have separate values for low memory addresses and high memory
   /// addresses.
   void SetAddressableBits(uint32_t lowmem_addressing_bits,
                           uint32_t highmem_addressing_bits);
 
-  void SetProcessMasks(lldb_private::Process &process);
+  void SetLowmemAddressableBits(uint32_t lowmem_addressing_bits);
+
+  void SetHighmemAddressableBits(uint32_t highmem_addressing_bits);
 
-  void Clear();
+  void SetProcessMasks(lldb_private::Process &process);
 
 private:
   uint32_t m_low_memory_addr_bits;
   uint32_t m_high_memory_addr_bits;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_ADDRESSABLEBITS_H

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

public:
   lldb_private::Address GetEntryPointAddress() override;
 
   lldb_private::Address GetBaseAddress() override;
 
   uint32_t GetNumThreadContexts() override;
 
   std::string GetIdentifierString() override;
 
-  bool GetAddressableBits(lldb_private::AddressableBits &address_bits) override;
+  lldb_private::AddressableBits GetAddressableBits() override;
 
   bool GetCorefileMainBinaryInfo(lldb::addr_t &value, bool &value_is_offset,
                                  lldb_private::UUID &uuid,
                                  ObjectFile::BinaryType &type) override;
 
   bool LoadCoreFileImages(lldb_private::Process &process) override;
 
   lldb::RegisterContextSP

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

for (uint32_t i = 0; i < m_header.ncmds; ++i) {
           free(buf);
       }
       offset = cmd_offset + ident_command.cmdsize;
     }
   }
   return result;
 }
 
-bool ObjectFileMachO::GetAddressableBits(AddressableBits &address_bits) {
-  address_bits.Clear();
+AddressableBits ObjectFileMachO::GetAddressableBits() {
+  AddressableBits addressable_bits;
 
   Log *log(GetLog(LLDBLog::Process));
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
     lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
     for (uint32_t i = 0; i < m_header.ncmds; ++i) {
       const uint32_t cmd_offset = offset;
for (uint32_t i = 0; i < m_header.ncmds; ++i) {
         // number of bits used in addressing.
         if (strcmp("addrable bits", data_owner) == 0) {
           offset = fileoff;
           uint32_t version;
           if (m_data.GetU32(&offset, &version, 1) != nullptr) {
             if (version == 3) {
               uint32_t num_addr_bits = m_data.GetU32_unchecked(&offset);
               if (num_addr_bits != 0) {
-                address_bits.SetAddressableBits(num_addr_bits);
+                addressable_bits.SetAddressableBits(num_addr_bits);
               }
               LLDB_LOGF(log,
                         "LC_NOTE 'addrable bits' v3 found, value %d "
                         "bits",
                         num_addr_bits);
-              return true;
+              break;
             }
             if (version == 4) {
               uint32_t lo_addr_bits = m_data.GetU32_unchecked(&offset);
               uint32_t hi_addr_bits = m_data.GetU32_unchecked(&offset);
 
-              address_bits.SetAddressableBits(lo_addr_bits, hi_addr_bits);
+              addressable_bits.SetAddressableBits(lo_addr_bits, hi_addr_bits);
               LLDB_LOGF(log,
                         "LC_NOTE 'addrable bits' v4 found, value %d & %d bits",
                         lo_addr_bits, hi_addr_bits);
 
-              return true;
+              break;
             }
           }
         }
       }
       offset = cmd_offset + lc.cmdsize;
     }
   }
-  return false;
+  return addressable_bits;
 }
 
 bool ObjectFileMachO::GetCorefileMainBinaryInfo(addr_t &value,
                                                 bool &value_is_offset,
                                                 UUID &uuid,
                                                 ObjectFile::BinaryType &type) {
   Log *log(
       GetLog(LLDBLog::Symbols | LLDBLog::Process | LLDBLog::DynamicLoader));

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

public:
   llvm::VersionTuple GetMacCatalystVersion();
 
   std::optional<std::string> GetOSBuildString();
 
   std::optional<std::string> GetOSKernelDescription();
 
   ArchSpec GetSystemArchitecture();
 
-  bool GetAddressableBits(lldb_private::AddressableBits &addressable_bits);
+  lldb_private::AddressableBits GetAddressableBits();
 
   bool GetHostname(std::string &s);
 
   lldb::addr_t GetShlibInfoAddr();
 
   bool GetProcessInfo(lldb::pid_t pid, ProcessInstanceInfo &process_info);
 
   uint32_t FindProcesses(const ProcessInstanceInfoMatch &process_match_info,

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

 
 const lldb_private::ArchSpec &
 GDBRemoteCommunicationClient::GetHostArchitecture() {
   if (m_qHostInfo_is_valid == eLazyBoolCalculate)
     GetHostInfo();
   return m_host_arch;
 }
 
-bool GDBRemoteCommunicationClient::GetAddressableBits(
-    lldb_private::AddressableBits &addressable_bits) {
-  addressable_bits.Clear();
+AddressableBits GDBRemoteCommunicationClient::GetAddressableBits() {
+  AddressableBits addressable_bits;
   if (m_qHostInfo_is_valid == eLazyBoolCalculate)
     GetHostInfo();
-  if (m_low_mem_addressing_bits != 0 || m_high_mem_addressing_bits != 0) {
-    addressable_bits.SetAddressableBits(m_low_mem_addressing_bits,
-                                        m_high_mem_addressing_bits);
-    return true;
-  }
-  return false;
+
+  // m_low_mem_addressing_bits and m_high_mem_addressing_bits
+  // will be 0 if we did not receive values; AddressableBits
+  // treats 0 as "unspecified".
+  return addressable_bits;
 }
 
 seconds GDBRemoteCommunicationClient::GetHostDefaultPacketTimeout() {
   if (m_qHostInfo_is_valid == eLazyBoolCalculate)
     GetHostInfo();
   return m_default_packet_timeout;
 }
 

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (remote_process_arch.IsValid()) {
     process_arch = m_gdb_comm.GetHostArchitecture();
     LLDB_LOG(log,
              "gdb-remote did not have process architecture, using gdb-remote "
              "host architecture {0} {1}",
              process_arch.GetArchitectureName(),
              process_arch.GetTriple().getTriple());
   }
 
-  AddressableBits addressable_bits;
-  if (m_gdb_comm.GetAddressableBits(addressable_bits)) {
-    addressable_bits.SetProcessMasks(*this);
-  }
+  AddressableBits addressable_bits = m_gdb_comm.GetAddressableBits();
 
   if (process_arch.IsValid()) {
     const ArchSpec &target_arch = GetTarget().GetArchitecture();
     if (target_arch.IsValid()) {
       LLDB_LOG(log, "analyzing target arch, currently {0} {1}",
                target_arch.GetArchitectureName(),
                target_arch.GetTriple().getTriple());
 
case 'S': {
     bool queue_vars_valid =
         false; // says if locals below that start with "queue_" are valid
     addr_t dispatch_queue_t = LLDB_INVALID_ADDRESS;
     LazyBool associated_with_dispatch_queue = eLazyBoolCalculate;
     std::string queue_name;
     QueueKind queue_kind = eQueueKindUnknown;
     uint64_t queue_serial_number = 0;
     ExpeditedRegisterMap expedited_register_map;
+    AddressableBits addressable_bits;
     while (stop_packet.GetNameColonValue(key, value)) {
       if (key.compare("metype") == 0) {
         // exception type in big endian hex
         value.getAsInteger(16, exc_type);
       } else if (key.compare("medata") == 0) {
         // exception data in big endian hex
         uint64_t x;
         value.getAsInteger(16, x);
while (stop_packet.GetNameColonValue(key, value)) {
 
         reason = key.str();
         StreamString ostr;
         ostr.Printf("%" PRIu64 " %" PRIu64, pid_tid->first, pid_tid->second);
         description = std::string(ostr.GetString());
       } else if (key.compare("addressing_bits") == 0) {
         uint64_t addressing_bits;
         if (!value.getAsInteger(0, addressing_bits)) {
-          addr_t address_mask = ~((1ULL << addressing_bits) - 1);
-          SetCodeAddressMask(address_mask);
-          SetDataAddressMask(address_mask);
+          addressable_bits.SetAddressableBits(addressing_bits);
+        }
+      } else if (key.compare("low_mem_addressing_bits") == 0) {
+        uint64_t addressing_bits;
+        if (!value.getAsInteger(0, addressing_bits)) {
+          addressable_bits.SetLowmemAddressableBits(addressing_bits);
+        }
+      } else if (key.compare("high_mem_addressing_bits") == 0) {
+        uint64_t addressing_bits;
+        if (!value.getAsInteger(0, addressing_bits)) {
+          addressable_bits.SetHighmemAddressableBits(addressing_bits);
         }
       } else if (key.size() == 2 && ::isxdigit(key[0]) && ::isxdigit(key[1])) {
         uint32_t reg = UINT32_MAX;
         if (!key.getAsInteger(16, reg))
           expedited_register_map[reg] = std::string(std::move(value));
       }
     }
 
if (tid == LLDB_INVALID_THREAD_ID) {
       // one.
       UpdateThreadIDList();
 
       if (!m_thread_ids.empty()) {
         tid = m_thread_ids.front();
       }
     }
 
+    addressable_bits.SetProcessMasks(*this);
+
     ThreadSP thread_sp = SetThreadStopInfo(
         tid, expedited_register_map, signo, thread_name, reason, description,
         exc_type, exc_data, thread_dispatch_qaddr, queue_vars_valid,
         associated_with_dispatch_queue, dispatch_queue_t, queue_name,
         queue_kind, queue_serial_number);
 
     return eStateStopped;
   } break;
 
   case 'W':
   case 'X':

lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

if (arch.IsValid())
     GetTarget().SetArchitecture(arch);
 
   CreateMemoryRegions();
 
   LoadBinariesAndSetDYLD();
 
   CleanupMemoryRegionPermissions();
 
-  AddressableBits addressable_bits;
-  if (core_objfile->GetAddressableBits(addressable_bits)) {
-    addressable_bits.SetProcessMasks(*this);
-  }
+  AddressableBits addressable_bits = core_objfile->GetAddressableBits();
+
   return error;
 }
 
 lldb_private::DynamicLoader *ProcessMachCore::GetDynamicLoader() {
   if (m_dyld_up.get() == nullptr)
     m_dyld_up.reset(DynamicLoader::FindPlugin(this, m_dyld_plugin_name));
   return m_dyld_up.get();
 }

lldb/source/Utility/AddressableBits.cpp

 }
 
 void AddressableBits::SetAddressableBits(uint32_t lowmem_addressing_bits,
                                          uint32_t highmem_addressing_bits) {
   m_low_memory_addr_bits = lowmem_addressing_bits;
   m_high_memory_addr_bits = highmem_addressing_bits;
 }
 
-void AddressableBits::Clear() {
-  m_low_memory_addr_bits = m_high_memory_addr_bits = 0;
+void AddressableBits::SetLowmemAddressableBits(
+    uint32_t lowmem_addressing_bits) {
+  m_low_memory_addr_bits = lowmem_addressing_bits;
 }
 
-void AddressableBits::SetProcessMasks(Process &process) {
-  // In case either value is set to 0, indicating it was not set, use the
-  // other value.
-  if (m_low_memory_addr_bits == 0)
-    m_low_memory_addr_bits = m_high_memory_addr_bits;
-  if (m_high_memory_addr_bits == 0)
-    m_high_memory_addr_bits = m_low_memory_addr_bits;
+void AddressableBits::SetHighmemAddressableBits(
+    uint32_t highmem_addressing_bits) {
+  m_high_memory_addr_bits = highmem_addressing_bits;
+}
 
-  if (m_low_memory_addr_bits == 0)
+void AddressableBits::SetProcessMasks(Process &process) {
+  if (m_low_memory_addr_bits == 0 && m_high_memory_addr_bits == 0)
     return;
 
-  addr_t address_mask = ~((1ULL << m_low_memory_addr_bits) - 1);
-  process.SetCodeAddressMask(address_mask);
-  process.SetDataAddressMask(address_mask);
-
-  if (m_low_memory_addr_bits != m_high_memory_addr_bits) {
-    lldb::addr_t hi_address_mask = ~((1ULL << m_high_memory_addr_bits) - 1);
-    process.SetHighmemCodeAddressMask(hi_address_mask);
-    process.SetHighmemDataAddressMask(hi_address_mask);
+  // If we don't have an addressable bits value for low memory,
+  // see if we have a Code/Data mask already, and use that.
+  // Or use the high memory addressable bits value as a last
+  // resort.
+  addr_t low_addr_mask;
+  if (m_low_memory_addr_bits == 0) {
+    if (process.GetCodeAddressMask() != UINT64_MAX)
+      low_addr_mask = process.GetCodeAddressMask();
+    else if (process.GetDataAddressMask() != UINT64_MAX)
+      low_addr_mask = process.GetDataAddressMask();
+    else
+      low_addr_mask = ~((1ULL << m_high_memory_addr_bits) - 1);
+  } else {
+    low_addr_mask = ~((1ULL << m_low_memory_addr_bits) - 1);
+  }
+
+  // If we don't have an addressable bits value for high memory,
+  // see if we have a Code/Data mask already, and use that.
+  // Or use the low memory addressable bits value as a last
+  // resort.
+  addr_t hi_addr_mask;
+  if (m_high_memory_addr_bits == 0) {
+    if (process.GetHighmemCodeAddressMask() != UINT64_MAX)
+      hi_addr_mask = process.GetHighmemCodeAddressMask();
+    else if (process.GetHighmemDataAddressMask() != UINT64_MAX)
+      hi_addr_mask = process.GetHighmemDataAddressMask();
+    else
+      hi_addr_mask = ~((1ULL << m_low_memory_addr_bits) - 1);
+  } else {
+    hi_addr_mask = ~((1ULL << m_high_memory_addr_bits) - 1);
+  }
+
+  process.SetCodeAddressMask(low_addr_mask);
+  process.SetDataAddressMask(low_addr_mask);
+
+  if (low_addr_mask != hi_addr_mask) {
+    process.SetHighmemCodeAddressMask(hi_addr_mask);
+    process.SetHighmemDataAddressMask(hi_addr_mask);
   }
 }