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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added a comment.

Glad to see this, it will be useful on AArch64 Linux as well.

General question, is yaml2obj not flexible enough to do this? I guess the issue would be even if it does do MachO the test could only work on a precompiled file, this way allows you to build `a.out` each time.

> I have ideas for how I can use this test case for some upcoming changes I want to make, so I expect to get more use out of this test case.

And having this program to build the corefile allows you the flexibility to do these future things you reference here?



================
Comment at: lldb/source/DataFormatters/CXXFunctionPointer.cpp:53
+          Process *process = exe_ctx.GetProcessPtr();
+          if (process) {
+            if (ABISP abi_sp = process->GetABI()) {
----------------
You could do `if (Process *process = ...`.


================
Comment at: lldb/source/DataFormatters/CXXFunctionPointer.cpp:55
+            if (ABISP abi_sp = process->GetABI()) {
+              addr_t fixed_addr = abi_sp->FixCodeAddress (func_ptr_address);
+              if (fixed_addr != func_ptr_address) {
----------------
Remove space before the `(`. Unless this is a code style I'm missing.


================
Comment at: lldb/source/DataFormatters/CXXFunctionPointer.cpp:58
+                Address test_address;
+                test_address.SetLoadAddress (fixed_addr, target);
+                if (test_address.GetSection() != nullptr) {
----------------
Stray space ` (` here too.


================
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;
----------------
Where does the closing ) come from, does the so_addr.Dump emit it?


================
Comment at: lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp:830
+  }
   return (pc & pac_sign_extension) ? pc | mask : pc & (~mask);
 }
----------------
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.


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


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/Makefile:9
+
+include Makefile.rules
----------------
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.


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/TestCorefileDefaultPtrauth.py:50
+
+        self.expect("ta v fmain", substrs=['fmain = 0x', 'actual=0x', 'main at main.c'])
----------------
I'd use the full commands here just for clarity.

Also given the brackets around the "actual" bit seem to be emitted by two different calls, include them in the match as well? Just in case one gets dropped at some point.


================
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 
----------------
Is the name spelled like that or should it be "addressable"?


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/create-corefile.c:30
+  if (!exe) {
+    fprintf (stderr, "Unable to open executable %s for reading\n", argv[1]);
+  }
----------------
`exit(1)` here


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/create-corefile.c:55
+  }
+  fclose (nm);
+
----------------
Should you have some validity check here that the address of main and fmain is not still 0? Just in case you manage to read half way through an nm line and miss one of them.


================
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);
+
----------------
What are the 4s?


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/create-corefile.c:59
+
+  struct mach_header_64 mh;
+  mh.magic = 0xfeedfacf;
----------------
Would help to have a comment here like:
```
// Data is written out in the order it will be in the file:
// Mach header
// Segment command
// Thread command
// Segment contents
```


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/create-corefile.c:77
+  seg.vmsize = 8;
+  seg.fileoff = sizeof (mh) + sizeof(seg) + size_of_thread_cmd;
+  seg.filesize = 8;
----------------
Do I understand correctly, this points to `segment_contents`?


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


================
Comment at: lldb/test/API/macosx/corefile-default-ptrauth/create-corefile.c:99
+
+  uint64_t segment_contents = 0xe46bff0000000000 | main_addr;
+
----------------
And here you've manually put some bits in the position of the signing bits? Please comment with that.

I assume it doesn't matter if they're generated or not, lldb isn't going to try and auth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115431