[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo added subscribers: bgianfo, penryu. lemo added a comment. > It looks like nobody except me is worried about the > module-without-an-object-file situation, so I guess we can try this out and > see how it goes. Sorry Pavel, I meant to respond to this: most of the code seems to explicitly handle this case (module-without-object-file), I just had to fix a couple of cases. It's possible that more fixes will be required, but the intention seems to be to accommodate for missing object files so at least in this area I didn't have to break new ground. I also considered creating placeholder object files, but it proved a bit more intrusive since there are numerous places where it's assumed that object files map to a real file which can be read and written to. Maybe at some point we'll need to reconsider this (placeholder object files) but for the initial iteration the placeholder modules seems to be sufficient. The only downside I noticed is mostly cosmetic, for example things like "target modules dump objfile" may print empty lines for the missing object files. The test you've added here has been failing on windows though. I've tried > to fix this in r330314, but it meant modifying the module.file.fullpath > output expectations. I'm not sure where you're going with the minidump > support, but if you are bothered by module.file.fullpath containing a > forward slash, you may want to look into fixing the SBFileSpec behavior. Thanks! Repository: rL LLVM https://reviews.llvm.org/D45700 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
> > It looks like nobody except me is worried about the > module-without-an-object-file situation, so I guess we can try this out and > see how it goes. > Sorry Pavel, I meant to respond to this: most of the code seems to explicitly handle this case (module-without-object-file), I just had to fix a couple of cases. It's possible that more fixes will be required, but the intention seems to be to accommodate for missing object files so at least in this area I didn't have to break new ground. I also considered creating placeholder object files, but it proved a bit more intrusive since there are numerous places where it's assumed that object files map to a real file which can be read and written to. Maybe at some point we'll need to reconsider this (placeholder object files) but for the initial iteration the placeholder modules seems to be sufficient. The only downside I noticed is mostly cosmetic, for example things like "target modules dump objfile" may print empty lines for the missing object files. The test you've added here has been failing on windows though. I've tried > to fix this in r330314, but it meant modifying the module.file.fullpath > output expectations. I'm not sure where you're going with the minidump > support, but if you are bothered by module.file.fullpath containing a > forward slash, you may want to look into fixing the SBFileSpec behavior. > Thanks! On Thu, Apr 19, 2018 at 2:47 AM, Pavel Labath via Phabricator < revi...@reviews.llvm.org> wrote: > labath added a comment. > > It looks like nobody except me is worried about the > module-without-an-object-file situation, so I guess we can try this out and > see how it goes. > > The test you've added here has been failing on windows though. I've tried > to fix this in r330314, but it meant modifying the module.file.fullpath > output expectations. I'm not sure where you're going with the minidump > support, but if you are bothered by module.file.fullpath containing a > forward slash, you may want to look into fixing the SBFileSpec behavior. > > > Repository: > rL LLVM > > https://reviews.llvm.org/D45700 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
labath added a comment. It looks like nobody except me is worried about the module-without-an-object-file situation, so I guess we can try this out and see how it goes. The test you've added here has been failing on windows though. I've tried to fix this in r330314, but it meant modifying the module.file.fullpath output expectations. I'm not sure where you're going with the minidump support, but if you are bothered by module.file.fullpath containing a forward slash, you may want to look into fixing the SBFileSpec behavior. Repository: rL LLVM https://reviews.llvm.org/D45700 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
clayborg added a comment. LGTM Repository: rL LLVM https://reviews.llvm.org/D45700 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
amccarth accepted this revision. amccarth added a comment. I actually preferred the factory solution. What I intended to express is that the modification of the target doesn't seems like it should be inside the PlaceholderModule class, so whether you use a factory or not doesn't really address that aspect of the coupling. In my head, modification of the target should be done by the client that instantiates the PlaceholderModule (whether it does that via constructor or factory). But this isn't a new problem, so I'm happy to consider the coupling problem outside the scope of this patch. https://reviews.llvm.org/D45700 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
Greg/Pavel, does the latest revision look good to you? Thanks!
On Wed, Apr 18, 2018 at 10:31 AM, Leonard Mosescu via Phabricator <
revi...@reviews.llvm.org> wrote:
> lemo marked an inline comment as done.
> lemo added a comment.
>
> In https://reviews.llvm.org/D45700#1070491, @amccarth wrote:
>
> > LGTM, but consider highlighting the side effect to `target` when the
> factory makes a Placeholder module.
>
>
> Good observation: taking a step back, the factory introduces too much
> coupling, especially if we want to extend this placeholder module approach
> to other uses. Because of this, I reverted back to the standalone
> PlaceholderModule::CreateImageSection() approach. Thanks Adrian!
>
>
>
>
> Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:70
> + // Creates a synthetic module section covering the whole module image
> + void CreateImageSection(const MinidumpModule *module, Target& target) {
> +const ConstString section_name(".module_image");
>
> amccarth wrote:
> > I didn't notice before that target is a non-const ref. Maybe the
> comment should explain why target is modified (and/or maybe in
> PlaceholderModule::Create).
> Updated the function comment. This is similar to how other places set the
> section load address (ex. ObjectFileELF::SetLoadAddress)
>
>
> https://reviews.llvm.org/D45700
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo added subscribers: amccarth, clayborg, labath, zturner. lemo marked an inline comment as done. lemo added a comment. Greg/Pavel, does the latest revision look good to you? Thanks! https://reviews.llvm.org/D45700 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo marked an inline comment as done.
lemo added a comment.
In https://reviews.llvm.org/D45700#1070491, @amccarth wrote:
> LGTM, but consider highlighting the side effect to `target` when the factory
> makes a Placeholder module.
Good observation: taking a step back, the factory introduces too much coupling,
especially if we want to extend this placeholder module approach to other uses.
Because of this, I reverted back to the standalone
PlaceholderModule::CreateImageSection() approach. Thanks Adrian!
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:70
+ // Creates a synthetic module section covering the whole module image
+ void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");
amccarth wrote:
> I didn't notice before that target is a non-const ref. Maybe the comment
> should explain why target is modified (and/or maybe in
> PlaceholderModule::Create).
Updated the function comment. This is similar to how other places set the
section load address (ex. ObjectFileELF::SetLoadAddress)
https://reviews.llvm.org/D45700
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo updated this revision to Diff 142960.
https://reviews.llvm.org/D45700
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
source/Commands/CommandObjectTarget.cpp
source/Core/Module.cpp
source/Core/Section.cpp
source/Plugins/Process/minidump/ProcessMinidump.cpp
source/Plugins/Process/minidump/ProcessMinidump.h
Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -61,6 +61,8 @@
uint32_t GetPluginVersion() override;
+ SystemRuntime *GetSystemRuntime() override { return nullptr; }
+
Status DoDestroy() override;
void RefreshStateAfterStop() override;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -19,6 +19,7 @@
#include "lldb/Core/State.h"
#include "lldb/Target/DynamicLoader.h"
#include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/SectionLoadList.h"
#include "lldb/Target/Target.h"
#include "lldb/Target/UnixSignals.h"
#include "lldb/Utility/DataBufferLLVM.h"
@@ -31,9 +32,53 @@
// C includes
// C++ includes
+using namespace lldb;
using namespace lldb_private;
using namespace minidump;
+//--
+/// A placeholder module used for minidumps, where the original
+/// object files may not be available (so we can't parse the object
+/// files to extract the set of sections/segments)
+///
+/// This placeholder module has a single synthetic section (.module_image)
+/// which represents the module memory range covering the whole module.
+//--
+class PlaceholderModule : public Module {
+public:
+ PlaceholderModule(const FileSpec _spec, const ArchSpec ) :
+Module(file_spec, arch) {}
+
+ // Creates a synthetic module section covering the whole module image
+ // (and sets the section load address as well)
+ void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");
+lldb::SectionSP section_sp(new Section(
+shared_from_this(), // Module to which this section belongs.
+nullptr,// ObjectFile
+0, // Section ID.
+section_name, // Section name.
+eSectionTypeContainer, // Section type.
+module->base_of_image, // VM address.
+module->size_of_image, // VM size in bytes of this section.
+0, // Offset of this section in the file.
+module->size_of_image, // Size of the section as found in the file.
+12, // Alignment of the section (log2)
+0, // Flags for this section.
+1));// Number of host bytes per target byte
+section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
+GetSectionList()->AddSection(section_sp);
+target.GetSectionLoadList().SetSectionLoadAddress(
+section_sp, module->base_of_image);
+ }
+
+ ObjectFile *GetObjectFile() override { return nullptr; }
+
+ SectionList *GetSectionList() override {
+return Module::GetUnifiedSectionList();
+ }
+};
+
ConstString ProcessMinidump::GetPluginNameStatic() {
static ConstString g_name("minidump");
return g_name;
@@ -281,7 +326,18 @@
Status error;
lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, );
if (!module_sp || error.Fail()) {
- continue;
+ // We failed to locate a matching local object file. Fortunately,
+ // the minidump format encodes enough information about each module's
+ // memory range to allow us to create placeholder modules.
+ //
+ // This enables most LLDB functionality involving address-to-module
+ // translations (ex. identifing the module for a stack frame PC) and
+ // modules/sections commands (ex. target modules list, ...)
+ auto placeholder_module =
+ std::make_shared(file_spec, GetArchitecture());
+ placeholder_module->CreateImageSection(module, GetTarget());
+ module_sp = placeholder_module;
+ GetTarget().GetImages().Append(module_sp);
}
if (log) {
Index: source/Core/Section.cpp
===
--- source/Core/Section.cpp
+++ source/Core/Section.cpp
@@ -326,10 +326,11 @@
// The top most section prints the module basename
const char *name = NULL;
ModuleSP module_sp(GetModule());
-const FileSpec _spec = m_obj_file->GetFileSpec();
-if
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.
LGTM, but consider highlighting the side effect to `target` when the factory
makes a Placeholder module.
In the future, we need to refactor the minidump tests to get rid of the
redundancy.
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:70
+ // Creates a synthetic module section covering the whole module image
+ void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");
I didn't notice before that target is a non-const ref. Maybe the comment
should explain why target is modified (and/or maybe in
PlaceholderModule::Create).
https://reviews.llvm.org/D45700
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo marked 2 inline comments as done. lemo added inline comments. Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53 +for module in self.target.modules: +self.assertTrue(module.IsValid()) + labath wrote: > lemo wrote: > > labath wrote: > > > Could we strengthen these assertions a bit. Given that this is static > > > data you are loading, I don't see why you couldn't hard-code the names of > > > the modules you should expect. > > Done. > > > > Just curious, do we have any support for golden output files? (it would be > > great for tests like this) > If you're feeling adventurous, you can try rewriting this as a lit test. You > should be able to just do a `lldb -c my-core -o "image list"` and FileCheck > the output. Thanks for the tip, I'll keep this in mind for the future. https://reviews.llvm.org/D45700 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
labath added inline comments. Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53 +for module in self.target.modules: +self.assertTrue(module.IsValid()) + lemo wrote: > labath wrote: > > Could we strengthen these assertions a bit. Given that this is static data > > you are loading, I don't see why you couldn't hard-code the names of the > > modules you should expect. > Done. > > Just curious, do we have any support for golden output files? (it would be > great for tests like this) If you're feeling adventurous, you can try rewriting this as a lit test. You should be able to just do a `lldb -c my-core -o "image list"` and FileCheck the output. https://reviews.llvm.org/D45700 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo marked 4 inline comments as done.
lemo added inline comments.
Comment at:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53
+for module in self.target.modules:
+self.assertTrue(module.IsValid())
+
labath wrote:
> Could we strengthen these assertions a bit. Given that this is static data
> you are loading, I don't see why you couldn't hard-code the names of the
> modules you should expect.
Done.
Just curious, do we have any support for golden output files? (it would be
great for tests like this)
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");
labath wrote:
> lemo wrote:
> > amccarth wrote:
> > > 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.
> > Thanks for the suggestion. I agree, this would look a bit cleaner to fold
> > everything in the constructor (except the extra arguments to the
> > constructor, but it will probably still be a net win)
> >
> > The reason for a separate method is the little "shared_from_this()". It
> > can't be done from the constructor since the object is not yet managed by a
> > shared_ptr, so we need to do it post-construction.
> This can be solved by hiding the constructor and having a static factory
> function which returns a shared_ptr.
The factory is a great idea, done. (one downside with a private constructor is
that we lose the benefits of std::make_shared though)
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:47
+//--
+class PlaceholderModule : public Module {
+public:
labath wrote:
> clayborg wrote:
> > I would be worth putting this class maybe in the same folder as
> > lldb_private::Module and possibly renaming it. I can see this kind of thing
> > being useful for symbolication in general and it won't be limited to use in
> > minidumps. It should have enough accessors that allows an external client
> > to modify everything.
> I concur. Besides postmortem, we can run into the situation where we cannot
> access the loaded modules for live debugging as well.
Thanks, I agree. I think this needs a bit more baking first though, I say let's
put it through some use first (and maybe identify a 2nd use case).
(in particular we'd also need a more generic interface and I'd like to avoid
creating an overly complex solution which may not even fit future use cases)
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:79
+private:
+ SectionList m_sections;
+};
clayborg wrote:
> Just use lldb_private::Module::m_sections_ap and put the sections in there?
> Any reason not to?
Thanks, done. (no good reason, I was just extra cautious not to introduce any
unexpected side-effects)
https://reviews.llvm.org/D45700
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo updated this revision to Diff 142799.
lemo edited the summary of this revision.
lemo added a comment.
Thanks for the feedback.
PS. The sample output in the description if from LLDB running on Linux, reading
minidumps captured on Windows.
https://reviews.llvm.org/D45700
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
source/Commands/CommandObjectTarget.cpp
source/Core/Module.cpp
source/Core/Section.cpp
source/Plugins/Process/minidump/ProcessMinidump.cpp
source/Plugins/Process/minidump/ProcessMinidump.h
Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -61,6 +61,8 @@
uint32_t GetPluginVersion() override;
+ SystemRuntime *GetSystemRuntime() override { return nullptr; }
+
Status DoDestroy() override;
void RefreshStateAfterStop() override;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -19,6 +19,7 @@
#include "lldb/Core/State.h"
#include "lldb/Target/DynamicLoader.h"
#include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/SectionLoadList.h"
#include "lldb/Target/Target.h"
#include "lldb/Target/UnixSignals.h"
#include "lldb/Utility/DataBufferLLVM.h"
@@ -31,9 +32,63 @@
// C includes
// C++ includes
+using namespace lldb;
using namespace lldb_private;
using namespace minidump;
+//--
+/// A placeholder module used for minidumps, where the original
+/// object files may not be available (so we can't parse the object
+/// files to extract the set of sections/segments)
+///
+/// This placeholder module has a single synthetic section (.module_image)
+/// which represents the module memory range covering the whole module.
+//--
+class PlaceholderModule : public Module {
+public:
+ static std::shared_ptr Create(const FileSpec _spec,
+ const ArchSpec ,
+ const MinidumpModule *module,
+ Target ) {
+std::shared_ptr placeholder_module(
+new PlaceholderModule(file_spec, arch));
+placeholder_module->CreateImageSection(module, target);
+return placeholder_module;
+ }
+
+ ObjectFile *GetObjectFile() override { return nullptr; }
+
+ SectionList *GetSectionList() override {
+return Module::GetUnifiedSectionList();
+ }
+
+private:
+ PlaceholderModule(const FileSpec _spec, const ArchSpec ) :
+Module(file_spec, arch) {}
+
+ // Creates a synthetic module section covering the whole module image
+ void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");
+lldb::SectionSP section_sp(new Section(
+shared_from_this(), // Module to which this section belongs.
+nullptr,// ObjectFile
+0, // Section ID.
+section_name, // Section name.
+eSectionTypeContainer, // Section type.
+module->base_of_image, // VM address.
+module->size_of_image, // VM size in bytes of this section.
+0, // Offset of this section in the file.
+module->size_of_image, // Size of the section as found in the file.
+12, // Alignment of the section (log2)
+0, // Flags for this section.
+1));// Number of host bytes per target byte
+section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
+GetSectionList()->AddSection(section_sp);
+target.GetSectionLoadList().SetSectionLoadAddress(
+section_sp, module->base_of_image);
+ }
+};
+
ConstString ProcessMinidump::GetPluginNameStatic() {
static ConstString g_name("minidump");
return g_name;
@@ -281,7 +336,16 @@
Status error;
lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, );
if (!module_sp || error.Fail()) {
- continue;
+ // We failed to locate a matching local object file. Fortunately,
+ // the minidump format encodes enough information about each module's
+ // memory range to allow us to create placeholder modules.
+ //
+ // This enables most LLDB functionality involving address-to-module
+ // translations (ex. identifing the module for a stack frame PC) and
+ // modules/sections commands (ex. target modules list, ...)
+ module_sp =
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
clayborg added a comment.
Looks good. I questions if we want PlaceholderModule to be available for all
symbolicators/core dump plugins. See inlined comments.
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:47
+//--
+class PlaceholderModule : public Module {
+public:
I would be worth putting this class maybe in the same folder as
lldb_private::Module and possibly renaming it. I can see this kind of thing
being useful for symbolication in general and it won't be limited to use in
minidumps. It should have enough accessors that allows an external client to
modify everything.
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:79
+private:
+ SectionList m_sections;
+};
Just use lldb_private::Module::m_sections_ap and put the sections in there? Any
reason not to?
https://reviews.llvm.org/D45700
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo updated this revision to Diff 142706.
lemo edited the summary of this revision.
https://reviews.llvm.org/D45700
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
source/Commands/CommandObjectTarget.cpp
source/Core/Section.cpp
source/Plugins/Process/minidump/ProcessMinidump.cpp
source/Plugins/Process/minidump/ProcessMinidump.h
Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -61,6 +61,8 @@
uint32_t GetPluginVersion() override;
+ SystemRuntime *GetSystemRuntime() override { return nullptr; }
+
Status DoDestroy() override;
void RefreshStateAfterStop() override;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -19,6 +19,7 @@
#include "lldb/Core/State.h"
#include "lldb/Target/DynamicLoader.h"
#include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/SectionLoadList.h"
#include "lldb/Target/Target.h"
#include "lldb/Target/UnixSignals.h"
#include "lldb/Utility/DataBufferLLVM.h"
@@ -31,9 +32,53 @@
// C includes
// C++ includes
+using namespace lldb;
using namespace lldb_private;
using namespace minidump;
+//--
+/// A placeholder module used for minidumps, where the original
+/// object files may not be available (so we can't parse the object
+/// files to extract the set of sections/segments)
+///
+/// This placeholder module has a single synthetic section (.module_image)
+/// which represents the module memory range covering the whole module.
+//--
+class PlaceholderModule : public Module {
+public:
+ PlaceholderModule(const FileSpec _spec, const ArchSpec ) :
+Module(file_spec, arch) {}
+
+ // Creates a synthetic module section covering the whole module image
+ void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");
+lldb::SectionSP section_sp(new Section(
+shared_from_this(), // Module to which this section belongs.
+nullptr,// ObjectFile
+0, // Section ID.
+section_name, // Section name.
+eSectionTypeContainer, // Section type.
+module->base_of_image, // VM address.
+module->size_of_image, // VM size in bytes of this section.
+0, // Offset of this section in the file.
+module->size_of_image, // Size of the section as found in the file.
+12, // Alignment of the section (log2)
+0, // Flags for this section.
+1));// Number of host bytes per target byte
+section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
+m_sections.AddSection(section_sp);
+target.GetSectionLoadList().SetSectionLoadAddress(
+section_sp, module->base_of_image);
+ }
+
+ ObjectFile *GetObjectFile() override { return nullptr; }
+
+ SectionList *GetSectionList() override { return _sections; }
+
+private:
+ SectionList m_sections;
+};
+
ConstString ProcessMinidump::GetPluginNameStatic() {
static ConstString g_name("minidump");
return g_name;
@@ -281,7 +326,18 @@
Status error;
lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, );
if (!module_sp || error.Fail()) {
- continue;
+ // We failed to locate a matching local object file. Fortunately,
+ // the minidump format encodes enough information about each module's
+ // memory range to allow us to create placeholder modules.
+ //
+ // This enables most LLDB functionality involving address-to-module
+ // translations (ex. identifing the module for a stack frame PC) and
+ // modules/sections commands (ex. target modules list, ...)
+ auto placeholder_module =
+ std::make_shared(file_spec, GetArchitecture());
+ placeholder_module->CreateImageSection(module, GetTarget());
+ module_sp = placeholder_module;
+ GetTarget().GetImages().Append(module_sp);
}
if (log) {
Index: source/Core/Section.cpp
===
--- source/Core/Section.cpp
+++ source/Core/Section.cpp
@@ -326,10 +326,11 @@
// The top most section prints the module basename
const char *name = NULL;
ModuleSP module_sp(GetModule());
-const FileSpec _spec = m_obj_file->GetFileSpec();
-if (m_obj_file)
+if (m_obj_file) {
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo added inline comments.
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");
amccarth wrote:
> 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.
Thanks for the suggestion. I agree, this would look a bit cleaner to fold
everything in the constructor (except the extra arguments to the constructor,
but it will probably still be a net win)
The reason for a separate method is the little "shared_from_this()". It can't
be done from the constructor since the object is not yet managed by a
shared_ptr, so we need to do it post-construction.
https://reviews.llvm.org/D45700
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
amccarth added a comment.
Nice!
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.
https://reviews.llvm.org/D45700
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps
lemo created this revision.
lemo added reviewers: clayborg, labath, amccarth.
lemo added a project: LLDB.
Herald added subscribers: JDevlieghere, aprantl.
Normally, LLDB is creating a high-fidelity representation of a live
process, including a list of modules and sections, with the
associated memory address ranges. In order to build the module and
section map LLDB tries to locate the local module image (object file)
and will parse it.
This does not work for postmortem debugging scenarios where the crash
dump (minidump in this case) was captured on a different machine.
Also, we may want to completly disable the search for matching
local object files if we load minidumps unless we can prove that the
local image matches the one from the crash origin.
(not part of this change, see: llvm.org/pr35193)
Fortunatelly the minidump format encodes enough information about
each module's memory range to allow us to create placeholder modules.
This enables most LLDB functionality involving address-to-module
translations:
Example: Identify the module from a stack frame PC:
---
Before:
- thread #1, stop reason = Exception 0xc005 encountered at address 0x164d14
- frame #0: 0x00164d14 frame #1: 0x00167c79 frame #2: 0x00167e6d frame #3:
0x7510336a frame #4: 0x77759882 frame #5: 0x77759855
After:
- thread #1, stop reason = Exception 0xc005 encountered at address 0x164d14
- frame #0: 0x00164d14 C:\Users\amccarth\Documents\Visual Studio
2013\Projects\fizzbuzz\Debug\fizzbuzz.exe frame #1: 0x00167c79
C:\Users\amccarth\Documents\Visual Studio
2013\Projects\fizzbuzz\Debug\fizzbuzz.exe frame #2: 0x00167e6d
C:\Users\amccarth\Documents\Visual Studio
2013\Projects\fizzbuzz\Debug\fizzbuzz.exe frame #3: 0x7510336a
C:\Windows\SysWOW64\kernel32.dll frame #4: 0x77759882
C:\Windows\SysWOW64\ntdll.dll frame #5: 0x77759855 C:\Windows\SysWOW64\ntdll.dll
Example: target modules list
Before:
error: the target has no associated executable images
After:
[ 0] C:\Windows\System32\MSVCP120D.dll
[ 1] C:\Windows\SysWOW64\kernel32.dll
[ 2] C:\Users\amccarth\Documents\Visual Studio
2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
[ 3] C:\Windows\System32\MSVCR120D.dll
[ 4] C:\Windows\SysWOW64\KERNELBASE.dll
[ 5] C:\Windows\SysWOW64\ntdll.dll
NOTE: the minidump format also includes the debug info GUID, so we can
fill-in the module UUID from it, but this part was excluded from this change
to keep the changes simple (the LLDB UUID is hardcoded to be either 16 or 20
bytes, while the CodeView GUIDs are normally 24 bytes)
https://reviews.llvm.org/D45700
Files:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
source/Commands/CommandObjectTarget.cpp
source/Core/Section.cpp
source/Plugins/Process/minidump/ProcessMinidump.cpp
source/Plugins/Process/minidump/ProcessMinidump.h
Index: source/Plugins/Process/minidump/ProcessMinidump.h
===
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -61,6 +61,8 @@
uint32_t GetPluginVersion() override;
+ SystemRuntime *GetSystemRuntime() override { return nullptr; }
+
Status DoDestroy() override;
void RefreshStateAfterStop() override;
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -19,6 +19,7 @@
#include "lldb/Core/State.h"
#include "lldb/Target/DynamicLoader.h"
#include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/SectionLoadList.h"
#include "lldb/Target/Target.h"
#include "lldb/Target/UnixSignals.h"
#include "lldb/Utility/DataBufferLLVM.h"
@@ -31,9 +32,53 @@
// C includes
// C++ includes
+using namespace lldb;
using namespace lldb_private;
using namespace minidump;
+//--
+/// A placeholder module used for minidumps, where the original
+/// object files may not be available (so we can't parse the object
+/// files to extract the set of sections/segments)
+///
+/// This placeholder module has a single synthetic section (.module_image)
+/// which represents the module memory range covering the whole module.
+//--
+class PlaceholderModule : public Module {
+public:
+ PlaceholderModule(const FileSpec _spec, const ArchSpec ) :
+Module(file_spec, arch) {}
+
+ // Creates a synthetic module section covering the whole module image
+ void CreateImageSection(const MinidumpModule *module, Target& target) {
+const ConstString section_name(".module_image");
+lldb::SectionSP section_sp(new Section(
