[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

58–60

Please implement operator bool() instead.

62–64

Where is this called?

[jasonmolenda]

lldb/source/Utility/AddressableBits.cpp

58–60

ok.

62–64

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

62–64

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

62–64

Why is this returning a bool and not AddressableBits?

[jasonmolenda]

lldb/source/Utility/AddressableBits.cpp

62–64

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/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 SetLowmemAddressableBits(uint32_t lowmem_addressing_bits);
+
+  void SetHighmemAddressableBits(uint32_t highmem_addressing_bits);
+
   void SetProcessMasks(lldb_private::Process &process);
 
+  bool HasValue() const;
+
   void Clear();
 
 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/Process/gdb-remote/ProcessGDBRemote.cpp

     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);

 
         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));
       }
     }
 

       // one.
       UpdateThreadIDList();
 
       if (!m_thread_ids.empty()) {
         tid = m_thread_ids.front();
       }
     }
 
+    if (addressable_bits.HasValue()) {
+      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;

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::SetHighmemAddressableBits(
+    uint32_t highmem_addressing_bits) {
+  m_high_memory_addr_bits = highmem_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;
 
   if (m_low_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);
   }
 }
+
+bool AddressableBits::HasValue() const {
+  return m_low_memory_addr_bits != 0 || m_high_memory_addr_bits != 0;
+}
+
+void AddressableBits::Clear() {
+  m_low_memory_addr_bits = m_high_memory_addr_bits = 0;
+}