[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lit/Minidump/Windows/Sigsegv/sigsegv.test:6
+CHECK: * thread #1, stop reason = Exception 0xc005 encountered at address 
0x7ff7a13110d9
+CHECK:   * frame #0: 0x7ff7a13110d9 sigsegv.exe`crash at sigsegv.cpp:22
+

teemperor wrote:
> This test fails for me because lldb is picking up my custom lldbinit (which 
> changes the format of the frame output). Not sure what's the right way to fix 
> it, but setting a custom fixed frameformat in the sigsegv.lldbinit seems like 
> an obvious solution.
Are you sure you have the right patch here?

Regardless, I actually think the right solution would be to pass 
`--no-lldb-init` as a part of the `%lldb` substitution (I'm surprised we don't 
do that already) to make sure **no** test sources random commands from users' 
.lldbinit files.


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: lit/Minidump/Windows/Sigsegv/sigsegv.test:6
+CHECK: * thread #1, stop reason = Exception 0xc005 encountered at address 
0x7ff7a13110d9
+CHECK:   * frame #0: 0x7ff7a13110d9 sigsegv.exe`crash at sigsegv.cpp:22
+

This test fails for me because lldb is picking up my custom lldbinit (which 
changes the format of the frame output). Not sure what's the right way to fix 
it, but setting a custom fixed frameformat in the sigsegv.lldbinit seems like 
an obvious solution.


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Some interesting ideas here. I like some of them, but I have reservations about 
others. I'm going to go through them one by one.

In D55142#1333077 , @zturner wrote:

> I thought about what my "ideal" design would look like.  I'm not suggesting 
> anyone should actually go off and do this, but since we're brainstorming 
> design anyway, it doesn't hurt to consider what an optimal one should look 
> like (and for the record, there may be things I haven't considered that cuase 
> this design to not work or be suboptimal or not work at all).  Just throwing 
> it out there.
>
> 1. Rename `ObjectFile` to `ModuleFile` to indicate that a) it makes no sense 
> without a Module, and b) it doesn't make sense for symbol-only files.


This is the one I have most problems with, actually, as it goes against my 
"ideal" design. I would actually like to have ObjectFile (or whatever it's 
called) be completely independent of the Module class. The motivation for this 
is, unsurprisingly, lldb-server, which needs to have access to very lightweight 
info about an object file (on the level of: UUID, architecture, maybe entry 
point, etc.), but nothing like the fancy interface the Module class provides. 
So ideally, I'd like for the ObjectFile to live on a completely different 
level, so that it can be used in lldb-server without pulling in Module and 
everything that goes with it. So, I don't agree with your (a) goal here. The 
(b) goal seems completely reasonable though.

> 
> 
> 2. Rename `SymbolVendor` to `SymbolFileLocator`.  Mostly cosmetic, but more 
> clearly differentiates the responsibilities of the two classes.  (e.g. 
> wouldn't you expect SymbolFiles to be able to "vend" symbols)?

+1000. It has always bugged me that SymbolVendor has all these methods that 
don't do anything and just forward their arguments to the SymbolFile (after 
performing a dozen null checks). I think it would be better if the symbol file 
interface was called directly. If some SymbolVendor/SymbolLocator ever wants to 
take an existing SymbolFile and alter its behavior slightly, it can always use 
the composition pattern and return it's own delegating SymbolFile object 
instead.

> 
> 
> 3. `SymbolFileLocator` returns a `unique_ptr`.  Nothing more.  It 
> has no other methods.

That seems to be the logical consequence of the previous item.

> 
> 
> 4. `Target` contains a `vector>` as well as 
> methods `AddSymbolFileLocator(unique_ptr)` and 
> `unique_ptr LocateSymboleFile(Module&)`.  The former gives things 
> like the Platform or Process the ability to customize the behavior.  The 
> latter, when called, will iterate through the list of `SymbolFileLocators`, 
> trying to find one that works.

I am not sure if `Target` is a good receptacle for this functionality, because 
one of lldb's goals (which seems completely reasonable to me) is to be able to 
reuse the same Module instance if the module happens to be loaded in multiple 
targets. If the target is responsible for locating the symbol files, then this 
opens the door for one target to unwittingly alter the symbols of another 
target. One way to get out of this would be to say that targets share a Module 
only if they (independently) choose the same symbol file. This might enable us 
to also solve the current problem, where "target symbols add" executed in one 
debugger, alters the behavior of another debugger instance. Another would be to 
store the list of SymbolLocators in a more global place. This would actually be 
pretty similar to what we have now.

> 
> 
> 5. `Module` contains a `unique_ptr`.  When `GetSymbolFile()` is 
> called for the first time, it calls `Target::LocateSymbolFile(*this)` and 
> updates its member variable.

Nothing to argue about here.

> 
> 
> 6. `ModuleFile` is updated to support "capabilities" much like `SymbolFile` 
> is today.  One of these capabilities can be "has unwind info".  Likewise, 
> "has unwind info is added to `SymbolFile` capabilities as well.  `Module` can 
> then select the best provider of unwind info using this approach.

I don't believe a single bit is enough to describe the suitability of unwind 
info, as the choice of unwind strategy may vary from function to function and 
depend on other circumstances too (e.g., whether we are stopped at a call 
site). Instead, I'd try to have much more granular capabilities. E.g., one of 
those capabilities might be "has function boundaries". Then if either entity 
provides the information about function boundaries, the Module can create an 
InstructionEmulationUnwinder, which uses these boundaries to create emulated 
unwind plans. And even this capability might not be just a "bit", but rather a 
pooling of function boundary information gathered from various sources.

> And finally
> 
> 7. `SymbolFile` now stores a variable `m_module` instead of `m_obj_file` 
> since this is usually what the `SymbolFile` requires anyway, and it also 
> 

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

I thought about what my "ideal" design would look like.  I'm not suggesting 
anyone should actually go off and do this, but since we're brainstorming design 
anyway, it doesn't hurt to consider what an optimal one should look like (and 
for the record, there may be things I haven't considered that cuase this design 
to not work or be suboptimal or not work at all).  Just throwing it out there.

1. Rename `ObjectFile` to `ModuleFile` to indicate that a) it makes no sense 
without a Module, and b) it doesn't make sense for symbol-only files.

2. Rename `SymbolVendor` to `SymbolFileLocator`.  Mostly cosmetic, but more 
clearly differentiates the responsibilities of the two classes.  (e.g. wouldn't 
you expect SymbolFiles to be able to "vend" symbols)?

3. `SymbolFileLocator` returns a `unique_ptr`.  Nothing more.  It 
has no other methods.

4. `Target` contains a `vector>` as well as 
methods `AddSymbolFileLocator(unique_ptr)` and 
`unique_ptr LocateSymboleFile(Module&)`.  The former gives things 
like the Platform or Process the ability to customize the behavior.  The 
latter, when called, will iterate through the list of `SymbolFileLocators`, 
trying to find one that works.

5. `Module` contains a `unique_ptr`.  When `GetSymbolFile()` is 
called for the first time, it calls `Target::LocateSymbolFile(*this)` and 
updates its member variable.

6. `ModuleFile` is updated to support "capabilities" much like `SymbolFile` is 
today.  One of these capabilities can be "has unwind info".  Likewise, "has 
unwind info is added to `SymbolFile` capabilities as well.  `Module` can then 
select the best provider of unwind info using this approach.

And finally

7. `SymbolFile` now stores a variable `m_module` instead of `m_obj_file` since 
this is usually what the `SymbolFile` requires anyway, and it also eliminates 
the dependency on `SymbolFile` being backed by an `ObjectFile`.  Note that in 
this case, a `SymbolFileBreakpad` for example would have its `m_module` 
variable set to the actual module that its providing symbols for.


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via lldb-commits]

On 13/12/2018 23:19, Zachary Turner wrote:
The permanent solution would be figuring out what to do about the 
ObjectFile situation (e.g. do we want to make an ObjectFilePDB or do we 
want to live in a world where SymbolFiles need not be backed by 
ObjectFiles?), and then regardless of what we decide there, implementing 
the SymbolVendor that can search a combination of locations including 
(but not limited to) a symbol server.  lldb already has the notion of a 
symbol path.  So using this we could just use that existing logic to 
search the symbol path, and before you run LLDB make sure your minidump 
.pdb files are in that search path.  If we make an ObjectFilePDB, then 
this should be able to fall out naturally of the existing design with no 
additional work, because it will use the normal SymbolVendor search 
logic.  If we allow SymbolFilePDB to live without an ObjectFilePDB, then 
we would have to make changes similar to what you proposed earlier where 
we can still get a SymbolVendor and use it to execute its default search 
algorithm, plus probably some other changes to make sure various other 
things work correctly in the presence of ObjectFile-less SymbolFiles.




+1 for that.


I'm particularly interested in the result of this, as I'm in the middle 
of adding ObjectFileBreakpad, whose raison d´être is to make things 
interoperate more nicely with the rest of lldb.


My take on this is that going the ObjectFile route is going to be faster 
and less intrusive, but it may produce some odd-looking APIs. E.g. maybe 
the ObjectFilePDB will claim it contains no sections (which is the 
traditional way of passing information from ObjectFiles to SymbolFiles), 
but instead provide some private API to access the underlying llvm data 
structures, which will be used by SymbolFilePDB to do it's thing.


OTOH, object-less symbol files has the potential to produce 
nicer-looking APIs overall, but it will require a lot of changes to core 
lldb to make it happen. E.g., besides the fact that searching for 
"symbols" happens on the object file level (which is the whole reason 
we're having this discussion), the object files are also responsible for 
providing unwind information. That would have to change too.

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

That said, as you mentioned this enables other developers to start working
on things, and if that means we can get the SymbolVendor in more quickly,
then we can get rid of this more quickly.  I just really want to converge
towards the permanent solution, rather than away from it, so if committing
this gets us there quicker then I can get behind it.  I trust your
judgement here :)

On Thu, Dec 13, 2018 at 2:19 PM Zachary Turner  wrote:

> The permanent solution would be figuring out what to do about the
> ObjectFile situation (e.g. do we want to make an ObjectFilePDB or do we
> want to live in a world where SymbolFiles need not be backed by
> ObjectFiles?), and then regardless of what we decide there, implementing
> the SymbolVendor that can search a combination of locations including (but
> not limited to) a symbol server.  lldb already has the notion of a symbol
> path.  So using this we could just use that existing logic to search the
> symbol path, and before you run LLDB make sure your minidump .pdb files are
> in that search path.  If we make an ObjectFilePDB, then this should be able
> to fall out naturally of the existing design with no additional work,
> because it will use the normal SymbolVendor search logic.  If we allow
> SymbolFilePDB to live without an ObjectFilePDB, then we would have to make
> changes similar to what you proposed earlier where we can still get a
> SymbolVendor and use it to execute its default search algorithm, plus
> probably some other changes to make sure various other things work
> correctly in the presence of ObjectFile-less SymbolFiles.
>
> On Thu, Dec 13, 2018 at 1:59 PM Leonard Mosescu 
> wrote:
>
>> Thanks Zach. Don't get me wrong, I have no problem tweaking it as long as
>> necessary assuming we all agree on the plan: we could implement a
>> ObjectFilePDB and a PDB SymbolVendor first, then the contentious PDB lookup
>> detail goes away.
>>
>> My intention was to enable other developers to start consuming PDBs with
>> minidumps, but perhaps I rushed ahead too much. The long discussion clearly
>> suggests that the changes are not ready, so I'll shelve them for now and
>> keep them until the timing is right.
>>
>>
>>> * It makes the behavior dependent on the environment, much like using an
>>> environment variable.  This is a potential source of flakiness in tests, or
>>> different behavior on different peoples' machines.
>>>
>> I mostly agree. Although 1) this matches what LLDB already does for DWARF
>> and 2) I think the issues with flakiness are a bit overblown: there's a lot
>> of stuff which depends on the CWD, and the environment for tests should be
>> predictable in general.
>>
>>
>>> * It doesn't match WinDbg or MSVC
>>>
>> Sure, but neither does looking next to the .dmp file (which is purely a
>> VisualStudio invention - and any IDE can do the same on top of LLDB if they
>> really choose to, but it should not be backed in IMO).
>>
>>
>>> * It's temporary functionality, and temporary functionality more often
>>> than not ends up not being so temporary, thereby contributing to technical
>>> debt.
>>>
>> If we unify the logic with the DWARF SymbolVendor then only the
>> implementation itself it temporary, the lookup logic would not change,
>> right?
>>
>>
>>> * We already know what the permanent solution is, and we're going to
>>> have to implement it anyway, so we could avoid this by just implementing
>>> the permanent solution first
>>>
>> The catch22 is that we can't test anything else involving minidumps +
>> PDBs in the meantime. I found that exercising that combination to be very
>> useful in uncovering other parts which need attention.
>>
>> Also, just a sanity check: what do you think is the permanent solution?
>>
>>
>> On Thu, Dec 13, 2018 at 1:44 PM Zachary Turner 
>> wrote:
>>
>>> At this point it seems like perpetuating the hack, or at least even if
>>> that's the direction we decide to go longterm, not implementing that
>>> solution fully and missing some of the corner cases.
>>>
>>> So I think I'd rather just go with the original hack of checking the
>>> current directory at this point.  Still though, I want to make sure that
>>> we're on the same page that in the future if we're going to need more hacks
>>> of some kind to delay implementing a proper solution, that we shelve the
>>> work until the proper solution is implemented and tested.  It may not be
>>> the best thing to do in the short term, but it is in the long term, which
>>> is what i'm trying to optimize for.
>>>
>>> On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 clayborg requested changes to this revision.
 clayborg added a comment.
 This revision now requires changes to proceed.

 Just need a way to verify symbols are good. See my inline comment.



 
 Comment at: source/Commands/CommandObjectTarget.cpp:4246
