[Lldb-commits] [PATCH] D99944: [LLDB] AArch64 PAC elf-core stack unwinder support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Setting the address size from the register context is a bit awkward. We have one register context per thread, and they all will be competing to set the value (it should always be the same value under normal circumstances, but it still makes for a strange relationship). It would be better if all of this could be contained in the ABI class. The ABI classes have intimate knowledge of the registers anyway, so it would not be unusual for it to access a specific register to get the mask (here I am assuming that the mask is actually stored in a register).

Then we'd have no need for Process::GetAddressBitsInUse and worry whether it has been initialized, as the abi class could fetch the value when it gets constructed, or something...

Do you envision using this in some other way than through the ABI class?



================
Comment at: lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp:41-42
+  uint32_t address_shift_width = GetProcessSP()->GetAddressBitsInUse();
+  if (address_shift_width &&
+      (address_shift_width < (sizeof(lldb::addr_t) * CHAR_BIT))) {
+    lldb::addr_t sign = (lldb::addr_t)1 << (address_shift_width - 1);
----------------
I think this should be an assert (here, or even somewhere earlier).


================
Comment at: lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp:43-45
+    lldb::addr_t sign = (lldb::addr_t)1 << (address_shift_width - 1);
+    pc &= ((lldb::addr_t)1 << address_shift_width) - 1;
+    pc = (pc ^ sign) - sign;
----------------
return llvm::SignExtend64(pc, address_shift_width)


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

https://reviews.llvm.org/D99944