This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB340578: Restrict the set of plugins used for
ProcessMinidump (authored by lemo, committed by ).
Repository:
rLLDB LLDB
https://reviews.llvm.org/D51176
Files:
source/Plugins/Process/minidump/Proce
lemo marked an inline comment as done.
lemo added a comment.
In https://reviews.llvm.org/D51176#1211433, @jingham wrote:
> The other option would be to move the code that populates the section load
> list into the mini dump dynamic loader. That has the benefit of keeping this
> consistent with
lemo updated this revision to Diff 162270.
lemo added a comment.
Added the comment requested by zturner
https://reviews.llvm.org/D51176
Files:
source/Plugins/Process/minidump/ProcessMinidump.cpp
source/Plugins/Process/minidump/ProcessMinidump.h
Index: source/Plugins/Process/minidump/Proce
zturner accepted this revision.
zturner added inline comments.
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:399-404
+JITLoaderList &ProcessMinidump::GetJITLoaders() {
+ if (!m_jit_loaders_ap) {
+m_jit_loaders_ap = llvm::make_unique();
+ }
+ return *m_jit
lemo added subscribers: bgianfo, clayborg, zturner.
lemo added a comment.
It's an interesting idea, thanks! I don't object moving code around if
there's a strong case for it, but I'd like to keep the fix small and simple
for now, but it's worth considering if the current minidump loading path
will
It's an interesting idea, thanks! I don't object moving code around if
there's a strong case for it, but I'd like to keep the fix small and simple
for now, but it's worth considering if the current minidump loading path
will need more flexibility.
On Thu, Aug 23, 2018 at 1:27 PM, Greg Clayton via
clayborg added a comment.
In https://reviews.llvm.org/D51176#1211433, @jingham wrote:
> The other option would be to move the code that populates the section load
> list into the mini dump dynamic loader. That has the benefit of keeping this
> consistent with the other process plugins, but OTO
jingham added a comment.
The other option would be to move the code that populates the section load list
into the mini dump dynamic loader. That has the benefit of keeping this
consistent with the other process plugins, but OTOH is just moving code
around...
Repository:
rLLDB LLDB
https:/
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
I am fine with this change then. Probably best to get the OK from Zach as well.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D51176
___
lemo added a comment.
In https://reviews.llvm.org/D51176#1211358, @clayborg wrote:
> So this might actually work. Take a look around and see what else the dynamic
> loader is used for and make sure that they won't be needed for anything else.
> If not, this fix should work, but we should docume
clayborg added a comment.
So this might actually work. Take a look around and see what else the dynamic
loader is used for and make sure that they won't be needed for anything else.
If not, this fix should work, but we should document it.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D511
lemo added a comment.
In https://reviews.llvm.org/D51176#1211307, @clayborg wrote:
> Dynamic loaders will fill out the section load list in the target that saids
> "__TEXT" from "/tmp/a.out" was loaded at address 0x120200. So yes they
> are needed. If the process minidump is manually settin
lemo added a comment.
> That's not changing the module list, that's changing the loaded section
> information. It is the dynamic loader's job to figure out what got loaded
> where, plus your stack trace show this happening after we've selected a
> plugin, not in the process of negotiating for t
clayborg added a comment.
Dynamic loaders will fill out the section load list in the target that saids
"__TEXT" from "/tmp/a.out" was loaded at address 0x120200. So yes they are
needed. If the process minidump is manually setting the section load list
itself, then there really is no need fo
jingham added a comment.
That's not changing the module list, that's changing the loaded section
information. It is the dynamic loader's job to figure out what got loaded
where, plus your stack trace show this happening after we've selected a plugin,
not in the process of negotiating for the p
lemo added a comment.
> Dynamic loaders are needed for loading breakpad minidumps that are for MacOSX
> and iOS processes. They should also be needed for loading any minidumps that
> have stack traces.
Thanks. I just validated the change against a macOS minidump and everything
works fine in my
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Dynamic loaders are needed for loading breakpad minidumps that are for MacOSX
and iOS processes. They should also be needed for loading any minidumps that
have stack traces.
Re
clayborg added a comment.
No dynamic loader plug-ins should be affecting the module list during the
plug-in loading/selection, if that is happening, that is a bug and it should be
fixed.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D51176
__
lemo created this revision.
lemo added reviewers: clayborg, zturner.
lemo added a project: LLDB.
Herald added a subscriber: abidh.
1. The dynamic loaders should not be needed for loading minidumps and they may
create problems (ex. the macOS loader resets the list of loaded modules)
2. In general,
19 matches
Mail list logo