if (symbol_file) {
 -ObjectFile 

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

The permanent solution would be figuring out what to do about the
ObjectFile situation (e.g. do we want to make an ObjectFilePDB or do we
want to live in a world where SymbolFiles need not be backed by
ObjectFiles?), and then regardless of what we decide there, implementing
the SymbolVendor that can search a combination of locations including (but
not limited to) a symbol server.  lldb already has the notion of a symbol
path.  So using this we could just use that existing logic to search the
symbol path, and before you run LLDB make sure your minidump .pdb files are
in that search path.  If we make an ObjectFilePDB, then this should be able
to fall out naturally of the existing design with no additional work,
because it will use the normal SymbolVendor search logic.  If we allow
SymbolFilePDB to live without an ObjectFilePDB, then we would have to make
changes similar to what you proposed earlier where we can still get a
SymbolVendor and use it to execute its default search algorithm, plus
probably some other changes to make sure various other things work
correctly in the presence of ObjectFile-less SymbolFiles.

On Thu, Dec 13, 2018 at 1:59 PM Leonard Mosescu  wrote:

> Thanks Zach. Don't get me wrong, I have no problem tweaking it as long as
> necessary assuming we all agree on the plan: we could implement a
> ObjectFilePDB and a PDB SymbolVendor first, then the contentious PDB lookup
> detail goes away.
>
> My intention was to enable other developers to start consuming PDBs with
> minidumps, but perhaps I rushed ahead too much. The long discussion clearly
> suggests that the changes are not ready, so I'll shelve them for now and
> keep them until the timing is right.
>
>
>> * It makes the behavior dependent on the environment, much like using an
>> environment variable.  This is a potential source of flakiness in tests, or
>> different behavior on different peoples' machines.
>>
> I mostly agree. Although 1) this matches what LLDB already does for DWARF
> and 2) I think the issues with flakiness are a bit overblown: there's a lot
> of stuff which depends on the CWD, and the environment for tests should be
> predictable in general.
>
>
>> * It doesn't match WinDbg or MSVC
>>
> Sure, but neither does looking next to the .dmp file (which is purely a
> VisualStudio invention - and any IDE can do the same on top of LLDB if they
> really choose to, but it should not be backed in IMO).
>
>
>> * It's temporary functionality, and temporary functionality more often
>> than not ends up not being so temporary, thereby contributing to technical
>> debt.
>>
> If we unify the logic with the DWARF SymbolVendor then only the
> implementation itself it temporary, the lookup logic would not change,
> right?
>
>
>> * We already know what the permanent solution is, and we're going to have
>> to implement it anyway, so we could avoid this by just implementing the
>> permanent solution first
>>
> The catch22 is that we can't test anything else involving minidumps + PDBs
> in the meantime. I found that exercising that combination to be very useful
> in uncovering other parts which need attention.
>
> Also, just a sanity check: what do you think is the permanent solution?
>
>
> On Thu, Dec 13, 2018 at 1:44 PM Zachary Turner  wrote:
>
>> At this point it seems like perpetuating the hack, or at least even if
>> that's the direction we decide to go longterm, not implementing that
>> solution fully and missing some of the corner cases.
>>
>> So I think I'd rather just go with the original hack of checking the
>> current directory at this point.  Still though, I want to make sure that
>> we're on the same page that in the future if we're going to need more hacks
>> of some kind to delay implementing a proper solution, that we shelve the
>> work until the proper solution is implemented and tested.  It may not be
>> the best thing to do in the short term, but it is in the long term, which
>> is what i'm trying to optimize for.
>>
>> On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> clayborg requested changes to this revision.
>>> clayborg added a comment.
>>> This revision now requires changes to proceed.
>>>
>>> Just need a way to verify symbols are good. See my inline comment.
>>>
>>>
>>>
>>> 
>>> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>>>if (symbol_file) {
>>> -ObjectFile *object_file = symbol_file->GetObjectFile();
>>>
>>> 
>>> labath wrote:
>>> > lemo wrote:
>>> > > note I had to bypass this check: we don't (yet) have a ObjectFilePDB
>>> so the SymbolFileNativePDB always points to the associated PE binary.
>>> > >
>>> > > the check itself seems valuable as a diagnostic but not strictly
>>> required. Should I add a TODO comment and/or open a bug to revisit this?
>>> > I not sure this is a good idea. Isn't this the only way of providing
>>> feedback about whether the symbols were actually added? If we 

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

Thanks Zach. Don't get me wrong, I have no problem tweaking it as long as
necessary assuming we all agree on the plan: we could implement a
ObjectFilePDB and a PDB SymbolVendor first, then the contentious PDB lookup
detail goes away.

My intention was to enable other developers to start consuming PDBs with
minidumps, but perhaps I rushed ahead too much. The long discussion clearly
suggests that the changes are not ready, so I'll shelve them for now and
keep them until the timing is right.


> * It makes the behavior dependent on the environment, much like using an
> environment variable.  This is a potential source of flakiness in tests, or
> different behavior on different peoples' machines.
>
I mostly agree. Although 1) this matches what LLDB already does for DWARF
and 2) I think the issues with flakiness are a bit overblown: there's a lot
of stuff which depends on the CWD, and the environment for tests should be
predictable in general.


> * It doesn't match WinDbg or MSVC
>
Sure, but neither does looking next to the .dmp file (which is purely a
VisualStudio invention - and any IDE can do the same on top of LLDB if they
really choose to, but it should not be backed in IMO).


> * It's temporary functionality, and temporary functionality more often
> than not ends up not being so temporary, thereby contributing to technical
> debt.
>
If we unify the logic with the DWARF SymbolVendor then only the
implementation itself it temporary, the lookup logic would not change,
right?


> * We already know what the permanent solution is, and we're going to have
> to implement it anyway, so we could avoid this by just implementing the
> permanent solution first
>
The catch22 is that we can't test anything else involving minidumps + PDBs
in the meantime. I found that exercising that combination to be very useful
in uncovering other parts which need attention.

Also, just a sanity check: what do you think is the permanent solution?


On Thu, Dec 13, 2018 at 1:44 PM Zachary Turner  wrote:

> At this point it seems like perpetuating the hack, or at least even if
> that's the direction we decide to go longterm, not implementing that
> solution fully and missing some of the corner cases.
>
> So I think I'd rather just go with the original hack of checking the
> current directory at this point.  Still though, I want to make sure that
> we're on the same page that in the future if we're going to need more hacks
> of some kind to delay implementing a proper solution, that we shelve the
> work until the proper solution is implemented and tested.  It may not be
> the best thing to do in the short term, but it is in the long term, which
> is what i'm trying to optimize for.
>
> On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> clayborg requested changes to this revision.
>> clayborg added a comment.
>> This revision now requires changes to proceed.
>>
>> Just need a way to verify symbols are good. See my inline comment.
>>
>>
>>
>> 
>> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>>if (symbol_file) {
>> -ObjectFile *object_file = symbol_file->GetObjectFile();
>>
>> 
>> labath wrote:
>> > lemo wrote:
>> > > note I had to bypass this check: we don't (yet) have a ObjectFilePDB
>> so the SymbolFileNativePDB always points to the associated PE binary.
>> > >
>> > > the check itself seems valuable as a diagnostic but not strictly
>> required. Should I add a TODO comment and/or open a bug to revisit this?
>> > I not sure this is a good idea. Isn't this the only way of providing
>> feedback about whether the symbols were actually added? If we are unable to
>> load the symbol file specified (perhaps because the user made a typo, or
>> the file is corrupted), then the symbol vendor will just create a default
>> SymbolFile backed by the original object file. Doesn't that mean this will
>> basically always return true now?
>> >
>> > I think this is strictly worse that the previous solution as it lets
>> the objectless-symbol-file hack leak out of SymbolFilePDB.
>> We need to add some sanity check where we ask the symbol file if it is
>> valid. It should be virtual function in SymbolFile that defaults to:
>>
>> ```
>> virtual bool SymbolFile::IsValid() const {
>>   return GetObjectFile() != nullptr;
>> }
>> ```
>> And we can override for SymbolFile subclasses that arenb't objfile based.
>> How does this sound?
>>
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D55142/new/
>>
>> https://reviews.llvm.org/D55142
>>
>>
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

At this point it seems like perpetuating the hack, or at least even if
that's the direction we decide to go longterm, not implementing that
solution fully and missing some of the corner cases.

So I think I'd rather just go with the original hack of checking the
current directory at this point.  Still though, I want to make sure that
we're on the same page that in the future if we're going to need more hacks
of some kind to delay implementing a proper solution, that we shelve the
work until the proper solution is implemented and tested.  It may not be
the best thing to do in the short term, but it is in the long term, which
is what i'm trying to optimize for.

On Thu, Dec 13, 2018 at 1:39 PM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg requested changes to this revision.
> clayborg added a comment.
> This revision now requires changes to proceed.
>
> Just need a way to verify symbols are good. See my inline comment.
>
>
>
> 
> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>if (symbol_file) {
> -ObjectFile *object_file = symbol_file->GetObjectFile();
>
> 
> labath wrote:
> > lemo wrote:
> > > note I had to bypass this check: we don't (yet) have a ObjectFilePDB
> so the SymbolFileNativePDB always points to the associated PE binary.
> > >
> > > the check itself seems valuable as a diagnostic but not strictly
> required. Should I add a TODO comment and/or open a bug to revisit this?
> > I not sure this is a good idea. Isn't this the only way of providing
> feedback about whether the symbols were actually added? If we are unable to
> load the symbol file specified (perhaps because the user made a typo, or
> the file is corrupted), then the symbol vendor will just create a default
> SymbolFile backed by the original object file. Doesn't that mean this will
> basically always return true now?
> >
> > I think this is strictly worse that the previous solution as it lets the
> objectless-symbol-file hack leak out of SymbolFilePDB.
> We need to add some sanity check where we ask the symbol file if it is
> valid. It should be virtual function in SymbolFile that defaults to:
>
> ```
> virtual bool SymbolFile::IsValid() const {
>   return GetObjectFile() != nullptr;
> }
> ```
> And we can override for SymbolFile subclasses that arenb't objfile based.
> How does this sound?
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just need a way to verify symbols are good. See my inline comment.




Comment at: source/Commands/CommandObjectTarget.cpp:4246
   if (symbol_file) {
-ObjectFile *object_file = symbol_file->GetObjectFile();
 

labath wrote:
> lemo wrote:
> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the 
> > SymbolFileNativePDB always points to the associated PE binary. 
> > 
> > the check itself seems valuable as a diagnostic but not strictly required. 
> > Should I add a TODO comment and/or open a bug to revisit this?
> I not sure this is a good idea. Isn't this the only way of providing feedback 
> about whether the symbols were actually added? If we are unable to load the 
> symbol file specified (perhaps because the user made a typo, or the file is 
> corrupted), then the symbol vendor will just create a default SymbolFile 
> backed by the original object file. Doesn't that mean this will basically 
> always return true now?
> 
> I think this is strictly worse that the previous solution as it lets the 
> objectless-symbol-file hack leak out of SymbolFilePDB.
We need to add some sanity check where we ask the symbol file if it is valid. 
It should be virtual function in SymbolFile that defaults to:

```
virtual bool SymbolFile::IsValid() const {
  return GetObjectFile() != nullptr;
}
```
And we can override for SymbolFile subclasses that arenb't objfile based. How 
does this sound?


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

The problems I have with current directory lookup are:

* It makes the behavior dependent on the environment, much like using an
environment variable.  This is a potential source of flakiness in tests, or
different behavior on different peoples' machines.
* It doesn't match WinDbg or MSVC
* It's temporary functionality, and temporary functionality more often than
not ends up not being so temporary, thereby contributing to technical debt.
* We already know what the permanent solution is, and we're going to have
to implement it anyway, so we could avoid this by just implementing the
permanent solution first.

Anyway, I don't want to hold this up any longer, but in the future let's
just implement the permanent solutions first so that we can avoid this type
of unnecessary blockage.

On Thu, Dec 13, 2018 at 1:10 PM Leonard Mosescu  wrote:

> I think we can fix that by changing the line to:
>>
>> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
>> }
>>
>
> The problem is that SymbolFileNativePDB returns the module object file as
> it's own object file. Are you suggesting we should track the module object
> file separate from SymbolFile::m_obj_file? In a way that would be a more
> accurate representation of what's going on (we don't have an ObjectFilePDB
> yet) - but the check is still a bit nonsensical (using the lack of an
> object file to indicate that the operation succeeded)
>
> I agree that the case that Pavel pointed out is an issue, although the
> non-nonsensical response only happens in the unlikely case you already have
> symbols (so there's little reason to explicitly re-add symbols). It's also
> not happening with PDBs (which is why it seemed safe).
>
> At this point I have to ask: what is the problem with the current
> directory lookup again? I seems to me that we complicated ourselves for no
> good reason (using a questionable functionality in a high level Windows IDE
> as reference).
>
>
> On Thu, Dec 13, 2018 at 12:23 PM Zachary Turner 
> wrote:
>
>> I think we can fix that by changing the line to:
>>
>> ```
>> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
>> }
>> ```
>>
>> On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath  wrote:
>>
>>> On 13/12/2018 19:32, Leonard Mosescu wrote:
>>> > What's the consensus?
>>> >
>>> > Personally I think that, even considering the potential issue that
>>> Paval
>>> > pointed out, the "target symbols add ..." is the most conservative
>>> > approach in terms of introducing new behavior. I'm fine with the
>>> current
>>> > directory lookup as well (the original change) since it's consistent
>>> > with DWARF lookup.
>>>
>>>
>>> Yes, but it also regresses existing functionality. Now if I do something
>>> completely nonsensical like:
>>> (lldb) target create "/bin/ls"
>>> Current executable set to '/bin/ls' (x86_64).
>>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>>> error: symbol file '/tmp/a.txt' does not match any existing module
>>>
>>> lldb will print a nice error for me. If I remove the safeguards like you
>>> did in your patch, it turns into this:
>>> (lldb) target create "/bin/ls"
>>> Current executable set to '/bin/ls' (x86_64).
>>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>>> symbol file '/tmp/a.txt' has been added to '/bin/ls'
>>>
>>> which is a blatant lie, because /bin/ls will continue to use symbols
>>> from the object file.
>>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

>
> I think we can fix that by changing the line to:
>
> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
> }
>

The problem is that SymbolFileNativePDB returns the module object file as
it's own object file. Are you suggesting we should track the module object
file separate from SymbolFile::m_obj_file? In a way that would be a more
accurate representation of what's going on (we don't have an ObjectFilePDB
yet) - but the check is still a bit nonsensical (using the lack of an
object file to indicate that the operation succeeded)

I agree that the case that Pavel pointed out is an issue, although the
non-nonsensical response only happens in the unlikely case you already have
symbols (so there's little reason to explicitly re-add symbols). It's also
not happening with PDBs (which is why it seemed safe).

At this point I have to ask: what is the problem with the current directory
lookup again? I seems to me that we complicated ourselves for no good
reason (using a questionable functionality in a high level Windows IDE as
reference).


On Thu, Dec 13, 2018 at 12:23 PM Zachary Turner  wrote:

> I think we can fix that by changing the line to:
>
> ```
> if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
> }
> ```
>
> On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath  wrote:
>
>> On 13/12/2018 19:32, Leonard Mosescu wrote:
>> > What's the consensus?
>> >
>> > Personally I think that, even considering the potential issue that
>> Paval
>> > pointed out, the "target symbols add ..." is the most conservative
>> > approach in terms of introducing new behavior. I'm fine with the
>> current
>> > directory lookup as well (the original change) since it's consistent
>> > with DWARF lookup.
>>
>>
>> Yes, but it also regresses existing functionality. Now if I do something
>> completely nonsensical like:
>> (lldb) target create "/bin/ls"
>> Current executable set to '/bin/ls' (x86_64).
>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>> error: symbol file '/tmp/a.txt' does not match any existing module
>>
>> lldb will print a nice error for me. If I remove the safeguards like you
>> did in your patch, it turns into this:
>> (lldb) target create "/bin/ls"
>> Current executable set to '/bin/ls' (x86_64).
>> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
>> symbol file '/tmp/a.txt' has been added to '/bin/ls'
>>
>> which is a blatant lie, because /bin/ls will continue to use symbols
>> from the object file.
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

I think we can fix that by changing the line to:

```
if (!object_file || object_file->GetFileSpec() == symbol_fspec) {
}
```

On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath  wrote:

> On 13/12/2018 19:32, Leonard Mosescu wrote:
> > What's the consensus?
> >
> > Personally I think that, even considering the potential issue that Paval
> > pointed out, the "target symbols add ..." is the most conservative
> > approach in terms of introducing new behavior. I'm fine with the current
> > directory lookup as well (the original change) since it's consistent
> > with DWARF lookup.
>
>
> Yes, but it also regresses existing functionality. Now if I do something
> completely nonsensical like:
> (lldb) target create "/bin/ls"
> Current executable set to '/bin/ls' (x86_64).
> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
> error: symbol file '/tmp/a.txt' does not match any existing module
>
> lldb will print a nice error for me. If I remove the safeguards like you
> did in your patch, it turns into this:
> (lldb) target create "/bin/ls"
> Current executable set to '/bin/ls' (x86_64).
> (lldb) target symbols add  -s /bin/ls /tmp/a.txt
> symbol file '/tmp/a.txt' has been added to '/bin/ls'
>
> which is a blatant lie, because /bin/ls will continue to use symbols
> from the object file.
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via lldb-commits]

