[Lldb-commits] [PATCH] D88282: Add support for a "main bin spec" LC_NOTE for standalone/firmware corefile binaries in MachO corefiles

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Symbol/ObjectFile.h:94
 
+  // If we have a corefile binary hint, this enum
+  // specifies the binary type which we can use to
----------------
`///`


================
Comment at: lldb/include/lldb/Symbol/ObjectFile.h:100
+    eBinaryTypeUnknown,
+    eBinaryTypeKernel,    // kernel binary
+    eBinaryTypeUser,      // user process binary
----------------
`/// <`


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5567
               uuid = UUID::fromOptionalData(raw_uuid, sizeof(uuid_t));
+              switch (binspec_type) {
+              case 0:
----------------
It seems like this could be outlined into a little static helper function. Something along the lines of 

```
BinaryType GetBinaryType(uint32_t binspec_type);
```


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:471
+      if (m_mach_kernel_addr != LLDB_INVALID_ADDRESS) {
+        LLDB_LOGF(log,
+                  "ProcessMachCore::DoLoadCore: Using kernel corefile image "
----------------
Maybe have a little lambda for the two bodies to reduce the duplication? 


================
Comment at: lldb/test/API/macosx/lc-note/firmware-corefile/create-empty-corefile.cpp:39
+add_uint64(std::vector<uint8_t> &buf, uint64_t val)
+{
+    uint64_buf conv;
----------------
This does not look clang-formatted 🤐


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88282