[Lldb-commits] [PATCH] D150928: Two bug fixes for loading process save-core created core files, plus perf improvements

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Bug fixes look good to me, the perf and code simplifications are also great to see. A few nits and I have one concern. Otherwise looks fine to me, I think.



================
Comment at: lldb/include/lldb/Target/DynamicLoader.h:270-273
+  LoadBinaryWithUUIDAndAddress(Process *process, llvm::StringRef name,
+                               UUID uuid, lldb::addr_t value,
+                               bool value_is_offset, bool force_symbol_search,
+                               bool notify, bool set_address_in_target);
----------------
Just thinking aloud here, I wonder if we should construct some kind of "Options" struct to contain all of these bools?


================
Comment at: lldb/source/Core/DynamicLoader.cpp:216-218
+    if (!module_sp || !module_sp->GetSymbolFileFileSpec()) {
+      Symbols::DownloadObjectAndSymbolFile(module_spec, error,
+                                           force_symbol_search);
----------------
I think this **technically** changes behavior and probably not in a desirable way. Specifically, we previously only called `DownloadObjectAndSymbolFile` if `force_symbol_search` was true. Now you're passing it through which makes sense looking at the interface of `DownloadObjectAndSymbolFile`. However, peeking at the beginning of that function we see this code block:
```
  // When dbgshell_command is empty, the user has not enabled the use of an
  // external program to find the symbols, don't run it for them.
  if (!force_lookup && dbgshell_command.empty())
    return false;
```
This means that even if `force_lookup` is false (or `force_symbol_search` as it's written here), if `dbgshell_command` is non-empty we'll still perform the lookup (assuming all the other checks pass) which is probably not what we intended performance-wise. That condition in the block above should probably be something like `!force_lookup || dbgshell_command.empty()` instead of `&&`.


================
Comment at: lldb/source/Core/DynamicLoader.cpp:246-248
+          LLDB_LOGF(log,
+                    "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+                    "binary UUID %s at %s 0x%" PRIx64,
----------------
You could use `LLDB_LOG` instead of `LLDB_LOGF` here and you'll get the file + function without explicitly writing it, but you'd have to change the formatting a bit since it uses `Format` and not `Printf` under the hood. Just a suggestion though.


================
Comment at: lldb/source/Core/DynamicLoader.cpp:255-257
+          LLDB_LOGF(log,
+                    "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+                    "binary UUID %s at file address",
----------------
Same here


================
Comment at: lldb/source/Core/DynamicLoader.cpp:264-266
+        LLDB_LOGF(log,
+                  "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading binary "
+                  "UUID %s from memory at address 0x%" PRIx64,
----------------
Also here


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:7021
         }
-        const bool address_is_slide = true;
         bool changed = false;
+        module_sp->SetLoadAddress(process.GetTarget(), value, value_is_offset,
----------------
nit: this can be const


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1000
           this, "", standalone_uuid, standalone_value,
-          standalone_value_is_offset, force_symbol_search, notify);
+          standalone_value_is_offset, force_symbol_search, notify, true);
     }
----------------
nit: `, /* set_address_in_target */ true);` (or something like this)

Or you can create a `const bool` like above with `force_symbol_search` and `notify`.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1031
+          this, llvm::StringRef(), uuid, addr, value_is_slide,
+          force_symbol_search, notify, true);
     }
----------------
Same here


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:262-264
               this, llvm::StringRef(), objfile_binary_uuid,
               objfile_binary_value, objfile_binary_value_is_offset,
+              force_symbol_search, notify, true)) {
----------------
Same here


================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:318
               this, llvm::StringRef(), ident_uuid, ident_binary_addr,
-              value_is_offset, force_symbol_search, notify)) {
+              value_is_offset, force_symbol_search, notify, true)) {
         found_main_binary_definitively = true;
----------------
Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150928