On 13/12/2018 19:32, Leonard Mosescu wrote:

What's the consensus?

Personally I think that, even considering the potential issue that Paval 
pointed out, the "target symbols add ..." is the most conservative 
approach in terms of introducing new behavior. I'm fine with the current 
directory lookup as well (the original change) since it's consistent 
with DWARF lookup.



Yes, but it also regresses existing functionality. Now if I do something 
completely nonsensical like:

(lldb) target create "/bin/ls"
Current executable set to '/bin/ls' (x86_64).
(lldb) target symbols add  -s /bin/ls /tmp/a.txt
error: symbol file '/tmp/a.txt' does not match any existing module

lldb will print a nice error for me. If I remove the safeguards like you 
did in your patch, it turns into this:

(lldb) target create "/bin/ls"
Current executable set to '/bin/ls' (x86_64).
(lldb) target symbols add  -s /bin/ls /tmp/a.txt
symbol file '/tmp/a.txt' has been added to '/bin/ls'

which is a blatant lie, because /bin/ls will continue to use symbols 
from the object file.

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

Well, Visual Studio also supports remote debugging, and searching next to
the .dmp is just one of several places it looks.  And LLDB also supports
local debugging, and so looking next to the .dmp file, being consistent
with Visual Studio, will be the expected behavior for people using LLDB in
local debugging scenarios, which is one of the supported use cases.

That said, I'm also fine with the latest patch.

On Thu, Dec 13, 2018 at 10:32 AM Leonard Mosescu  wrote:

