[Lldb-commits] [PATCH] D115431: Set a default number of addressing bits for Darwin arm64 systems

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda marked 12 inline comments as done.
jasonmolenda added a comment.

Thanks so much for the thorough review Davide, I really appreciate the help and time you spent doing this.  I addressed the comments and will update the patch momentarily.

I have another small change I want to make soon where I add a new "load binary" LC_NOTE which will be a way for a corefile to tell lldb to load additional binaries for a corefile, and I'm planning to add this load binary command to the corefile and confirm that lldb loads the a.out correctly via that.

I didn't look at obj2yaml too hard, but the first thing I tried was running it on my created corefile, and it didn't include the memory contents (LC_SEGMENT_64), so I didn't pursue it much further. I knew I was going to add a custom LC_NOTE to this test corefile soon too and I don't think those are handled r.n.  It was pretty straightforward to create a simple corefile.



================
Comment at: lldb/source/DataFormatters/CXXFunctionPointer.cpp:61
+                  int addrsize = target->GetArchitecture().GetAddressByteSize();
+                  sstr.Printf("actual=0x%*.*" PRIx64 " ", addrsize * 2, addrsize * 2, fixed_addr);
+                  so_addr = test_address;
----------------
DavidSpickett wrote:
> Where does the closing ) come from, does the so_addr.Dump emit it?
This method (and Address::Dump) are concatenating text into the Stream sstr which is a symbolic description of the address; we want to print

fmain = 0xPTRAUTH (actual=0xACTUAL <name of function, source line, etc>)

so this is prepending the "actual=0xACTUAL " part, replacing the Address we're looking up with the ptrauth-stripped-off Address, and relying on Address::Dump to append its normal symbolication stuff.  Then just below in this method we have

  if (sstr.GetSize() > 0) {
    stream.Printf("(%s)", sstr.GetData());

where the parens are added.


================
Comment at: lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp:830
+  }
   return (pc & pac_sign_extension) ? pc | mask : pc & (~mask);
 }
----------------
DavidSpickett wrote:
> Can you explain this logic? The "else" part I get, it's the sign extension check I'm not sure of.
> 
> I have to admit, we have this over in AArch64 Linux and it's been on my list to find out what it actually does and document it there too. My naive assumption is that removing signature bits would just be a mask but clearly I'm missing something.
I intend this to handle the case of code running in high memory, where bit 55 is set, indicating that all of the high bits should be set to 1.  Unlike userland memory on Darwin which runs in low memory, where bit 55 is clear and so all the high bits should be cleared to 0 when removing the PAC bits.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5161
     } else {
-      // We didn't find a LC_VERSION_MIN load command and this isn't a KEXT
-      // so lets not say our Vendor is Apple, leave it as an unspecified
-      // unknown.
-      base_triple.setVendor(llvm::Triple::UnknownVendor);
-      base_triple.setVendorName(llvm::StringRef());
       add_triple(base_triple);
     }
----------------
DavidSpickett wrote:
> You could pull that out to after the if.
> 
> ```
> if (header.filetype == MH_KEXT_BUNDLE) {
>     base_triple.setVendor(llvm::Triple::Apple);
> }
> add_triple(base_triple);
> ```
Good point, altho looking at this bit of code my goal was to force Vendor to apple for any mach-o binary, not just kexts.  Whatever, the key part was not clearing the Vendor here.


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/Makefile:9
+
+include Makefile.rules
----------------
DavidSpickett wrote:
> Does a.out get built here because the default testing makefile does that? Please comment that if so, otherwise it looks like you forgot to check in the prebuilt a.out.
OK.  I copied a Makefile I used in another test, where I have a helper program and the test binary, and didn't think about how it reads.


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/create-corefile.c:18
+//
+// The corefile does not include the "addrable bits"
+// LC_NOTE, so lldb will need to fall back on its 
----------------
DavidSpickett wrote:
> Is the name spelled like that or should it be "addressable"?
The LC_NOTE names are only 16 characters, so unfortunately, "addrable bits" it is. :) 


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/create-corefile.c:57
+
+  int size_of_thread_cmd = 4 + 4 + 4 + 4 + sizeof (arm_thread_state64_t);
+
----------------
DavidSpickett wrote:
> What are the 4s?
The LC_THREAD load command has four uint32_t's at the start, but you're right and I was even thinking of commenting in this last night & did not.  I added a comment with the format of LC_THREAD.


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/create-corefile.c:92
+  fwrite (&flavor, sizeof (flavor), 1, out);
+  uint32_t count = sizeof (arm_thread_state64_t) / 4;
+  fwrite (&count, sizeof (count), 1, out);
----------------
DavidSpickett wrote:
> Why /4 not /8 here? Assuming 64 bit registers.
> 
> Is count in 32 bit words? (I found some sources on google but they didn't include the arm64 version)
It's a Darwin specific feature register contexts; most API accept a register state "flavor" and a size, expressed in number of 32-bit words of content.  So if a register context grows, a kernel etc can distinguish between an older smaller regctx and a newer larger one that both use the same flavor enum.  (in reality I don't think I've ever seen this done with a register flavor as far as I can remember, but it's a common pattern used elsewhere too).  Added a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115431