[Lldb-commits] [PATCH] D145547: When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

Some code style nits but the change itself LGTM.



================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:819-821
+    if (ondisk_object_file)
+      ondisk_objfile_macho =
+          llvm::dyn_cast<ObjectFileMachO>(ondisk_object_file);
----------------
You can use `dyn_cast_or_null` to eliminate the null check:
 ```ObjectFileMachO *ondisk_objfile_macho = llvm::dyn_cast_or_null<ObjectFileMachO>(ondisk_object_file);```

I hate temporaries that are used only once below, so I'd probably write:

```
ObjectFileMachO *ondisk_objfile_macho = llvm::dyn_cast_or_null<ObjectFileMachO>(m_module_sp ? m_module_sp->GetObjectFile() : nullptr);
```
but I'm probably the only one that likes that better.


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:835
+                      m_name.c_str(), m_uuid.GetAsString().c_str());
+            m_module_sp.reset();
+          }
----------------
What's the purpose of reseting the shared pointer? It'll just go out of scope right after. 


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:881-883
+        ObjectFileMachO *memory_objfile_macho =
+            llvm::dyn_cast<ObjectFileMachO>(memory_object_file);
+        if (memory_objfile_macho) {
----------------
Since you already did it for `Section` below...
```
if(ObjectFileMachO *memory_objfile_macho =
            llvm::dyn_cast<ObjectFileMachO>(memory_object_file)) {
```


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1146
+  return m_header.filetype == MH_KEXT_BUNDLE;
+}
 uint32_t ObjectFileMachO::GetAddressByteSize() const {
----------------
Newline below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145547