> What's the consensus?
>
> Personally I think that, even considering the potential issue that Paval
> pointed out, the "target symbols add ..." is the most conservative approach
> in terms of introducing new behavior. I'm fine with the current directory
> lookup as well (the original change) since it's consistent with DWARF
> lookup.
> (the only choice I'm less excited about is the implicit lookup next to the
> .dmp or .dll/.exe - it's highly specific to a local debugging scenario
> which may be appropriate for something like an IDE, ex. VisualStudio but
> not for a general purpose debugger)
>
> So my preference is for the latest patch - what does everyone else think?
>
> On Thu, Dec 13, 2018 at 4:03 AM Zachary Turner  wrote:
>
>> On irc earlier i was talking about this with Greg and he said it should
>> be fine in his opinion. I’ll point him to this review in the morning so he
>> can comment
>> On Thu, Dec 13, 2018 at 3:30 AM Pavel Labath via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> labath added inline comments.
>>>
>>>
>>> 
>>> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>>>if (symbol_file) {
>>> -ObjectFile *object_file = symbol_file->GetObjectFile();
>>>
>>> 
>>> lemo wrote:
>>> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB
>>> so the SymbolFileNativePDB always points to the associated PE binary.
>>> >
>>> > the check itself seems valuable as a diagnostic but not strictly
>>> required. Should I add a TODO comment and/or open a bug to revisit this?
>>> I not sure this is a good idea. Isn't this the only way of providing
>>> feedback about whether the symbols were actually added? If we are unable to
>>> load the symbol file specified (perhaps because the user made a typo, or
>>> the file is corrupted), then the symbol vendor will just create a default
>>> SymbolFile backed by the original object file. Doesn't that mean this will
>>> basically always return true now?
>>>
>>> I think this is strictly worse that the previous solution as it lets the
>>> objectless-symbol-file hack leak out of SymbolFilePDB.
>>>
>>>
>>> CHANGES SINCE LAST ACTION
>>>   https://reviews.llvm.org/D55142/new/
>>>
>>> https://reviews.llvm.org/D55142
>>>
>>>
>>>
>>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

What's the consensus?

Personally I think that, even considering the potential issue that Paval
pointed out, the "target symbols add ..." is the most conservative approach
in terms of introducing new behavior. I'm fine with the current directory
lookup as well (the original change) since it's consistent with DWARF
lookup.
(the only choice I'm less excited about is the implicit lookup next to the
.dmp or .dll/.exe - it's highly specific to a local debugging scenario
which may be appropriate for something like an IDE, ex. VisualStudio but
not for a general purpose debugger)

So my preference is for the latest patch - what does everyone else think?

On Thu, Dec 13, 2018 at 4:03 AM Zachary Turner  wrote:

> On irc earlier i was talking about this with Greg and he said it should be
> fine in his opinion. I’ll point him to this review in the morning so he can
> comment
> On Thu, Dec 13, 2018 at 3:30 AM Pavel Labath via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> labath added inline comments.
>>
>>
>> 
>> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>>if (symbol_file) {
>> -ObjectFile *object_file = symbol_file->GetObjectFile();
>>
>> 
>> lemo wrote:
>> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB so
>> the SymbolFileNativePDB always points to the associated PE binary.
>> >
>> > the check itself seems valuable as a diagnostic but not strictly
>> required. Should I add a TODO comment and/or open a bug to revisit this?
>> I not sure this is a good idea. Isn't this the only way of providing
>> feedback about whether the symbols were actually added? If we are unable to
>> load the symbol file specified (perhaps because the user made a typo, or
>> the file is corrupted), then the symbol vendor will just create a default
>> SymbolFile backed by the original object file. Doesn't that mean this will
>> basically always return true now?
>>
>> I think this is strictly worse that the previous solution as it lets the
>> objectless-symbol-file hack leak out of SymbolFilePDB.
>>
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D55142/new/
>>
>> https://reviews.llvm.org/D55142
>>
>>
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

On irc earlier i was talking about this with Greg and he said it should be
fine in his opinion. I’ll point him to this review in the morning so he can
comment
On Thu, Dec 13, 2018 at 3:30 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added inline comments.
>
>
> 
> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>if (symbol_file) {
> -ObjectFile *object_file = symbol_file->GetObjectFile();
>
> 
> lemo wrote:
> > note I had to bypass this check: we don't (yet) have a ObjectFilePDB so
> the SymbolFileNativePDB always points to the associated PE binary.
> >
> > the check itself seems valuable as a diagnostic but not strictly
> required. Should I add a TODO comment and/or open a bug to revisit this?
> I not sure this is a good idea. Isn't this the only way of providing
> feedback about whether the symbols were actually added? If we are unable to
> load the symbol file specified (perhaps because the user made a typo, or
> the file is corrupted), then the symbol vendor will just create a default
> SymbolFile backed by the original object file. Doesn't that mean this will
> basically always return true now?
>
> I think this is strictly worse that the previous solution as it lets the
> objectless-symbol-file hack leak out of SymbolFilePDB.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Commands/CommandObjectTarget.cpp:4246
   if (symbol_file) {
-ObjectFile *object_file = symbol_file->GetObjectFile();
 

lemo wrote:
> note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the 
> SymbolFileNativePDB always points to the associated PE binary. 
> 
> the check itself seems valuable as a diagnostic but not strictly required. 
> Should I add a TODO comment and/or open a bug to revisit this?
I not sure this is a good idea. Isn't this the only way of providing feedback 
about whether the symbols were actually added? If we are unable to load the 
symbol file specified (perhaps because the user made a typo, or the file is 
corrupted), then the symbol vendor will just create a default SymbolFile backed 
by the original object file. Doesn't that mean this will basically always 
return true now?

I think this is strictly worse that the previous solution as it lets the 
objectless-symbol-file hack leak out of SymbolFilePDB.


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

I ended up implementing the support for "target symbols add" since it's
something we needed anyway. This allowed the removal of the contentious
implicit search in the current directory.

I tried to verify this behavior, but it seems like it should already work
> out of the box?   So we're on the same page, we already do have a real
> SymbolVendor implementation, it just happens to be the *default*
> SymbolVendor implementation.  It's not the case that one doesn't exist at
> all.


There were a few missing pieces, although you're right, it works with the
default SymbolVendor as you pointed out (btw, what I meant by "real"
SymbolVendor is a "PDB specific" SymbolVendor, sorry for the confusion)

There is another option which I was just made aware of.  LLDB already has a
> setting called `target.debug-file-search-paths`.  This is basically a
> symbol path.  If you call Symbols::LocateExecutableSymbolFile, it will
> already add use this setting, and moreover it will implicitly add current
> working directory to this path.
> So, if you want this behavior in a supported way that isn't temporary, we
> should move the code for findMatchingPDBFile() out of SymbolFilePDB and
> into this function.  Then everyone is happy I think.
>

As far as I can tell, Symbols::LocateExecutableSymbolFile() is a helper
intended for specialized SymbolVendors. So yes, when we get around to build
a specialized SymbolVendorPDB we'd be able to use it but until then I don't
think that moving that logic inside SymbolFileNativePDB is appropriate.

On Wed, Dec 12, 2018 at 1:19 PM Leonard Mosescu via Phabricator <
revi...@reviews.llvm.org> wrote:

> lemo marked an inline comment as done.
> lemo added inline comments.
>
>
> 
> Comment at: source/Commands/CommandObjectTarget.cpp:4246
>if (symbol_file) {
> -ObjectFile *object_file = symbol_file->GetObjectFile();
>
> 
> note I had to bypass this check: we don't (yet) have a ObjectFilePDB so
> the SymbolFileNativePDB always points to the associated PE binary.
>
> the check itself seems valuable as a diagnostic but not strictly required.
> Should I add a TODO comment and/or open a bug to revisit this?
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked an inline comment as done.
lemo added inline comments.



Comment at: source/Commands/CommandObjectTarget.cpp:4246
   if (symbol_file) {
-ObjectFile *object_file = symbol_file->GetObjectFile();
 

note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the 
SymbolFileNativePDB always points to the associated PE binary. 

the check itself seems valuable as a diagnostic but not strictly required. 
Should I add a TODO comment and/or open a bug to revisit this?


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 177898.

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

https://reviews.llvm.org/D55142

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Commands/CommandObjectTarget.cpp
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,57 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile& obj_file,
+llvm::BumpPtrAllocator ) {
   using namespace llvm::object;
-  auto expected_binary = createBinary(exe_path);
 
-  // If the file isn't a PE/COFF executable, fail.
-  if (!expected_binary) {
-llvm::consumeError(expected_binary.takeError());
-return nullptr;
-  }
-  OwningBinary binary = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  const auto module = obj_file.GetModule();
+  const auto symbol_file_filespec = module->GetSymbolFileFileSpec();
+  if (symbol_file_filespec.IsResolved()) {
+// If we have the symbol file filespec set, use it
+pdb_file = symbol_file_filespec.GetPath();
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file.GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(guid) + 4) // CvRecordPdb70
+  return nullptr;
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  } else {
+// Use the PDB path embedded in the binary
+auto expected_binary = createBinary(obj_file.GetFileSpec().GetPath());
+if (!expected_binary) {
+  llvm::consumeError(expected_binary.takeError());
+  return nullptr;
+}
 
-  // If it doesn't have a debug directory, fail.
-  llvm::StringRef pdb_file;
-  auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
-  if (ec)
-return nullptr;
+OwningBinary binary = std::move(*expected_binary);
+
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
+  // If it doesn't have a debug directory, fail.
+  return nullptr;
+}
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  }
 
   // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
   // fail.
   llvm::file_magic magic;
-  ec = llvm::identify_magic(pdb_file, magic);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +165,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +546,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+loadMatchingPDBFile(*m_obj_file, m_allocator);
 
 if (!file_up) {
   auto module_sp = m_obj_file->GetModule();
@@ -1459,11 +1484,10 @@
 llvm::Optional modi = m_index->GetModuleIndexForVa(file_addr);
 if (!modi)
   return 0;
-CompilandIndexItem *cci = m_index->compilands().GetCompiland(*modi);
-if (!cci)
+if 

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

There is another option which I was just made aware of.  LLDB already has a
setting called `target.debug-file-search-paths`.  This is basically a
symbol path.  If you call Symbols::LocateExecutableSymbolFile, it will
already add use this setting, and moreover it will implicitly add current
working directory to this path.

So, if you want this behavior in a supported way that isn't temporary, we
should move the code for findMatchingPDBFile() out of SymbolFilePDB and
into this function.  Then everyone is happy I think.

What do you think?

On Tue, Dec 11, 2018 at 4:42 PM Zachary Turner  wrote:

> On Tue, Dec 11, 2018 at 4:22 PM Leonard Mosescu 
> wrote:
>
>> I guess I don't see why we need a temporary solution at all.  If we can
>>> have logic that can be rolled into the SymbolVendor when we get it, and
>>> makes sense there, and is also simple, why not go with it?  Failing that,
>>> doesn't the `target symbols add` solution also work fine?
>>>
>>
>> I just checked again, and "target symbols add" depends on having a real
>> SymbolVendor implementation. So unfortunately we still need a temporary
>> solution.
>>
>
> I tried to verify this behavior, but it seems like it should already work
> out of the box?   So we're on the same page, we already do have a real
> SymbolVendor implementation, it just happens to be the *default*
> SymbolVendor implementation.  It's not the case that one doesn't exist at
> all.
>
> So anyway, when I run "target symbols add foo.exe path/to/foo.pdb"
> internally what this does is set a member variable called m_symfile_spec on
> the Module object.  Then, it calls our normal CalculateAbilities() function
> which calls loadMatchingPDBFile.
>
> I think all that needs to happen is that we need to check this field.  If
> it's empty, try to load a matching PDB file.  If it's set to something,
> then load that file instead.
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

On Tue, Dec 11, 2018 at 4:22 PM Leonard Mosescu  wrote:

> I guess I don't see why we need a temporary solution at all.  If we can
>> have logic that can be rolled into the SymbolVendor when we get it, and
>> makes sense there, and is also simple, why not go with it?  Failing that,
>> doesn't the `target symbols add` solution also work fine?
>>
>
> I just checked again, and "target symbols add" depends on having a real
> SymbolVendor implementation. So unfortunately we still need a temporary
> solution.
>

I tried to verify this behavior, but it seems like it should already work
out of the box?   So we're on the same page, we already do have a real
SymbolVendor implementation, it just happens to be the *default*
SymbolVendor implementation.  It's not the case that one doesn't exist at
all.

So anyway, when I run "target symbols add foo.exe path/to/foo.pdb"
internally what this does is set a member variable called m_symfile_spec on
the Module object.  Then, it calls our normal CalculateAbilities() function
which calls loadMatchingPDBFile.

I think all that needs to happen is that we need to check this field.  If
it's empty, try to load a matching PDB file.  If it's set to something,
then load that file instead.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

>
> I guess I don't see why we need a temporary solution at all.  If we can
> have logic that can be rolled into the SymbolVendor when we get it, and
> makes sense there, and is also simple, why not go with it?  Failing that,
> doesn't the `target symbols add` solution also work fine?
>

I just checked again, and "target symbols add" depends on having a real
SymbolVendor implementation. So unfortunately we still need a temporary
solution.

So far we're left with:
1) look in the current directory
2) look in the same directory as the .dmp file (we can't use the .exe/.dll
locations since we don't have them in this case)

I know we had a bit of back and forth about this. After revisiting the code
(and for what is worth taking Visual Studio and windbg behavior in
consideration), I still think that the current directory is the best
choice. It's consistent with what LLDB already does for DWARF, so it's not
adding another magic lookup.

On Tue, Dec 11, 2018 at 3:41 PM Adrian McCarthy  wrote:

> >  But here, we're talking about a situation where there is no EXE, only
> a minidump.  If there is a minidump and no EXE then neither WinDbg nor VS
> will search the minidump folder for the PDB.
>
> For the record, the experiments do not bear this out.  VS will indeed
> search in the minidump folder for the PDB.  Unfortunately, a lot of this
> conversation was taken offline.
>
> On Tue, Dec 11, 2018 at 3:30 PM Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> zturner added a comment.
>>
>> In D55142#1326247 , @lemo wrote:
>>
>> > > How large is the PDB file here?
>> >
>> > ~100kb
>>
>>
>> We have a couple of tests in LLVM where PDB files are checked in, but
>> they are very few.  We cannot explode the repo with large numbers of binary
>> files.  So this is probably fine, but if this becomes a pattern, we will
>> need to come up with a different solution.
>>
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D55142/new/
>>
>> https://reviews.llvm.org/D55142
>>
>>
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Adrian McCarthy via lldb-commits]

>  But here, we're talking about a situation where there is no EXE, only a
minidump.  If there is a minidump and no EXE then neither WinDbg nor VS
will search the minidump folder for the PDB.

For the record, the experiments do not bear this out.  VS will indeed
search in the minidump folder for the PDB.  Unfortunately, a lot of this
conversation was taken offline.

On Tue, Dec 11, 2018 at 3:30 PM Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added a comment.
>
> In D55142#1326247 , @lemo wrote:
>
> > > How large is the PDB file here?
> >
> > ~100kb
>
>
> We have a couple of tests in LLVM where PDB files are checked in, but they
> are very few.  We cannot explode the repo with large numbers of binary
> files.  So this is probably fine, but if this becomes a pattern, we will
> need to come up with a different solution.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In D55142#1326247 , @lemo wrote:

> > How large is the PDB file here?
>
> ~100kb


We have a couple of tests in LLVM where PDB files are checked in, but they are 
very few.  We cannot explode the repo with large numbers of binary files.  So 
this is probably fine, but if this becomes a pattern, we will need to come up 
with a different solution.


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

I guess I don't see why we need a temporary solution at all.  If we can
have logic that can be rolled into the SymbolVendor when we get it, and
makes sense there, and is also simple, why not go with it?  Failing that,
doesn't the `target symbols add` solution also work fine?

On Tue, Dec 11, 2018 at 3:24 PM Leonard Mosescu  wrote:

> But here, we're talking about a situation where there is no EXE, only a
>> minidump.  If there is a minidump and no EXE then neither WinDbg nor VS
>> will search the minidump folder for the PDB.
>
>
> Indeed, this is key part.
>
>
>>  In my mind, the algorithm could be something like: ...
>>
>
> I'm not a big fan of this kind of magic. VS sometime does it, and while
> it's nice in the 80% of the cases I found it hard to troubleshoot when
> something goes wrong. For what it's worth, recent versions of VC++ debugger
> are more aligned with ntsd/cdb/windbg.
>
> Anyway, for this patch I think the question is: what's the simplest
> temporary solution? All of these ideas are interesting but more appropriate
> to evaluate when we start designing the PDB SymbolVendor.
>
>
>
> On Tue, Dec 11, 2018 at 3:05 PM Zachary Turner  wrote:
>
>> Only one way to know for sure, and that's to test it :)  So I did.
>>
>> Yes, it will search the directory of the EXE for the PDB.  But here,
>> we're talking about a situation where there is no EXE, only a minidump.  If
>> there is a minidump and no EXE then neither WinDbg nor VS will search the
>> minidump folder for the PDB.  Personally I think it should and I wouldn't
>> mind if that were a documented and tested feature of LLDB, because since it
>> already is established behavior to search the minidump folder for the EXE,
>> it doesn't seem hacky at all for it to also search that location for the
>> PDB.  In my mind, the algorithm could be something like:
>>
>> ```
>> if (ExeLocation = FindExecutableInMinidumpFolder()) {
>>   PdbLocation = FindPdbFromExecutable(ExeLocation)
>> } else {
>>   PdbLocation = FindPdbInMinidumpFolder();
>>   if (!PdbLocation)
>> PdbLocation = FindPdbInSympath();
>> }
>> ```
>>
>> and that would be pretty logical and not code that we would have to
>> consider temporary.
>>
>> My main pushback here is that it's harder to remove code than it is to
>> add it, because once you add it, someone is going to depend on it and
>> complain when you try to remove it.  So if we can just establish some
>> behavior that satisfies the use case while not being temporary, I would
>> prefer to do it.
>>
>> Another option I can think of is to just run `target symbols add -s
>> foo.exe foo.pdb`.  I think we would need to have SymbolFileNativePDB check
>> the value of this setting, but using this workflow, you could just put a
>> .lldbinit file next to lldb.exe that runs this command at startup.
>>
>> On Tue, Dec 11, 2018 at 2:17 PM Adrian McCarthy 
>> wrote:
>>
>>> I believe the PDB is searched for in the EXE directory before the symbol
>>> search path is used.  At least, that's what it used to do, back when I used
>>> VS debugger for post-mortem debugging.  It was the only sane way to ensure
>>> it would find the right version of the PDB if you didn't have a local
>>> symbol server.
>>>
>>> Yes, I understand that the security note was about DLL loading.  My
>>> point was that, in general, Windows looks in well known places first,
>>> before checking the current working directory.
>>>
>>> On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu 
>>> wrote:
>>>
 The Windowsy thing to do is what Zach said:  Check the directory that
> contains the .dmp for the .pdb.  It's the first place the VS debugger 
> looks
> when opening a minidump.  It's less sensitive to the user's environment.
> (Making the user change the current working directory for this could be at
> odds with other bits of the software that look relative the cwd.)
>
> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>

 Except that it doesn't :) Neither VisualStudio nor the Windows
 Debuggers (windbg & co) look for PDBs in the same directory as the dump
 file. The search is controlled by an explicit "symbol search path". The
 link you mentioned is a bit confusing indeed (although it only claims that
 the .exe's are searched in the same directory as the .dmp)

 While security is not a big issue here, note that Windows generally
> searches for DLLs in the known/expected places _before_ checking to see if
> it's in the current working directory.  This prevents a sneaky download
> from effectively replacing a DLL.  Replacing a PDB is probably less of a
> concern.
>
> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>

 This is about the runtime dynamic module loader, not the debugger.




 On 

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

>
> But here, we're talking about a situation where there is no EXE, only a
> minidump.  If there is a minidump and no EXE then neither WinDbg nor VS
> will search the minidump folder for the PDB.


Indeed, this is key part.


>  In my mind, the algorithm could be something like: ...
>

I'm not a big fan of this kind of magic. VS sometime does it, and while
it's nice in the 80% of the cases I found it hard to troubleshoot when
something goes wrong. For what it's worth, recent versions of VC++ debugger
are more aligned with ntsd/cdb/windbg.

Anyway, for this patch I think the question is: what's the simplest
temporary solution? All of these ideas are interesting but more appropriate
to evaluate when we start designing the PDB SymbolVendor.



On Tue, Dec 11, 2018 at 3:05 PM Zachary Turner  wrote:

> Only one way to know for sure, and that's to test it :)  So I did.
>
> Yes, it will search the directory of the EXE for the PDB.  But here, we're
> talking about a situation where there is no EXE, only a minidump.  If there
> is a minidump and no EXE then neither WinDbg nor VS will search the
> minidump folder for the PDB.  Personally I think it should and I wouldn't
> mind if that were a documented and tested feature of LLDB, because since it
> already is established behavior to search the minidump folder for the EXE,
> it doesn't seem hacky at all for it to also search that location for the
> PDB.  In my mind, the algorithm could be something like:
>
> ```
> if (ExeLocation = FindExecutableInMinidumpFolder()) {
>   PdbLocation = FindPdbFromExecutable(ExeLocation)
> } else {
>   PdbLocation = FindPdbInMinidumpFolder();
>   if (!PdbLocation)
> PdbLocation = FindPdbInSympath();
> }
> ```
>
> and that would be pretty logical and not code that we would have to
> consider temporary.
>
> My main pushback here is that it's harder to remove code than it is to add
> it, because once you add it, someone is going to depend on it and complain
> when you try to remove it.  So if we can just establish some behavior that
> satisfies the use case while not being temporary, I would prefer to do it.
>
> Another option I can think of is to just run `target symbols add -s
> foo.exe foo.pdb`.  I think we would need to have SymbolFileNativePDB check
> the value of this setting, but using this workflow, you could just put a
> .lldbinit file next to lldb.exe that runs this command at startup.
>
> On Tue, Dec 11, 2018 at 2:17 PM Adrian McCarthy 
> wrote:
>
>> I believe the PDB is searched for in the EXE directory before the symbol
>> search path is used.  At least, that's what it used to do, back when I used
>> VS debugger for post-mortem debugging.  It was the only sane way to ensure
>> it would find the right version of the PDB if you didn't have a local
>> symbol server.
>>
>> Yes, I understand that the security note was about DLL loading.  My point
>> was that, in general, Windows looks in well known places first, before
>> checking the current working directory.
>>
>> On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu 
>> wrote:
>>
>>> The Windowsy thing to do is what Zach said:  Check the directory that
 contains the .dmp for the .pdb.  It's the first place the VS debugger looks
 when opening a minidump.  It's less sensitive to the user's environment.
 (Making the user change the current working directory for this could be at
 odds with other bits of the software that look relative the cwd.)

 https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files

>>>
>>> Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
>>> (windbg & co) look for PDBs in the same directory as the dump file. The
>>> search is controlled by an explicit "symbol search path". The link you
>>> mentioned is a bit confusing indeed (although it only claims that the
>>> .exe's are searched in the same directory as the .dmp)
>>>
>>> While security is not a big issue here, note that Windows generally
 searches for DLLs in the known/expected places _before_ checking to see if
 it's in the current working directory.  This prevents a sneaky download
 from effectively replacing a DLL.  Replacing a PDB is probably less of a
 concern.

 https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications

>>>
>>> This is about the runtime dynamic module loader, not the debugger.
>>>
>>>
>>>
>>>
>>> On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy 
>>> wrote:
>>>
 It's really frustrating how the email discussion doesn't always make it
 to Phabricator.

 The Windowsy thing to do is what Zach said:  Check the directory that
 contains the .dmp for the .pdb.  It's the first place the VS debugger looks
 when opening a minidump.  It's less sensitive to the user's environment.
 (Making the user change the current working 

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

Actually, Adrian just tested this on his machine and it did look in his
minidump folder.  I don't know why we're seeing different behavior.  Either
way, regardless of whether MSVC / WinDbg look in the minidump folder, I
still think it's a pretty intuitive location to add in the default search
path.  But if people disagree, it seems like target symbols add would still
address the problem in a permanent way.

On Tue, Dec 11, 2018 at 3:04 PM Zachary Turner  wrote:

> Only one way to know for sure, and that's to test it :)  So I did.
>
> Yes, it will search the directory of the EXE for the PDB.  But here, we're
> talking about a situation where there is no EXE, only a minidump.  If there
> is a minidump and no EXE then neither WinDbg nor VS will search the
> minidump folder for the PDB.  Personally I think it should and I wouldn't
> mind if that were a documented and tested feature of LLDB, because since it
> already is established behavior to search the minidump folder for the EXE,
> it doesn't seem hacky at all for it to also search that location for the
> PDB.  In my mind, the algorithm could be something like:
>
> ```
> if (ExeLocation = FindExecutableInMinidumpFolder()) {
>   PdbLocation = FindPdbFromExecutable(ExeLocation)
> } else {
>   PdbLocation = FindPdbInMinidumpFolder();
>   if (!PdbLocation)
> PdbLocation = FindPdbInSympath();
> }
> ```
>
> and that would be pretty logical and not code that we would have to
> consider temporary.
>
> My main pushback here is that it's harder to remove code than it is to add
> it, because once you add it, someone is going to depend on it and complain
> when you try to remove it.  So if we can just establish some behavior that
> satisfies the use case while not being temporary, I would prefer to do it.
>
> Another option I can think of is to just run `target symbols add -s
> foo.exe foo.pdb`.  I think we would need to have SymbolFileNativePDB check
> the value of this setting, but using this workflow, you could just put a
> .lldbinit file next to lldb.exe that runs this command at startup.
>
> On Tue, Dec 11, 2018 at 2:17 PM Adrian McCarthy 
> wrote:
>
>> I believe the PDB is searched for in the EXE directory before the symbol
>> search path is used.  At least, that's what it used to do, back when I used
>> VS debugger for post-mortem debugging.  It was the only sane way to ensure
>> it would find the right version of the PDB if you didn't have a local
>> symbol server.
>>
>> Yes, I understand that the security note was about DLL loading.  My point
>> was that, in general, Windows looks in well known places first, before
>> checking the current working directory.
>>
>> On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu 
>> wrote:
>>
>>> The Windowsy thing to do is what Zach said:  Check the directory that
 contains the .dmp for the .pdb.  It's the first place the VS debugger looks
 when opening a minidump.  It's less sensitive to the user's environment.
 (Making the user change the current working directory for this could be at
 odds with other bits of the software that look relative the cwd.)

 https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files

>>>
>>> Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
>>> (windbg & co) look for PDBs in the same directory as the dump file. The
>>> search is controlled by an explicit "symbol search path". The link you
>>> mentioned is a bit confusing indeed (although it only claims that the
>>> .exe's are searched in the same directory as the .dmp)
>>>
>>> While security is not a big issue here, note that Windows generally
 searches for DLLs in the known/expected places _before_ checking to see if
 it's in the current working directory.  This prevents a sneaky download
 from effectively replacing a DLL.  Replacing a PDB is probably less of a
 concern.

 https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications

>>>
>>> This is about the runtime dynamic module loader, not the debugger.
>>>
>>>
>>>
>>>
>>> On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy 
>>> wrote:
>>>
 It's really frustrating how the email discussion doesn't always make it
 to Phabricator.

 The Windowsy thing to do is what Zach said:  Check the directory that
 contains the .dmp for the .pdb.  It's the first place the VS debugger looks
 when opening a minidump.  It's less sensitive to the user's environment.
 (Making the user change the current working directory for this could be at
 odds with other bits of the software that look relative the cwd.)


 https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files

 While security is not a big issue here, note that Windows generally
 

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

Only one way to know for sure, and that's to test it :)  So I did.

Yes, it will search the directory of the EXE for the PDB.  But here, we're
talking about a situation where there is no EXE, only a minidump.  If there
is a minidump and no EXE then neither WinDbg nor VS will search the
minidump folder for the PDB.  Personally I think it should and I wouldn't
mind if that were a documented and tested feature of LLDB, because since it
already is established behavior to search the minidump folder for the EXE,
it doesn't seem hacky at all for it to also search that location for the
PDB.  In my mind, the algorithm could be something like:

```
if (ExeLocation = FindExecutableInMinidumpFolder()) {
  PdbLocation = FindPdbFromExecutable(ExeLocation)
} else {
  PdbLocation = FindPdbInMinidumpFolder();
  if (!PdbLocation)
PdbLocation = FindPdbInSympath();
}
```

and that would be pretty logical and not code that we would have to
consider temporary.

My main pushback here is that it's harder to remove code than it is to add
it, because once you add it, someone is going to depend on it and complain
when you try to remove it.  So if we can just establish some behavior that
satisfies the use case while not being temporary, I would prefer to do it.

Another option I can think of is to just run `target symbols add -s foo.exe
foo.pdb`.  I think we would need to have SymbolFileNativePDB check the
value of this setting, but using this workflow, you could just put a
.lldbinit file next to lldb.exe that runs this command at startup.

On Tue, Dec 11, 2018 at 2:17 PM Adrian McCarthy  wrote:

> I believe the PDB is searched for in the EXE directory before the symbol
> search path is used.  At least, that's what it used to do, back when I used
> VS debugger for post-mortem debugging.  It was the only sane way to ensure
> it would find the right version of the PDB if you didn't have a local
> symbol server.
>
> Yes, I understand that the security note was about DLL loading.  My point
> was that, in general, Windows looks in well known places first, before
> checking the current working directory.
>
> On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu 
> wrote:
>
>> The Windowsy thing to do is what Zach said:  Check the directory that
>>> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
>>> when opening a minidump.  It's less sensitive to the user's environment.
>>> (Making the user change the current working directory for this could be at
>>> odds with other bits of the software that look relative the cwd.)
>>>
>>> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>>>
>>
>> Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
>> (windbg & co) look for PDBs in the same directory as the dump file. The
>> search is controlled by an explicit "symbol search path". The link you
>> mentioned is a bit confusing indeed (although it only claims that the
>> .exe's are searched in the same directory as the .dmp)
>>
>> While security is not a big issue here, note that Windows generally
>>> searches for DLLs in the known/expected places _before_ checking to see if
>>> it's in the current working directory.  This prevents a sneaky download
>>> from effectively replacing a DLL.  Replacing a PDB is probably less of a
>>> concern.
>>>
>>> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>>>
>>
>> This is about the runtime dynamic module loader, not the debugger.
>>
>>
>>
>>
>> On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy 
>> wrote:
>>
>>> It's really frustrating how the email discussion doesn't always make it
>>> to Phabricator.
>>>
>>> The Windowsy thing to do is what Zach said:  Check the directory that
>>> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
>>> when opening a minidump.  It's less sensitive to the user's environment.
>>> (Making the user change the current working directory for this could be at
>>> odds with other bits of the software that look relative the cwd.)
>>>
>>>
>>> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>>>
>>> While security is not a big issue here, note that Windows generally
>>> searches for DLLs in the known/expected places _before_ checking to see if
>>> it's in the current working directory.  This prevents a sneaky download
>>> from effectively replacing a DLL.  Replacing a PDB is probably less of a
>>> concern.
>>>
>>>
>>> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>>>
>>> So I +1 Zach's proposal.
>>>
>>> On Tue, Dec 11, 2018 at 12:07 PM Leonard Mosescu 
>>> wrote:
>>>
 I think as combination of explicit symbol search path + something
 similar to Microsoft's 

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Adrian McCarthy via lldb-commits]

I believe the PDB is searched for in the EXE directory before the symbol
search path is used.  At least, that's what it used to do, back when I used
VS debugger for post-mortem debugging.  It was the only sane way to ensure
it would find the right version of the PDB if you didn't have a local
symbol server.

Yes, I understand that the security note was about DLL loading.  My point
was that, in general, Windows looks in well known places first, before
checking the current working directory.

On Tue, Dec 11, 2018 at 2:12 PM Leonard Mosescu  wrote:

> The Windowsy thing to do is what Zach said:  Check the directory that
>> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
>> when opening a minidump.  It's less sensitive to the user's environment.
>> (Making the user change the current working directory for this could be at
>> odds with other bits of the software that look relative the cwd.)
>>
>> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>>
>
> Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
> (windbg & co) look for PDBs in the same directory as the dump file. The
> search is controlled by an explicit "symbol search path". The link you
> mentioned is a bit confusing indeed (although it only claims that the
> .exe's are searched in the same directory as the .dmp)
>
> While security is not a big issue here, note that Windows generally
>> searches for DLLs in the known/expected places _before_ checking to see if
>> it's in the current working directory.  This prevents a sneaky download
>> from effectively replacing a DLL.  Replacing a PDB is probably less of a
>> concern.
>>
>> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>>
>
> This is about the runtime dynamic module loader, not the debugger.
>
>
>
>
> On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy 
> wrote:
>
>> It's really frustrating how the email discussion doesn't always make it
>> to Phabricator.
>>
>> The Windowsy thing to do is what Zach said:  Check the directory that
>> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
>> when opening a minidump.  It's less sensitive to the user's environment.
>> (Making the user change the current working directory for this could be at
>> odds with other bits of the software that look relative the cwd.)
>>
>>
>> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>>
>> While security is not a big issue here, note that Windows generally
>> searches for DLLs in the known/expected places _before_ checking to see if
>> it's in the current working directory.  This prevents a sneaky download
>> from effectively replacing a DLL.  Replacing a PDB is probably less of a
>> concern.
>>
>>
>> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>>
>> So I +1 Zach's proposal.
>>
>> On Tue, Dec 11, 2018 at 12:07 PM Leonard Mosescu 
>> wrote:
>>
>>> I think as combination of explicit symbol search path + something
>>> similar to Microsoft's symsrv would be a good "real" solution (and yes,
>>> that would be packaged as a SymbolVendor, outside SymbolFilePDB)
>>>
>>> For short term, I don't see a clearly superior alternative to searching
>>> the current directory.
>>>
>>> On Tue, Dec 11, 2018 at 12:02 PM Pavel Labath  wrote:
>>>
 On 11/12/2018 20:34, Zachary Turner wrote:
 > I meant the location of the minidump.  So if you have
 C:\A\B\C\foo.dmp
 > which is the dump file for bar.exe which crashed on another machine,
 > then it would look for C:\A\B\C\bar.pdb.  That actually seems like
 > fairly intuitive behavior to me, but maybe I'm in the minority :)
 >
 > We can see what Pavel, Adrian, and others think though or if they
 have
 > any other suggestions.
 >

 It sounds like there is a precedent for searching in CWD. I don't know
 how useful it is (I traced it back to r185366, but it is not mentioned
 there specifically), but it is there, and I guess it's not completely
 nonsensical from the POV of a command line user.

 I guess we can just keep that there and not call it a hack (though, the
 fact that the searching happens inside SymbolFilePDB *is* a hack).

 Searching in the minidump directory would also make sense somewhat, but
 I expect you would need more plumbing for that to happen (and I don't
 know of a precedent for that).

 pl

>>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

>
> The Windowsy thing to do is what Zach said:  Check the directory that
> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
> when opening a minidump.  It's less sensitive to the user's environment.
> (Making the user change the current working directory for this could be at
> odds with other bits of the software that look relative the cwd.)
>
> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>

Except that it doesn't :) Neither VisualStudio nor the Windows Debuggers
(windbg & co) look for PDBs in the same directory as the dump file. The
search is controlled by an explicit "symbol search path". The link you
mentioned is a bit confusing indeed (although it only claims that the
.exe's are searched in the same directory as the .dmp)

While security is not a big issue here, note that Windows generally
> searches for DLLs in the known/expected places _before_ checking to see if
> it's in the current working directory.  This prevents a sneaky download
> from effectively replacing a DLL.  Replacing a PDB is probably less of a
> concern.
>
> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>

This is about the runtime dynamic module loader, not the debugger.




On Tue, Dec 11, 2018 at 1:28 PM Adrian McCarthy  wrote:

> It's really frustrating how the email discussion doesn't always make it to
> Phabricator.
>
> The Windowsy thing to do is what Zach said:  Check the directory that
> contains the .dmp for the .pdb.  It's the first place the VS debugger looks
> when opening a minidump.  It's less sensitive to the user's environment.
> (Making the user change the current working directory for this could be at
> odds with other bits of the software that look relative the cwd.)
>
>
> https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files
>
> While security is not a big issue here, note that Windows generally
> searches for DLLs in the known/expected places _before_ checking to see if
> it's in the current working directory.  This prevents a sneaky download
> from effectively replacing a DLL.  Replacing a PDB is probably less of a
> concern.
>
>
> https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications
>
> So I +1 Zach's proposal.
>
> On Tue, Dec 11, 2018 at 12:07 PM Leonard Mosescu 
> wrote:
>
>> I think as combination of explicit symbol search path + something similar
>> to Microsoft's symsrv would be a good "real" solution (and yes, that would
>> be packaged as a SymbolVendor, outside SymbolFilePDB)
>>
>> For short term, I don't see a clearly superior alternative to searching
>> the current directory.
>>
>> On Tue, Dec 11, 2018 at 12:02 PM Pavel Labath  wrote:
>>
>>> On 11/12/2018 20:34, Zachary Turner wrote:
>>> > I meant the location of the minidump.  So if you have C:\A\B\C\foo.dmp
>>> > which is the dump file for bar.exe which crashed on another machine,
>>> > then it would look for C:\A\B\C\bar.pdb.  That actually seems like
>>> > fairly intuitive behavior to me, but maybe I'm in the minority :)
>>> >
>>> > We can see what Pavel, Adrian, and others think though or if they have
>>> > any other suggestions.
>>> >
>>>
>>> It sounds like there is a precedent for searching in CWD. I don't know
>>> how useful it is (I traced it back to r185366, but it is not mentioned
>>> there specifically), but it is there, and I guess it's not completely
>>> nonsensical from the POV of a command line user.
>>>
>>> I guess we can just keep that there and not call it a hack (though, the
>>> fact that the searching happens inside SymbolFilePDB *is* a hack).
>>>
>>> Searching in the minidump directory would also make sense somewhat, but
>>> I expect you would need more plumbing for that to happen (and I don't
>>> know of a precedent for that).
>>>
>>> pl
>>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Adrian McCarthy via lldb-commits]

It's really frustrating how the email discussion doesn't always make it to
Phabricator.

The Windowsy thing to do is what Zach said:  Check the directory that
contains the .dmp for the .pdb.  It's the first place the VS debugger looks
when opening a minidump.  It's less sensitive to the user's environment.
(Making the user change the current working directory for this could be at
odds with other bits of the software that look relative the cwd.)

https://docs.microsoft.com/en-us/visualstudio/debugger/using-dump-files?view=vs-2017#BKMK_Find_binaries__symbol___pdb__files__and_source_files

While security is not a big issue here, note that Windows generally
searches for DLLs in the known/expected places _before_ checking to see if
it's in the current working directory.  This prevents a sneaky download
from effectively replacing a DLL.  Replacing a PDB is probably less of a
concern.

https://docs.microsoft.com/en-us/windows/desktop/dlls/dynamic-link-library-search-order#standard-search-order-for-desktop-applications

So I +1 Zach's proposal.

On Tue, Dec 11, 2018 at 12:07 PM Leonard Mosescu  wrote:

> I think as combination of explicit symbol search path + something similar
> to Microsoft's symsrv would be a good "real" solution (and yes, that would
> be packaged as a SymbolVendor, outside SymbolFilePDB)
>
> For short term, I don't see a clearly superior alternative to searching
> the current directory.
>
> On Tue, Dec 11, 2018 at 12:02 PM Pavel Labath  wrote:
>
>> On 11/12/2018 20:34, Zachary Turner wrote:
>> > I meant the location of the minidump.  So if you have C:\A\B\C\foo.dmp
>> > which is the dump file for bar.exe which crashed on another machine,
>> > then it would look for C:\A\B\C\bar.pdb.  That actually seems like
>> > fairly intuitive behavior to me, but maybe I'm in the minority :)
>> >
>> > We can see what Pavel, Adrian, and others think though or if they have
>> > any other suggestions.
>> >
>>
>> It sounds like there is a precedent for searching in CWD. I don't know
>> how useful it is (I traced it back to r185366, but it is not mentioned
>> there specifically), but it is there, and I guess it's not completely
>> nonsensical from the POV of a command line user.
>>
>> I guess we can just keep that there and not call it a hack (though, the
>> fact that the searching happens inside SymbolFilePDB *is* a hack).
>>
>> Searching in the minidump directory would also make sense somewhat, but
>> I expect you would need more plumbing for that to happen (and I don't
>> know of a precedent for that).
>>
>> pl
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

I meant the location of the minidump.  So if you have C:\A\B\C\foo.dmp
which is the dump file for bar.exe which crashed on another machine, then
it would look for C:\A\B\C\bar.pdb.  That actually seems like fairly
intuitive behavior to me, but maybe I'm in the minority :)

We can see what Pavel, Adrian, and others think though or if they have any
other suggestions.

On Tue, Dec 11, 2018 at 11:21 AM Leonard Mosescu  wrote:

> But if the minidump and PDBs are in the same directory, then wouldn't my
>> proposed solution also work (while also being a permanent solution)?
>>
>
> If we're looking in the same directory as the binary file (which is how I
> read your suggestion) then it would not be found in this case, since the
> binary path is coming from the machine where the crash occurred (so there's
> no relation with the host where we run LLDB). Using the minidump location
> as search path seems an even bigger hack than searching the current
> directory.
>
> Speaking of the current directory, I still don't like to use it implicitly
> in general, but it's actually consistent with what LLDB does for DWARF (see
> Symbols::LocateExecutableSymbolFile). So maybe it's not the terrible hack I
> made it look like.
>
> On Tue, Dec 11, 2018 at 10:48 AM Zachary Turner 
> wrote:
>
>> But if the minidump and PDBs are in the same directory, then wouldn't my
>> proposed solution also work (while also being a permanent solution)?
>>
>> On Tue, Dec 11, 2018 at 10:47 AM Leonard Mosescu 
>> wrote:
>>
>>> We talked about this offline, but bringing the discussion back here.
 Can you describe the use case that this is addressing?  As you mention,
 this is a temporary hack until we have proper symbol searching logic, but
 proper symbol searching logic will do more than just look up symbols in a
 symbol server.  It will also, for example, look in the same directory as
 the executable file.  If we changed this logic to do that, would your use
 case still be addressed?  At least that way, the logic we're adding is not
 temporary, even if it will eventually live in a different place (e.g. the
 SymbolVendor).

>>>
>>> This is intended to provide an easy way to experiment with minidumps +
>>> PDBs: just copy the minidump and the PDBs in the same directory (and run
>>> lldb from there).
>>>
>>> It's far from a general solution. I don't think that defaulting to the
>>> current directory should even be a hardcoded default - it's just a
>>> convenient but temporary hack. I'm open to any alternative ideas we can use
>>> until we implement a SymbolVendor.
>>>
>>> On Tue, Dec 11, 2018 at 10:39 AM Zachary Turner via Phabricator <
>>> revi...@reviews.llvm.org> wrote:
>>>
 zturner added inline comments.


 
 Comment at:
 source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
 +llvm::consumeError(expected_binary.takeError());
 +pdb_file = obj_file.GetFileSpec()
 +   .GetFileNameStrippingExtension()
 +   .GetStringRef()
 +   .str();
 +pdb_file += ".pdb";
 
 We talked about this offline, but bringing the discussion back here.
 Can you describe the use case that this is addressing?  As you mention,
 this is a temporary hack until we have proper symbol searching logic, but
 proper symbol searching logic will do more than just look up symbols in a
 symbol server.  It will also, for example, look in the same directory as
 the executable file.  If we changed this logic to do that, would your use
 case still be addressed?  At least that way, the logic we're adding is not
 temporary, even if it will eventually live in a different place (e.g. the
 SymbolVendor).


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

 https://reviews.llvm.org/D55142




___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

>
> But if the minidump and PDBs are in the same directory, then wouldn't my
> proposed solution also work (while also being a permanent solution)?
>

If we're looking in the same directory as the binary file (which is how I
read your suggestion) then it would not be found in this case, since the
binary path is coming from the machine where the crash occurred (so there's
no relation with the host where we run LLDB). Using the minidump location
as search path seems an even bigger hack than searching the current
directory.

Speaking of the current directory, I still don't like to use it implicitly
in general, but it's actually consistent with what LLDB does for DWARF (see
Symbols::LocateExecutableSymbolFile). So maybe it's not the terrible hack I
made it look like.

On Tue, Dec 11, 2018 at 10:48 AM Zachary Turner  wrote:

> But if the minidump and PDBs are in the same directory, then wouldn't my
> proposed solution also work (while also being a permanent solution)?
>
> On Tue, Dec 11, 2018 at 10:47 AM Leonard Mosescu 
> wrote:
>
>> We talked about this offline, but bringing the discussion back here.  Can
>>> you describe the use case that this is addressing?  As you mention, this is
>>> a temporary hack until we have proper symbol searching logic, but proper
>>> symbol searching logic will do more than just look up symbols in a symbol
>>> server.  It will also, for example, look in the same directory as the
>>> executable file.  If we changed this logic to do that, would your use case
>>> still be addressed?  At least that way, the logic we're adding is not
>>> temporary, even if it will eventually live in a different place (e.g. the
>>> SymbolVendor).
>>>
>>
>> This is intended to provide an easy way to experiment with minidumps +
>> PDBs: just copy the minidump and the PDBs in the same directory (and run
>> lldb from there).
>>
>> It's far from a general solution. I don't think that defaulting to the
>> current directory should even be a hardcoded default - it's just a
>> convenient but temporary hack. I'm open to any alternative ideas we can use
>> until we implement a SymbolVendor.
>>
>> On Tue, Dec 11, 2018 at 10:39 AM Zachary Turner via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> zturner added inline comments.
>>>
>>>
>>> 
>>> Comment at:
>>> source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
>>> +llvm::consumeError(expected_binary.takeError());
>>> +pdb_file = obj_file.GetFileSpec()
>>> +   .GetFileNameStrippingExtension()
>>> +   .GetStringRef()
>>> +   .str();
>>> +pdb_file += ".pdb";
>>> 
>>> We talked about this offline, but bringing the discussion back here.
>>> Can you describe the use case that this is addressing?  As you mention,
>>> this is a temporary hack until we have proper symbol searching logic, but
>>> proper symbol searching logic will do more than just look up symbols in a
>>> symbol server.  It will also, for example, look in the same directory as
>>> the executable file.  If we changed this logic to do that, would your use
>>> case still be addressed?  At least that way, the logic we're adding is not
>>> temporary, even if it will eventually live in a different place (e.g. the
>>> SymbolVendor).
>>>
>>>
>>> CHANGES SINCE LAST ACTION
>>>   https://reviews.llvm.org/D55142/new/
>>>
>>> https://reviews.llvm.org/D55142
>>>
>>>
>>>
>>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via lldb-commits]

