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

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks for the feedback!



================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5567
               uuid = UUID::fromOptionalData(raw_uuid, sizeof(uuid_t));
+              switch (binspec_type) {
+              case 0:
----------------
JDevlieghere wrote:
> It seems like this could be outlined into a little static helper function. Something along the lines of 
> 
> ```
> BinaryType GetBinaryType(uint32_t binspec_type);
> ```
I don't feel strongly, but all of the "main bin spec" specific knowledge is centralized here in GetCorefileMainBinaryInfo, and converting the constant values this LC_NOTE uses for binary type into the enumerated values seems best kept local to this area.


================
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 "
----------------
JDevlieghere wrote:
> Maybe have a little lambda for the two bodies to reduce the duplication? 
Yeah, it's not great - but we're checking them in opposite order in the duplicated blocks, so I could make a lambda that did both, but it would need to take an arg about which to prefer and wouldn't be that much better I think.


================
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;
----------------
JDevlieghere wrote:
> This does not look clang-formatted 🤐
oh my how did that happen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88282