[Lldb-commits] [PATCH] D147462: Use kernel's global variable indicating how many bits are used in addressing when loading Darwin xnu kernel

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1087
+      if (symbol && wordsize == 8) {
+        if (symbol->GetByteSizeIsValid()) {
+          // Mark all bits as addressable so we don't strip any from our
----------------
What does this check add? Should this be:
```
if (symbol && symbol->GetByteSize() == wordsize ==8)
```


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1090
+          // memory read below, with an incorrect default value.
+          uint32_t old_bits_value = m_process->GetVirtualAddressableBits();
+          // b55 is the sign extension bit with PAC, b56:63 are TBI,
----------------
Maybe make this `const` to make it clear nobody should modify this. 


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1093
+          // don't mark those as addressable.
+          m_process->SetVirtualAddressableBits((wordsize * 8) - 9);
+          addr_t sym_addr = symbol->GetLoadAddress(&m_process->GetTarget());
----------------
Instead of changing the addressable bits for the process, should we modify `GetLoadAddress` to explicitly skip the stripping instead? This is probably purely theoretical, but what if another (host) thread tried to read memory in the meantime? Changing global state like this can lead to subtle bugs. 


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1101
+          // this problem.
+          const size_t bytesize = 8; // size of gT1Sz value
+          uint64_t sym_value =
----------------
You can probably just reuse `wordsize` here?


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1111
+            // addressing, bits 39..63 are used for PAC/TBI or whatever.
+            int virt_addr_bits = 64 - sym_value;
+            m_process->SetVirtualAddressableBits(virt_addr_bits);
----------------
Why is this singed? This should be a `uint32_t`.


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1114
+          } else {
+            if (old_bits_value != 0)
+              m_process->SetVirtualAddressableBits(old_bits_value);
----------------
Is this actually necessary? Does that mean that you can't do `process->SetVirtualAddressableBits(process->GetVirtualAddressableBits())` in general?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147462/new/

https://reviews.llvm.org/D147462