But if the minidump and PDBs are in the same directory, then wouldn't my
proposed solution also work (while also being a permanent solution)?

On Tue, Dec 11, 2018 at 10:47 AM Leonard Mosescu  wrote:

> We talked about this offline, but bringing the discussion back here.  Can
>> you describe the use case that this is addressing?  As you mention, this is
>> a temporary hack until we have proper symbol searching logic, but proper
>> symbol searching logic will do more than just look up symbols in a symbol
>> server.  It will also, for example, look in the same directory as the
>> executable file.  If we changed this logic to do that, would your use case
>> still be addressed?  At least that way, the logic we're adding is not
>> temporary, even if it will eventually live in a different place (e.g. the
>> SymbolVendor).
>>
>
> This is intended to provide an easy way to experiment with minidumps +
> PDBs: just copy the minidump and the PDBs in the same directory (and run
> lldb from there).
>
> It's far from a general solution. I don't think that defaulting to the
> current directory should even be a hardcoded default - it's just a
> convenient but temporary hack. I'm open to any alternative ideas we can use
> until we implement a SymbolVendor.
>
> On Tue, Dec 11, 2018 at 10:39 AM Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> zturner added inline comments.
>>
>>
>> 
>> Comment at:
>> source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
>> +llvm::consumeError(expected_binary.takeError());
>> +pdb_file = obj_file.GetFileSpec()
>> +   .GetFileNameStrippingExtension()
>> +   .GetStringRef()
>> +   .str();
>> +pdb_file += ".pdb";
>> 
>> We talked about this offline, but bringing the discussion back here.  Can
>> you describe the use case that this is addressing?  As you mention, this is
>> a temporary hack until we have proper symbol searching logic, but proper
>> symbol searching logic will do more than just look up symbols in a symbol
>> server.  It will also, for example, look in the same directory as the
>> executable file.  If we changed this logic to do that, would your use case
>> still be addressed?  At least that way, the logic we're adding is not
>> temporary, even if it will eventually live in a different place (e.g. the
>> SymbolVendor).
>>
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D55142/new/
>>
>> https://reviews.llvm.org/D55142
>>
>>
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

