[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

Adrian McCarthy via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Apr 16 14:48:32 PDT 2018

amccarth added a comment.


Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:53
+  // Creates a synthetic module section covering the whole module image
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+    const ConstString section_name(".module_image");
I wonder if this should just be part of the constructor.  I don't see a scenario where you'd create a PlaceholderModule and not want to create exactly one fake image section.  I know the style guide is against doing lots of work in a constructor, but that's mostly because it's hard to report failures, which you're not doing here anyway.

Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:329
     if (!module_sp || error.Fail()) {
-      continue;
+      // We failed to locate a matching local object file. Fortunatelly
+      // the minidump format encodes enough information about each module's
nit:  s/Fortunatelly/Fortunately/

Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:339
+      my_module->CreateImageSection(module, GetTarget());
+      module_sp = my_module;
+      GetTarget().GetImages().Append(module_sp);
If CreateImageSection bit were done by the PlaceholderModule's constructor, then there'd be no need for the artificial `my_module` temporary, and nobody would ever make a PlaceholderModule without an image section.


More information about the lldb-commits mailing list