>
> We talked about this offline, but bringing the discussion back here.  Can
> you describe the use case that this is addressing?  As you mention, this is
> a temporary hack until we have proper symbol searching logic, but proper
> symbol searching logic will do more than just look up symbols in a symbol
> server.  It will also, for example, look in the same directory as the
> executable file.  If we changed this logic to do that, would your use case
> still be addressed?  At least that way, the logic we're adding is not
> temporary, even if it will eventually live in a different place (e.g. the
> SymbolVendor).
>

This is intended to provide an easy way to experiment with minidumps +
PDBs: just copy the minidump and the PDBs in the same directory (and run
lldb from there).

It's far from a general solution. I don't think that defaulting to the
current directory should even be a hardcoded default - it's just a
convenient but temporary hack. I'm open to any alternative ideas we can use
until we implement a SymbolVendor.

On Tue, Dec 11, 2018 at 10:39 AM Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added inline comments.
>
>
> 
> Comment at:
> source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
> +llvm::consumeError(expected_binary.takeError());
> +pdb_file = obj_file.GetFileSpec()
> +   .GetFileNameStrippingExtension()
> +   .GetStringRef()
> +   .str();
> +pdb_file += ".pdb";
> 
> We talked about this offline, but bringing the discussion back here.  Can
> you describe the use case that this is addressing?  As you mention, this is
> a temporary hack until we have proper symbol searching logic, but proper
> symbol searching logic will do more than just look up symbols in a symbol
> server.  It will also, for example, look in the same directory as the
> executable file.  If we changed this logic to do that, would your use case
> still be addressed?  At least that way, the logic we're adding is not
> temporary, even if it will eventually live in a different place (e.g. the
> SymbolVendor).
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144
+llvm::consumeError(expected_binary.takeError());
+pdb_file = obj_file.GetFileSpec()
+   .GetFileNameStrippingExtension()
+   .GetStringRef()
+   .str();
+pdb_file += ".pdb";

We talked about this offline, but bringing the discussion back here.  Can you 
describe the use case that this is addressing?  As you mention, this is a 
temporary hack until we have proper symbol searching logic, but proper symbol 
searching logic will do more than just look up symbols in a symbol server.  It 
will also, for example, look in the same directory as the executable file.  If 
we changed this logic to do that, would your use case still be addressed?  At 
least that way, the logic we're adding is not temporary, even if it will 
eventually live in a different place (e.g. the SymbolVendor).


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

Thanks Pavel and Greg.

It sounds to me like it would be better to have a separate command
> (let's call it "target modules replace" for now) for adding an "object
> file" to a "placeholder" module, instead of repurposing "target symbols
> add" to do that.


Yes, that would be my preference as well.

Imagine the following scenario:
> - I load a minidump, without any supporting files around ...


I'd like to support the "other" variation as well: we start with a
minidump, load the symbols, _then_ dig for some module's binary (for
example if we really need image bits that are not captured in the minidump)

On Tue, Dec 11, 2018 at 9:50 AM Greg Clayton  wrote:

>
>
> > On Dec 11, 2018, at 7:14 AM, Pavel Labath  wrote:
> >
> > On 11/12/2018 01:08, Greg Clayton wrote:
> >>> On Dec 10, 2018, at 3:11 PM, Leonard Mosescu  > wrote:
> >>>
> >>> I can see how this works for the PDB, no-module-binary case. What
> about the PDB & module-binary case?
> >> That would work fine because the symbol vendor will make an object file
> from the m_symfile_spec and use both the original binary + the symbol file
> to make symbols.
> >>> Maybe I missed the rationale, but I think that modeling the general
> case: module and symbols are separate files, is a better foundation for the
> particular case where the symbols are embedded in the binary file.
> >> Yeah, my case should handle the following cases:
> >> - Minidumps Placeholder modules only, no real binaries, add symbols by
> downloading them and using "target symbols add ..."
> >> - Download real binaries up front and load them into LLDB, then load
> the core file. For any binaries we do have, we should grab them from the
> global module cache (GetTarget().GetSharedModule(...) in the current code)
> and they will use real object files, not placeholder files. "target symbols
> add ..." still works in this case
> >
> > It sounds to me like it would be better to have a separate command
> (let's call it "target modules replace" for now) for adding an "object
> file" to a "placeholder" module, instead of repurposing "target symbols
> add" to do that.
> >
> > This creates a strange loop between the symbol and object files --
> normally we use the object file for symbol representation if the user
> hasn't specified a symbol file, and here, we would do the opposite.
> >
> > With "target modules replace", one command would still be enough to set
> both the symbol and object files if they are the same, as the default
> use-symbols-from-object-file behavior would kick in. However, it would
> leave the "target symbols add" command free to add symbols from an external
> file.
> >
> > Imagine the following scenario:
> > - I load a minidump, without any supporting files around
> > - I see that my object file is missing, I do some digging around, and
> manage to find the stripped object file for this module
> > - I do "target modules replace index_of_placeholder_module
> /path/to/elf.stripped"
> > - now my sections are there, but I still don't have symbols
> > - I do more digging and find the breakpad file
> > - I do "target symbols add /path/to/breakpad.syms"
> > - success. I now have both symbols and sections
> >
> > Now this is probably not the most common scenario, but I think it can be
> used as a benchmark to see if the infrastructure is set up the right way.
> If things are set up correctly this should work out-of-the-box. With your
> setup, the scenario above might "just work" if I execute "target symbols
> add" twice (with different files), because the second "target symbols add"
> will do something different than the first one, but that sounds to me like
> another reason why these should be different commands.
> >
> > Maybe there should be there should be some code to help the user and
> detect the situation when he is adding a symbol file to a placeholder
> module and the symbol file is able serve as a good object file too, but
> that code could live higher up than Module::GetObjectFile. It could perhaps
> be directly in the "target symbols add" command via something like:
> > if (module_I_am_about_to_add_symbols_to->IsPlaceholder() &&
> symbol_file->ContainsGoodObjectFileRepresentation())
> >  TargetModulesReplace(module_I_am_about_to_add_symbols_to, symbol_file)
> > else
> >  the_module_I_am_about_to_add_symbols_to->SetSymbolFile(symbol_file)
>
> I like the replace idea. Another scenario this will fix is when you start
> debugging with a stripped object file straight from a device, and later
> download the unstripped version and want to replace it. We will need to
> make sure all breakpoints get unresolved and re-resolved correctly in
> testing. Also, stack frames should get flushed as they do when we do
> "target symbols add"
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Greg Clayton via lldb-commits]

> On Dec 11, 2018, at 7:14 AM, Pavel Labath  wrote:
> 
> On 11/12/2018 01:08, Greg Clayton wrote:
>>> On Dec 10, 2018, at 3:11 PM, Leonard Mosescu >> > wrote:
>>> 
>>> I can see how this works for the PDB, no-module-binary case. What about the 
>>> PDB & module-binary case?
>> That would work fine because the symbol vendor will make an object file from 
>> the m_symfile_spec and use both the original binary + the symbol file to 
>> make symbols.
>>> Maybe I missed the rationale, but I think that modeling the general case: 
>>> module and symbols are separate files, is a better foundation for the 
>>> particular case where the symbols are embedded in the binary file.
>> Yeah, my case should handle the following cases:
>> - Minidumps Placeholder modules only, no real binaries, add symbols by 
>> downloading them and using "target symbols add ..."
>> - Download real binaries up front and load them into LLDB, then load the 
>> core file. For any binaries we do have, we should grab them from the global 
>> module cache (GetTarget().GetSharedModule(...) in the current code) and they 
>> will use real object files, not placeholder files. "target symbols add ..." 
>> still works in this case
> 
> It sounds to me like it would be better to have a separate command (let's 
> call it "target modules replace" for now) for adding an "object file" to a 
> "placeholder" module, instead of repurposing "target symbols add" to do that.
> 
> This creates a strange loop between the symbol and object files -- normally 
> we use the object file for symbol representation if the user hasn't specified 
> a symbol file, and here, we would do the opposite.
> 
> With "target modules replace", one command would still be enough to set both 
> the symbol and object files if they are the same, as the default 
> use-symbols-from-object-file behavior would kick in. However, it would leave 
> the "target symbols add" command free to add symbols from an external file.
> 
> Imagine the following scenario:
> - I load a minidump, without any supporting files around
> - I see that my object file is missing, I do some digging around, and manage 
> to find the stripped object file for this module
> - I do "target modules replace index_of_placeholder_module 
> /path/to/elf.stripped"
> - now my sections are there, but I still don't have symbols
> - I do more digging and find the breakpad file
> - I do "target symbols add /path/to/breakpad.syms"
> - success. I now have both symbols and sections
> 
> Now this is probably not the most common scenario, but I think it can be used 
> as a benchmark to see if the infrastructure is set up the right way. If 
> things are set up correctly this should work out-of-the-box. With your setup, 
> the scenario above might "just work" if I execute "target symbols add" twice 
> (with different files), because the second "target symbols add" will do 
> something different than the first one, but that sounds to me like another 
> reason why these should be different commands.
> 
> Maybe there should be there should be some code to help the user and detect 
> the situation when he is adding a symbol file to a placeholder module and the 
> symbol file is able serve as a good object file too, but that code could live 
> higher up than Module::GetObjectFile. It could perhaps be directly in the 
> "target symbols add" command via something like:
> if (module_I_am_about_to_add_symbols_to->IsPlaceholder() && 
> symbol_file->ContainsGoodObjectFileRepresentation())
>  TargetModulesReplace(module_I_am_about_to_add_symbols_to, symbol_file)
> else
>  the_module_I_am_about_to_add_symbols_to->SetSymbolFile(symbol_file)

I like the replace idea. Another scenario this will fix is when you start 
debugging with a stripped object file straight from a device, and later 
download the unstripped version and want to replace it. We will need to make 
sure all breakpoints get unresolved and re-resolved correctly in testing. Also, 
stack frames should get flushed as they do when we do "target symbols add"

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via lldb-commits]

On 11/12/2018 01:08, Greg Clayton wrote:



On Dec 10, 2018, at 3:11 PM, Leonard Mosescu > wrote:


I can see how this works for the PDB, no-module-binary case. What 
about the PDB & module-binary case?


That would work fine because the symbol vendor will make an object file 
from the m_symfile_spec and use both the original binary + the symbol 
file to make symbols.


Maybe I missed the rationale, but I think that modeling the general 
case: module and symbols are separate files, is a better foundation 
for the particular case where the symbols are embedded in the binary file.


Yeah, my case should handle the following cases:
- Minidumps Placeholder modules only, no real binaries, add symbols by 
downloading them and using "target symbols add ..."
- Download real binaries up front and load them into LLDB, then load the 
core file. For any binaries we do have, we should grab them from the 
global module cache (GetTarget().GetSharedModule(...) in the current 
code) and they will use real object files, not placeholder files. 
"target symbols add ..." still works in this case




It sounds to me like it would be better to have a separate command 
(let's call it "target modules replace" for now) for adding an "object 
file" to a "placeholder" module, instead of repurposing "target symbols 
add" to do that.


This creates a strange loop between the symbol and object files -- 
normally we use the object file for symbol representation if the user 
hasn't specified a symbol file, and here, we would do the opposite.


With "target modules replace", one command would still be enough to set 
both the symbol and object files if they are the same, as the default 
use-symbols-from-object-file behavior would kick in. However, it would 
leave the "target symbols add" command free to add symbols from an 
external file.


Imagine the following scenario:
- I load a minidump, without any supporting files around
- I see that my object file is missing, I do some digging around, and 
manage to find the stripped object file for this module
- I do "target modules replace index_of_placeholder_module 
/path/to/elf.stripped"

- now my sections are there, but I still don't have symbols
- I do more digging and find the breakpad file
- I do "target symbols add /path/to/breakpad.syms"
- success. I now have both symbols and sections

Now this is probably not the most common scenario, but I think it can be 
used as a benchmark to see if the infrastructure is set up the right 
way. If things are set up correctly this should work out-of-the-box. 
With your setup, the scenario above might "just work" if I execute 
"target symbols add" twice (with different files), because the second 
"target symbols add" will do something different than the first one, but 
that sounds to me like another reason why these should be different 
commands.


Maybe there should be there should be some code to help the user and 
detect the situation when he is adding a symbol file to a placeholder 
module and the symbol file is able serve as a good object file too, but 
that code could live higher up than Module::GetObjectFile. It could 
perhaps be directly in the "target symbols add" command via something like:
if (module_I_am_about_to_add_symbols_to->IsPlaceholder() && 
symbol_file->ContainsGoodObjectFileRepresentation())

  TargetModulesReplace(module_I_am_about_to_add_symbols_to, symbol_file)
else
  the_module_I_am_about_to_add_symbols_to->SetSymbolFile(symbol_file)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Greg Clayton via lldb-commits]

> On Dec 10, 2018, at 3:11 PM, Leonard Mosescu  wrote:
> 
> I can see how this works for the PDB, no-module-binary case. What about the 
> PDB & module-binary case?

That would work fine because the symbol vendor will make an object file from 
the m_symfile_spec and use both the original binary + the symbol file to make 
symbols.

> Maybe I missed the rationale, but I think that modeling the general case: 
> module and symbols are separate files, is a better foundation for the 
> particular case where the symbols are embedded in the binary file.

Yeah, my case should handle the following cases:
- Minidumps Placeholder modules only, no real binaries, add symbols by 
downloading them and using "target symbols add ..."
- Download real binaries up front and load them into LLDB, then load the core 
file. For any binaries we do have, we should grab them from the global module 
cache (GetTarget().GetSharedModule(...) in the current code) and they will use 
real object files, not placeholder files. "target symbols add ..." still works 
in this case

But we can easily layer on top of your PlaceholderModule and the placeholder 
object file if needed. 

Greg

> 
> 
> On Mon, Dec 10, 2018 at 2:48 PM Greg Clayton via Phabricator 
> mailto:revi...@reviews.llvm.org>> wrote:
> clayborg added a comment.
> 
> FYI: my approach to getting symbols to load was a bit different. I always let 
> a simple PlaceholderModule be created, but I played some tricks in the 
> GetObjectFile() method if someone had setting symbols for the module with 
> "target symbols add ..". I will attach my PlaceholderModule so you can see 
> what I had done. Since these modules always must be associated with a target, 
> I keep a weak pointer to the target in the constructor. Then later, if 
> someone does "target symbols add ..." the module will have Module:: 
> m_symfile_spec filled in, so I set the m_platform_file to be m_file, and then 
> move m_symfile_spec into m_file and then call Module::GetObjectFile(). This 
> is nice and clean because you don't have to make up fake symbols to fake 
> sections. When you create the placeholder module it needs the target:
> 
>   auto placeholder_module =
>   std::make_shared(module_spec, GetTarget());
> 
> Here is the copy of the PlaceholderModule that does what was discussed above:
> 
>   //--
>   /// 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 ModuleSpec _spec, Target& target) :
>   Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()),
>   m_target_wp(target.shared_from_this()),
>   m_base_of_image(LLDB_INVALID_ADDRESS), m_size_of_image(0) {
>   if (module_spec.GetUUID().IsValid())
> SetUUID(module_spec.GetUUID());
> }
> 
> // Creates a synthetic module section covering the whole module image (and
> // sets the section load address as well)
> void CreateImageSection(const MinidumpModule *module) {
>   m_base_of_image = module->base_of_image;
>   m_size_of_image = module->size_of_image;
>   TargetSP target_sp = m_target_wp.lock();
>   if (!target_sp)
> return;
>   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_sp->GetSectionLoadList().SetSectionLoadAddress(
>   section_sp, module->base_of_image);
> }
> 
> ObjectFile *GetObjectFile() override {
>   // Since there is no object file for these place holder modules, check
>   // if the symbol file spec has been set, and if so, then transfer it 
> over
>   // to the file spec 

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

> How large is the PDB file here?

~100kb


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

I can see how this works for the PDB, no-module-binary case. What about the
PDB & module-binary case?

Maybe I missed the rationale, but I think that modeling the general case:
module and symbols are separate files, is a better foundation for the
particular case where the symbols are embedded in the binary file.


On Mon, Dec 10, 2018 at 2:48 PM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> FYI: my approach to getting symbols to load was a bit different. I always
> let a simple PlaceholderModule be created, but I played some tricks in the
> GetObjectFile() method if someone had setting symbols for the module with
> "target symbols add ..". I will attach my PlaceholderModule so you can see
> what I had done. Since these modules always must be associated with a
> target, I keep a weak pointer to the target in the constructor. Then later,
> if someone does "target symbols add ..." the module will have Module::
> m_symfile_spec filled in, so I set the m_platform_file to be m_file, and
> then move m_symfile_spec into m_file and then call Module::GetObjectFile().
> This is nice and clean because you don't have to make up fake symbols to
> fake sections. When you create the placeholder module it needs the target:
>
>   auto placeholder_module =
>   std::make_shared(module_spec, GetTarget());
>
> Here is the copy of the PlaceholderModule that does what was discussed
> above:
>
>   //--
>   /// 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 ModuleSpec _spec, Target& target) :
>   Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()),
>   m_target_wp(target.shared_from_this()),
>   m_base_of_image(LLDB_INVALID_ADDRESS), m_size_of_image(0) {
>   if (module_spec.GetUUID().IsValid())
> SetUUID(module_spec.GetUUID());
> }
>
> // Creates a synthetic module section covering the whole module image
> (and
> // sets the section load address as well)
> void CreateImageSection(const MinidumpModule *module) {
>   m_base_of_image = module->base_of_image;
>   m_size_of_image = module->size_of_image;
>   TargetSP target_sp = m_target_wp.lock();
>   if (!target_sp)
> return;
>   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_sp->GetSectionLoadList().SetSectionLoadAddress(
>   section_sp, module->base_of_image);
> }
>
> ObjectFile *GetObjectFile() override {
>   // Since there is no object file for these place holder modules,
> check
>   // if the symbol file spec has been set, and if so, then transfer it
> over
>   // to the file spec so the module can make a real object file out of
> it.
>   if (m_symfile_spec) {
> // We need to load the sections once. We check of m_objfile_sp is
> valid.
> // If it is, we already have loaded the sections. If it isn't, we
> will
> // load the sections.
> const bool load_sections = !m_objfile_sp;
> if (load_sections) {
>   m_platform_file = m_file;
>   m_file = m_symfile_spec;
> }
> ObjectFile *obj_file = Module::GetObjectFile();
> if (load_sections && obj_file) {
>   TargetSP target_sp = m_target_wp.lock();
>   // The first time we create the object file from the external
> symbol
>   // file, we must load its sections and unload the ".module_image"
>   // section
>   bool changed;
>   SetLoadAddress(*target_sp, m_base_of_image, 

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

FYI: my approach to getting symbols to load was a bit different. I always let a 
simple PlaceholderModule be created, but I played some tricks in the 
GetObjectFile() method if someone had setting symbols for the module with 
"target symbols add ..". I will attach my PlaceholderModule so you can see what 
I had done. Since these modules always must be associated with a target, I keep 
a weak pointer to the target in the constructor. Then later, if someone does 
"target symbols add ..." the module will have Module:: m_symfile_spec filled 
in, so I set the m_platform_file to be m_file, and then move m_symfile_spec 
into m_file and then call Module::GetObjectFile(). This is nice and clean 
because you don't have to make up fake symbols to fake sections. When you 
create the placeholder module it needs the target:

  auto placeholder_module =
  std::make_shared(module_spec, GetTarget());

Here is the copy of the PlaceholderModule that does what was discussed above:

  //--
  /// 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 ModuleSpec _spec, Target& target) :
  Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()),
  m_target_wp(target.shared_from_this()),
  m_base_of_image(LLDB_INVALID_ADDRESS), m_size_of_image(0) {
  if (module_spec.GetUUID().IsValid())
SetUUID(module_spec.GetUUID());
}
  
// Creates a synthetic module section covering the whole module image (and
// sets the section load address as well)
void CreateImageSection(const MinidumpModule *module) {
  m_base_of_image = module->base_of_image;
  m_size_of_image = module->size_of_image;
  TargetSP target_sp = m_target_wp.lock();
  if (!target_sp)
return;
  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_sp->GetSectionLoadList().SetSectionLoadAddress(
  section_sp, module->base_of_image);
}
  
ObjectFile *GetObjectFile() override {
  // Since there is no object file for these place holder modules, check
  // if the symbol file spec has been set, and if so, then transfer it over
  // to the file spec so the module can make a real object file out of it.
  if (m_symfile_spec) {
// We need to load the sections once. We check of m_objfile_sp is valid.
// If it is, we already have loaded the sections. If it isn't, we will
// load the sections.
const bool load_sections = !m_objfile_sp;
if (load_sections) {
  m_platform_file = m_file;
  m_file = m_symfile_spec;
}
ObjectFile *obj_file = Module::GetObjectFile();
if (load_sections && obj_file) {
  TargetSP target_sp = m_target_wp.lock();
  // The first time we create the object file from the external symbol
  // file, we must load its sections and unload the ".module_image"
  // section
  bool changed;
  SetLoadAddress(*target_sp, m_base_of_image, false, changed);
}
return obj_file;
  }
  return nullptr;
}
  
SectionList *GetSectionList() override {
  return Module::GetUnifiedSectionList();
}
  protected:
// In case we need to load the sections in 
PlaceholderModule::GetObjectFile()
// after a symbol file has been specified, we might need the target.
lldb::TargetWP m_target_wp;
lldb::addr_t m_base_of_image;
lldb::addr_t m_size_of_image;
  };


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

https://reviews.llvm.org/D55142

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Greg Clayton via lldb-commits]

> On Dec 10, 2018, at 11:23 AM, Leonard Mosescu  wrote:
> 
> > BTW: check my changes in: https://reviews.llvm.org/D55522 
> > 
> > It will be interesting to you since it parses the linux maps info if it is 
> > available in breakpad generated minidump files. This will give us enough 
> > info to create correct sections for object files when we have no ELF file 
> > or no symbol file (ELF/DWARF or breakpad).  
> 
> Looks interesting, thanks for pointing it out. From a quick glance it seems 
> that your change would complement the generic support I'm adding here.

Indeed it will. Part of my patch was a hook in the placeholder module 
GetObjectFile() where if someone set the symbol file for it via "target symbol 
add ..." it would replace the "m_file" in the module with the symbol file and 
then get the object file from the symbol file that was added.

So it is a different approach to yours. I will attach my copy of 
"PlaceholderModule" to your patch so you can see what I did.

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

> How large is the PDB file here?
100Kb

On Mon, Dec 10, 2018 at 11:48 AM Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added a comment.
>
> How large is the PDB file here?
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

How large is the PDB file here?


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 177576.

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

https://reviews.llvm.org/D55142

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,59 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile& obj_file,
+llvm::BumpPtrAllocator ) {
   using namespace llvm::object;
-  auto expected_binary = createBinary(exe_path);
 
-  // If the file isn't a PE/COFF executable, fail.
-  if (!expected_binary) {
-llvm::consumeError(expected_binary.takeError());
-return nullptr;
-  }
-  OwningBinary binary = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file.GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);
 
-  // If it doesn't have a debug directory, fail.
-  llvm::StringRef pdb_file;
-  auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
-  if (ec)
-return nullptr;
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
+  // If it doesn't have a debug directory, fail.
+  return nullptr;
+}
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+// If the file isn't a PE/COFF executable, look for the PDB in the
+// current directory. This provides a basic solution for debugging minidumps
+// although it's only a stop-gap (until we implement a SymbolVendor)
+//
+// TODO: Implement a SymbolVendor for PDBs.
+//
+llvm::consumeError(expected_binary.takeError());
+pdb_file = obj_file.GetFileSpec()
+   .GetFileNameStrippingExtension()
+   .GetStringRef()
+   .str();
+pdb_file += ".pdb";
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file.GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(guid) + 4) // CvRecordPdb70
+  return nullptr;
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  }
 
   // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
   // fail.
   llvm::file_magic magic;
-  ec = llvm::identify_magic(pdb_file, magic);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +167,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +548,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+loadMatchingPDBFile(*m_obj_file, m_allocator);
 
 if (!file_up) {
   auto module_sp = m_obj_file->GetModule();
@@ -1459,11 +1486,10 @@
 llvm::Optional modi = m_index->GetModuleIndexForVa(file_addr);
 if (!modi)
   return 0;
-CompilandIndexItem *cci = 

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks fine to me. The test is a bit more coarse-grained than I'd like, 
though I can't think of a substantially better approach right now. One of the 
pdb folks should ok this too.




Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:386
 
-  for (lldb::tid_t tid = 0; tid < num_threads; ++tid) {
+  for (lldb::tid_t t_index = 0; t_index < num_threads; ++t_index) {
 llvm::ArrayRef context;

Since you're changing this, you might as well change the type to something more 
appropriate (size_t?).


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via lldb-commits]

> BTW: check my changes in: https://reviews.llvm.org/D55522
> It will be interesting to you since it parses the linux maps info if it
is available in breakpad generated minidump files. This will give us enough
info to create correct sections for object files when we have no ELF file
or no symbol file (ELF/DWARF or breakpad).

Looks interesting, thanks for pointing it out. From a quick glance it seems
that your change would complement the generic support I'm adding here.

On Mon, Dec 10, 2018 at 10:32 AM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> BTW: check my changes in: https://reviews.llvm.org/D55522
> It will be interesting to you since it parses the linux maps info if it is
> available in breakpad generated minidump files. This will give us enough
> info to create correct sections for object files when we have no ELF file
> or no symbol file (ELF/DWARF or breakpad).
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/
>
> https://reviews.llvm.org/D55142
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

BTW: check my changes in: https://reviews.llvm.org/D55522
It will be interesting to you since it parses the linux maps info if it is 
available in breakpad generated minidump files. This will give us enough info 
to create correct sections for object files when we have no ELF file or no 
symbol file (ELF/DWARF or breakpad).


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:171-174
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+

labath wrote:
> Mainly out of curiosity, what's the complication here? The llvm interface 
> does not provide the means to retrieve the age?
Yes, the interface doesn't expose the age, although that's not not really the 
problem (the age is a detail - part of the generic "Debug ID")

We just need to make the handling of PE/COFF & VC UUIDs more consistent: right 
now most places expect a stripped version (just the GUID). This is wrong since 
a correct PDB match should take the age into account, but it's outside the 
scope of this change.


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 177329.
lemo marked 3 inline comments as done.
lemo added a comment.

Incorporating feedback + adding a test case


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

https://reviews.llvm.org/D55142

Files:
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
  lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
  lit/Minidump/Windows/Sigsegv/sigsegv.test
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,59 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile& obj_file,
+llvm::BumpPtrAllocator ) {
   using namespace llvm::object;
-  auto expected_binary = createBinary(exe_path);
 
-  // If the file isn't a PE/COFF executable, fail.
-  if (!expected_binary) {
-llvm::consumeError(expected_binary.takeError());
-return nullptr;
-  }
-  OwningBinary binary = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file.GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);
 
-  // If it doesn't have a debug directory, fail.
-  llvm::StringRef pdb_file;
-  auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
-  if (ec)
-return nullptr;
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
+  // If it doesn't have a debug directory, fail.
+  return nullptr;
+}
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+// If the file isn't a PE/COFF executable, look for the PDB in the
+// current directory. This provides a basic solution for debugging minidumps
+// although it's only a stop-gap (until we implement a SymbolVendor)
+//
+// TODO: Implement a SymbolVendor for PDBs.
+//
+llvm::consumeError(expected_binary.takeError());
+pdb_file = obj_file.GetFileSpec()
+   .GetFileNameStrippingExtension()
+   .GetStringRef()
+   .str();
+pdb_file += ".pdb";
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file.GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(guid) + 4) // CvRecordPdb70
+  return nullptr;
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  }
 
   // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
   // fail.
   llvm::file_magic magic;
-  ec = llvm::identify_magic(pdb_file, magic);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +167,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +548,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+loadMatchingPDBFile(*m_obj_file, m_allocator);
 
 if (!file_up) {
   auto module_sp = m_obj_file->GetModule();
@@ -1459,11 +1486,10 @@
 llvm::Optional modi = 

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D55142#1320447 , @lemo wrote:

> I agree with both comments. The intention is to add some tests but I wanted 
> to get the review out early to surface concerns, if any. I also needed more 
> time to investigate a few complex failures uncovered by this change (ex. 
> https://bugs.llvm.org/show_bug.cgi?id=39882 and 
> https://bugs.llvm.org/show_bug.cgi?id=39897)
>
> Yes, this change can also be split in three parts: the reason it's bundled up 
> in this review is that all three parts are required to enable the basic 
> functionality (and overall it's a relatively small change). Maybe it was 
> better if I sent out the parts separately, but right now I'd like to preserve 
> the continuity in the review comments. 
>  I'm about to send out a new revision and once this review satisfies all the 
> comments I'll split it out and send individual reviews.


Sounds good. I think it would be nice to see the isolated patches standing next 
to their tests.




Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:134-136
 lldb::SectionSP section_sp(new Section(
 shared_from_this(), // Module to which this section belongs.
+GetObjectFile(),// ObjectFile

I don't know whether you'll run into any problems because of this, but the way 
this works for "normal" object files is that the each object file has a list of 
"own" sections, and then the Module has a "unified" list, which contains the 
sections of the main object file possibly combined with sections from the 
symbol object files. Here you are adding a section to the unified list without 
adding it to the object file, which is a bit nonstandard. I think it would be 
better to just add it to both places.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:106
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+llvm::BumpPtrAllocator ) {

lemo wrote:
> zturner wrote:
> > Perhaps `obj_file` should be a reference just to clarify that it can't be 
> > null.
> That would make sense. Unfortunately, obj_file can't be const 
> (ObjectFile::GetUUID() is non const and it's not easy to surgically fix it)
> 
> So we'd have to pass by non-const ref, which would read "out-parameter", 
> which IMO is more confusing than the non-null part is worth.
I don't think the `ObjectFile&` would be an out-parameter any more than the 
existing `BumpPtrAllocator&` is an out-parameter. So making this a reference 
would also make things locally consistent.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:150
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(llvm::codeview::GUID) + 4) // CvRecordPdb70
+  return nullptr;

make this `sizeof(guid)` for consistency.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:171-174
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+

Mainly out of curiosity, what's the complication here? The llvm interface does 
not provide the means to retrieve the age?


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Mark Mentovai via Phabricator via lldb-commits]

markmentovai added a comment.

This seems like a step in the right direction.


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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 176853.
lemo marked an inline comment as done.

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

https://reviews.llvm.org/D55142

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,59 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+llvm::BumpPtrAllocator ) {
   using namespace llvm::object;
-  auto expected_binary = createBinary(exe_path);
 
-  // If the file isn't a PE/COFF executable, fail.
-  if (!expected_binary) {
-llvm::consumeError(expected_binary.takeError());
-return nullptr;
-  }
-  OwningBinary binary = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);
 
-  // If it doesn't have a debug directory, fail.
-  llvm::StringRef pdb_file;
-  auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
-  if (ec)
-return nullptr;
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
+  // If it doesn't have a debug directory, fail.
+  return nullptr;
+}
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+// If the file isn't a PE/COFF executable, look for the PDB in the
+// current directory. This provides a basic solution for debugging minidumps
+// although it's only a stop-gap (until we implement a SymbolVendor)
+//
+// TODO: Implement a SymbolVendor for PDBs.
+//
+llvm::consumeError(expected_binary.takeError());
+pdb_file = obj_file->GetFileSpec()
+   .GetFileNameStrippingExtension()
+   .GetStringRef()
+   .str();
+pdb_file += ".pdb";
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file->GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+if (uuid_bytes.size() != sizeof(llvm::codeview::GUID) + 4) // CvRecordPdb70
+  return nullptr;
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  }
 
   // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
   // fail.
   llvm::file_magic magic;
-  ec = llvm::identify_magic(pdb_file, magic);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +167,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +548,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+loadMatchingPDBFile(m_obj_file, m_allocator);
 
 if (!file_up) {
   auto module_sp = m_obj_file->GetModule();
@@ -1459,11 +1486,10 @@
 llvm::Optional modi = m_index->GetModuleIndexForVa(file_addr);
 if (!modi)
   return 0;
-CompilandIndexItem *cci = m_index->compilands().GetCompiland(*modi);
-if (!cci)
+if (!m_index->compilands().HasCompilandInfo(*modi))
   return 0;
-
-sc.comp_unit = GetOrCreateCompileUnit(*cci).get();
+

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked 9 inline comments as done.
lemo added a comment.

In D55142#1316153 , @labath wrote:

> I don't see any tests :(.
>  Also, the three bullet points in the description sound like rather 
> independent pieces of functionality. Would it be possible to split them up 
> into separate patches? That would make things easier to review, particularly 
> for those who don't look at this code very often :).


I agree with both comments. The intention is to add some tests but I wanted to 
get the review out early to surface concerns, if any. I also needed more time 
to investigate a few complex failures uncovered by this change (ex. 
https://bugs.llvm.org/show_bug.cgi?id=39882 and 
https://bugs.llvm.org/show_bug.cgi?id=39897)

Yes, this change can also be split in three parts: the reason it's bundled up 
in this review is that all three parts are required to enable the basic 
functionality (and overall it's a relatively small change). Maybe it was better 
if I sent out the parts separately, but right now I'd like to preserve the 
continuity in the review comments. 
I'm about to send out a new revision and once this review satisfies all the 
comments I'll split it out and send individual reviews.




Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:132-135
+if (so.segment == 0) {
+  lldbassert(so.offset == 0);
+  continue;
+}

zturner wrote:
> What kind of symbols have a segment and offset of 0?
68 | S_LDATA32 [size = 44] `trace_event_unique_atomic140`
 type = 0x0013 (__int64), addr = :
   112 | S_LDATA32 [size = 44] `trace_event_unique_atomic171`
 type = 0x0013 (__int64), addr = :
   156 | S_LDATA32 [size = 44] `trace_event_unique_atomic196`
 type = 0x0013 (__int64), addr = :
...

Tracked by : https://bugs.llvm.org/show_bug.cgi?id=39882




Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:150-157
 // The odds of an error in some function such as GetSegmentAndOffset or
 // MakeVirtualAddress are much higher than the odds of encountering bad
 // debug info, so assert that this item was inserted in the map as opposed
 // to having already been there.
-lldbassert(insert_result.second);
+//
+// TODO: revisit this check since it fires for apparently valid PDBs
+//

zturner wrote:
> If we're going to comment it out, then just delete the code IMO.
> 
> Do you have some llvm-pdbutil output that demonstrates this on a valid PDB?  
> I guess we'd be looking for 2 symbols with the same Virtual Address.  Seems 
> odd to have that, but maybe an example of where it's happening would make it 
> clear whether this is actually a valid case.
I spent more time to investigate this and opened a bug to track it: 
https://bugs.llvm.org/show_bug.cgi?id=39897

With very large PDBs it's not easy to have a definitive verdict so a 2nd 
opinion is welcome, but as I noted in the bug is seems that ICF is one case 
where we can end up with multiple symbols at the same address. This means that 
the data structure needs to be revisited.

For now, I think it's better to suppress the assert so we can exercise the rest 
of the native PDB reader (I updated the comment to point to the new bug I just 
opened)



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:106
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+llvm::BumpPtrAllocator ) {

zturner wrote:
> Perhaps `obj_file` should be a reference just to clarify that it can't be 
> null.
That would make sense. Unfortunately, obj_file can't be const 
(ObjectFile::GetUUID() is non const and it's not easy to surgically fix it)

So we'd have to pass by non-const ref, which would read "out-parameter", which 
IMO is more confusing than the non-null part is worth.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:114-115
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);

zturner wrote:
> Instead of trying to load it as a PE/COFF fail and then trying something else 
> if it fails (hoping that it's a minidump), perhaps we could use 
> `llvm::identify_magic()` to figure out what it is actually is first.  That 
> function currently cannot detect a Windows minidump, but we coudl teach it to 
> support that.  I think that would make this code more logical.  
> 
> ```
> if (type == exe) {
> } else if (type == minidump) {
> } else {
>   // actual error
> }
> ```
We 

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't see any tests :(.

Also, the three bullet points in the description sound like rather independent 
pieces of functionality. Would it be possible to split them up into separate 
patches? That would make things easier to review, particularly for those who 
don't look at this code very often :).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:60
+  void Dump(Stream *s) override { *s << "PlaceholderObjectFile\n"; }
+  uint32_t GetAddressByteSize() const override { return 0; }
+  uint32_t GetDependentModules(FileSpecList _list) override { return 0; }

Should we return something valid for this function?



Comment at: source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h:92
+
+  bool HasIndexForModule(uint16_t modi) const;
 };

This method name is a little bit confusing.  Is it referring to a numeric index 
like 1, 2, 3, 4, 5.  Or a `CompilandIndexItem`?  I would call this something 
like `HasCompilandInfo(uint16_t modi)`



Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:132-135
+if (so.segment == 0) {
+  lldbassert(so.offset == 0);
+  continue;
+}

What kind of symbols have a segment and offset of 0?



Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:150-157
 // The odds of an error in some function such as GetSegmentAndOffset or
 // MakeVirtualAddress are much higher than the odds of encountering bad
 // debug info, so assert that this item was inserted in the map as opposed
 // to having already been there.
-lldbassert(insert_result.second);
+//
+// TODO: revisit this check since it fires for apparently valid PDBs
+//

If we're going to comment it out, then just delete the code IMO.

Do you have some llvm-pdbutil output that demonstrates this on a valid PDB?  I 
guess we'd be looking for 2 symbols with the same Virtual Address.  Seems odd 
to have that, but maybe an example of where it's happening would make it clear 
whether this is actually a valid case.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:106
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+llvm::BumpPtrAllocator ) {

Perhaps `obj_file` should be a reference just to clarify that it can't be null.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:114-115
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);

Instead of trying to load it as a PE/COFF fail and then trying something else 
if it fails (hoping that it's a minidump), perhaps we could use 
`llvm::identify_magic()` to figure out what it is actually is first.  That 
function currently cannot detect a Windows minidump, but we coudl teach it to 
support that.  I think that would make this code more logical.  

```
if (type == exe) {
} else if (type == minidump) {
} else {
  // actual error
}
```


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This looks good to me, save for a couple comment nits.




Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:155
+//
+// TODO: revisit this check since it fires for apparently valid PDBs
+//

Every apparently valid PDB or certain ones?  If it's particular ones, then 
perhaps we should create a bug report with a repro to make it easier to 
investigate in the future.



Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:133
+// TODO: If the file isn't a PE/COFF executable, look for the PDB in the
+//  current directly. This provides a basic solution for debugging 
minidumps
+//  although it's only a stop-gap until we implement a proper SymbolVendor

Did you mean "directory" instead of "directly"?

Also, this is worded as a TODO, but it describes the current situation.  
Describing the current situation is fine, but I'd like the TODO to more clearly 
say what's do be done.  I gather that's "implement a proper SymbolVendor."


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55142



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: labath, zturner, amccarth.
lemo added a project: LLDB.
Herald added subscribers: jfb, abidh, arphaman.

A few changes required to enable the use of the native PDB reader when 
debugging minidumps:

1. Consume PDBs even if the module binary is not available Implementing a 
PlaceholderObjectFile to complement the existing PlaceholderModule

2. Use the minidump exception record if present

3. Relax the PDB file search As noted in the comments this is a stop-gap until 
we add a proper SymbolVendor

Overall this is a modest step towards improving the minidump debugging 
experience. But more importantly,
it enables the basic consumption of PDBs using the NativePDB(TM) reader as a 
foundation to add more functionality incrementally.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D55142

Files:
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp
  source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/PdbIndex.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -30,6 +30,7 @@
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -102,33 +103,53 @@
 }
 
 static std::unique_ptr
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator ) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+llvm::BumpPtrAllocator ) {
   using namespace llvm::object;
-  auto expected_binary = createBinary(exe_path);
 
-  // If the file isn't a PE/COFF executable, fail.
-  if (!expected_binary) {
-llvm::consumeError(expected_binary.takeError());
-return nullptr;
-  }
-  OwningBinary binary = std::move(*expected_binary);
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
 
-  auto *obj = llvm::dyn_cast(binary.getBinary());
-  if (!obj)
-return nullptr;
-  const llvm::codeview::DebugInfo *pdb_info = nullptr;
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
+OwningBinary binary = std::move(*expected_binary);
 
-  // If it doesn't have a debug directory, fail.
-  llvm::StringRef pdb_file;
-  auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
-  if (ec)
-return nullptr;
+auto *obj = llvm::dyn_cast(binary.getBinary());
+if (!obj)
+  return nullptr;
+const llvm::codeview::DebugInfo *pdb_info = nullptr;
+
+// If it doesn't have a debug directory, fail.
+llvm::StringRef pdb_file_name_ref;
+if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref))
+  return nullptr;
+pdb_file = pdb_file_name_ref;
+
+// Extract the debug GUID from the debug directory
+memcpy(, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+// TODO: If the file isn't a PE/COFF executable, look for the PDB in the
+//  current directly. This provides a basic solution for debugging minidumps
+//  although it's only a stop-gap until we implement a proper SymbolVendor
+//  for PDBs.
+llvm::consumeError(expected_binary.takeError());
+pdb_file =
+obj_file->GetFileSpec().GetFileNameStrippingExtension().GetCString();
+pdb_file += ".pdb";
+
+// Extract the debug GUID from the ObjectFile
+lldb_private::UUID uuid;
+obj_file->GetUUID();
+auto uuid_bytes = uuid.GetBytes();
+lldbassert(uuid_bytes.size() == sizeof(llvm::codeview::GUID) + 4);
+memcpy(, uuid_bytes.data(), sizeof(guid));
+  }
 
   // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
   // fail.
   llvm::file_magic magic;
-  ec = llvm::identify_magic(pdb_file, magic);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
 return nullptr;
   std::unique_ptr pdb = loadPDBFile(pdb_file, allocator);
@@ -140,11 +161,11 @@
 llvm::consumeError(expected_info.takeError());
 return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
 return nullptr;
+
   return pdb;
 }
 
@@ -521,7 +542,7 @@
   if (!m_index) {
 // Lazily load and match the PDB file, but only do this once.
 std::unique_ptr file_up =
-