Re: [Lldb-commits] [lldb] r313210 - Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Leonard Mosescu via lldb-commits]

Yes, this is the culprit indeed, thanks Ed for reporting it!

The fix is trivial and I can take care of it on Monday, but if anyone is
blocked by it we should revert the change.

On Sat, Sep 16, 2017 at 5:21 AM, Ed Maste via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> On 13 September 2017 at 18:57, Adrian McCarthy via lldb-commits
>  wrote:
> > Author: amccarth
> > Date: Wed Sep 13 15:57:11 2017
> > New Revision: 313210
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=313210=rev
> > Log:
> > Fix for bug 34532 - A few rough corners related to post-mortem debugging
> (core/minidump)
>
> I suspect this was responsible for breaking debugging on FreeBSD:
> ```
> % bin/lldb /bin/ls
> (lldb) target create "/bin/ls"
> Current executable set to '/bin/ls' (x86_64).
> (lldb) b main
> Breakpoint 1: where = ls`main + 33 at ls.c:163, address =
> 0x004023f1
> (lldb) run
> Process 42854 launching
> error: process resume at entry point failed: error: freebsd does not
> support resuming processes
> (lldb)
> ```
>
> I haven't bisected yet to confirm, just reviewed changes between
> rL312270 (working) and rL313327 (failing). It looks like the fix may
> be straightforward, but I'm traveling to a conference and won't be
> able to look for a short while so am noting it here.
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

It's a good question - here's a two part answer:

1. The actual printing of the output is the longest blocking in many cases
(as mentioned in the description: the actual data gathering for "target
module dump symtab" can take 1-2sec, but printing it can take 20min. For
quick experiment, try dis -p -c 1).
2. This change provides the scaffolding for cooperative interruption that
can be used were appropriate, not just the printing part. I did this for
"target" commands (see the changes in CommandObjectTarget.cpp), and it's
very easy to do the same in other places as needed.




On Mon, Sep 18, 2017 at 3:05 PM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> I think Zach might be correct. If we already gathered the data, why not
> just output it all? Lets answer this first before returning to which
> implementation to use when parsing it up into lines and dumping it. Seems a
> shame to throw any data away.
>
>
> https://reviews.llvm.org/D37923
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Thanks for the context.

On Tue, Sep 19, 2017 at 11:58 AM, Jim Ingham  wrote:

>
> > On Sep 19, 2017, at 11:25 AM, Leonard Mosescu 
> wrote:
> >
> > These are all great suggestions, thanks everyone!
> >
> > > We should have a test. The test would need to use pexpect or something
> similar. If anyone has any pointer on how to make a test for this, please
> chime in. I was thinking just a pexpect based test.
> > I love tests but what exactly do we want to test here? Sorry if I'm
> missing something obvious - are you talking about splitting a string into
> chunks? The actual interruption? ...
> >
> > Also, I like the idea of exposing this though the SB APIs, but that's a
> significant expansion of the original goal which is to improve the
> interactive LLDB debugger.
>
> This is sort of a side point, but one of the goals of lldb is to make the
> command set extensible by adding commands implemented with the SB API's
> either from Python or from C++ (Python is more common but both are
> possible.)  And there are a bunch of groups here at Apple that have all
> sorts of special purpose LLDB commands implemented this way.  The xnu.py
> and Co. that gets distributed with the kernel development kit has a wide
> array of examples of this that are publicly available if you want to have a
> look.
>
> So the SB API's are in fact an intrinsic part of the interactive LLDB
> debugger.  We aren't there yet (there isn't an SB API to define command
> options & arguments so SB added commands don't parse like lldb commands do)
> but the end goal is that it you can't tell whether commands are from SB
> API's or are builtin.
>
> BTW, I think the fact that we use lldb_private API's is an unfortunate
> artifact of how lldb was implemented.  Having to use our vended API's to
> implement the commands would make them better implemented - since we would
> really be on the hook for them, and would reduce a lot of code duplication
> where we do things one way in the commands and slightly differently in the
> equivalent SB API's, and reduce the testing surface considerably.  I'm not
> sure this is a project we will ever actually get to, but it is a good
> aspirational goal to keep in mind.
>
> We didn't do it that way originally because we needed something we could
> poke at early on in the development of lldb, at a point where it would have
> been premature to design the SB API's.  Then lldb went from skunkworks
> project to mission critical debugger too quickly for us to be able to
> revise the decision.
>
> Jim
>
>
>
> > It may be nice looking at SB APIs later on, but I'm afraid that trying
> to pile up everything in one change is just going to spiral the complexity
> out of control.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner 
> wrote:
> > That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
> >
> > If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > I'd really prefer to do this as/along with an SB API test since we also
> need commands made through the SB API to be interruptible, and we should
> test that that also works.  So this kills two birds with one stone.
> >
> > In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > >
> > > zturner added a comment.
> > >
> > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > >
> > >> We should have a test. The test would need to use pexpect or
> something similar. If anyone has any pointer on how to make a test for
> this, please chime in. I was thinking just a pexpect based test.
> > >
> > >
> > > This kind of thing is perfect for a unit test, but I'm not sure how
> easy that would be with the current design.  You'd basically do something
> like:
> > >
> > >  struct MockStream {
> > >explicit MockStream(CommandInterpreter , int
> InterruptAfter)
> > >  : CommandInterpreter(Interpreter),
> 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Any pointers on the steps required to make changes to the SB APIs? Is it
documented anywhere? Thanks!

On Tue, Sep 19, 2017 at 11:58 AM, Jim Ingham  wrote:

>
> > On Sep 19, 2017, at 11:25 AM, Leonard Mosescu 
> wrote:
> >
> > These are all great suggestions, thanks everyone!
> >
> > > We should have a test. The test would need to use pexpect or something
> similar. If anyone has any pointer on how to make a test for this, please
> chime in. I was thinking just a pexpect based test.
> > I love tests but what exactly do we want to test here? Sorry if I'm
> missing something obvious - are you talking about splitting a string into
> chunks? The actual interruption? ...
> >
> > Also, I like the idea of exposing this though the SB APIs, but that's a
> significant expansion of the original goal which is to improve the
> interactive LLDB debugger.
>
> This is sort of a side point, but one of the goals of lldb is to make the
> command set extensible by adding commands implemented with the SB API's
> either from Python or from C++ (Python is more common but both are
> possible.)  And there are a bunch of groups here at Apple that have all
> sorts of special purpose LLDB commands implemented this way.  The xnu.py
> and Co. that gets distributed with the kernel development kit has a wide
> array of examples of this that are publicly available if you want to have a
> look.
>
> So the SB API's are in fact an intrinsic part of the interactive LLDB
> debugger.  We aren't there yet (there isn't an SB API to define command
> options & arguments so SB added commands don't parse like lldb commands do)
> but the end goal is that it you can't tell whether commands are from SB
> API's or are builtin.
>
> BTW, I think the fact that we use lldb_private API's is an unfortunate
> artifact of how lldb was implemented.  Having to use our vended API's to
> implement the commands would make them better implemented - since we would
> really be on the hook for them, and would reduce a lot of code duplication
> where we do things one way in the commands and slightly differently in the
> equivalent SB API's, and reduce the testing surface considerably.  I'm not
> sure this is a project we will ever actually get to, but it is a good
> aspirational goal to keep in mind.
>
> We didn't do it that way originally because we needed something we could
> poke at early on in the development of lldb, at a point where it would have
> been premature to design the SB API's.  Then lldb went from skunkworks
> project to mission critical debugger too quickly for us to be able to
> revise the decision.
>
> Jim
>
>
>
> > It may be nice looking at SB APIs later on, but I'm afraid that trying
> to pile up everything in one change is just going to spiral the complexity
> out of control.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner 
> wrote:
> > That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
> >
> > If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > I'd really prefer to do this as/along with an SB API test since we also
> need commands made through the SB API to be interruptible, and we should
> test that that also works.  So this kills two birds with one stone.
> >
> > In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > >
> > > zturner added a comment.
> > >
> > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > >
> > >> We should have a test. The test would need to use pexpect or
> something similar. If anyone has any pointer on how to make a test for
> this, please chime in. I was thinking just a pexpect based test.
> > >
> > >
> > > This kind of thing is perfect for a unit test, but I'm not sure how
> easy that would be with the current design.  You'd basically do something
> like:
> > >
> > >  struct MockStream {
> > >explicit 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Yes, using line endings as split points is somewhat arbitrary, anything
that's a reasonable compromise between interruption responsiveness and low
interrupt polling overhead would do. I feel that the lines are somewhat
nicer since arbitrary splitting may lead to confusion and/or formatting
ugliness.

On Wed, Sep 20, 2017 at 4:24 PM, Adrian McCarthy via Phabricator <
revi...@reviews.llvm.org> wrote:

> amccarth accepted this revision.
> amccarth added a comment.
>
> LGTM.
>
> But just a thought:  Is it worth doing all the work to scan for line
> endings for the interruption points?  What if, instead, it just printed the
> next _n_ characters on each iteration until the entire buffer has been
> printed.  Sure, sometimes an interruption will split a line, and sometimes
> it won't.  I'm not sure that's important for interactive usage.  It would
> mean less fiddly code, faster output (because you don't have to scan every
> character), and a zillion short lines will print as fast as a smaller
> number of longer lines that represents the same volume of text.
>
>
> https://reviews.llvm.org/D37923
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

This looks beautiful indeed. The problem is that it doesn't quite work with
the current MemoryBuffer and the line_iterator : for one thing there's no
way to construct a MemoryBuffer from a StringRef, or to use the
line_iterator directly with a StringRef.


On Tue, Sep 19, 2017 at 10:39 AM, Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added a comment.
>
> Give me a few more hours, if there's a way to make this work with
> `line_iterator` I'd really prefer that since I think it improves
> readability.  Can you confirm that if you were able to write:
>
>   auto begin = line_iterator(str, /* skip_empty_lines =*/ false);
>   auto end = line_iterator();
>   while (begin != end && !WasInterrupted()) {
> stream.Write(*begin);
> if (++begin != end)
>   stream.Write("\n");
>   }
>
> That this would be equivalent?
>
>
> https://reviews.llvm.org/D37923
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

These are all great suggestions, thanks everyone!

> We should have a test. The test would need to use pexpect or something
similar. If anyone has any pointer on how to make a test for this, please
chime in. I was thinking just a pexpect based test.
I love tests but what exactly do we want to test here? Sorry if I'm missing
something obvious - are you talking about splitting a string into chunks?
The actual interruption? ...

Also, I like the idea of exposing this though the SB APIs, but that's a
significant expansion of the original goal which is to improve the
interactive LLDB debugger. It may be nice looking at SB APIs later on, but
I'm afraid that trying to pile up everything in one change is just going to
spiral the complexity out of control.

On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner  wrote:

> That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
>
> If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
>
> On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
>
>> I'd really prefer to do this as/along with an SB API test since we also
>> need commands made through the SB API to be interruptible, and we should
>> test that that also works.  So this kills two birds with one stone.
>>
>> In general, when developing any high-level features in lldb one of the
>> questions you should ask yourself is whether this is useful at the SB API
>> layer.  In this case it obviously is (actually I'm a little embarrassed I
>> didn't think of this requirement).  If so, then it's simplest to test it at
>> that layer.  If the SB API is anything more than a pass-through, then you
>> might also want to write a unit test for the underlying C++ API's.  But in
>> this case the SB function will just call the underlying CommandInterpreter
>> one, so testing at the SB layer is sufficient.
>>
>> Jim
>>
>> > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>> >
>> > zturner added a comment.
>> >
>> > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
>> >
>> >> We should have a test. The test would need to use pexpect or something
>> similar. If anyone has any pointer on how to make a test for this, please
>> chime in. I was thinking just a pexpect based test.
>> >
>> >
>> > This kind of thing is perfect for a unit test, but I'm not sure how
>> easy that would be with the current design.  You'd basically do something
>> like:
>> >
>> >  struct MockStream {
>> >explicit MockStream(CommandInterpreter , int
>> InterruptAfter)
>> >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter)
>> {}
>> >
>> >CommandInterpreter 
>> >const int InterruptAfter;
>> >int Lines = 0;
>> >std::string Buffer;
>> >
>> >void Write(StringRef S) {
>> >  ++Lines;
>> >  if (Lines >= InterruptAfter) {
>> >Interpreter.Interrupt();
>> >return;
>> >  }
>> >  Buffer += S;
>> >}
>> >  };
>> >
>> >  TEST_F(CommandInterruption) {
>> >CommandInterpreter Interpreter;
>> >MockStream Stream(Interpreter, 3);
>> >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
>> >EXPECT_EQ(Stream.Lines == 3);
>> >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
>> >  }
>> >
>> >
>> > https://reviews.llvm.org/D37923
>> >
>> >
>> >
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Looking at line_iterator, it seems to be designed to work with
MemoryBuffer, which in turn seems specialized for dealing with file content
(so while it may be possible to force init a MemoryBuffer from a StringRef
it seems a bit awkward to me). Also, line_iterator has extra stuff which we
don't need here yet requires extra care (skipping empty lines and comments,
tracking line numbers...)


On Mon, Sep 18, 2017 at 3:18 PM, Zachary Turner  wrote:

>
>
> On Mon, Sep 18, 2017 at 3:13 PM Leonard Mosescu 
> wrote:
>
>> It's a good question - here's a two part answer:
>>
>> 1. The actual printing of the output is the longest blocking in many
>> cases (as mentioned in the description: the actual data gathering for
>> "target module dump symtab" can take 1-2sec, but printing it can take
>> 20min. For quick experiment, try dis -p -c 1).
>> 2. This change provides the scaffolding for cooperative interruption that
>> can be used were appropriate, not just the printing part. I did this for
>> "target" commands (see the changes in CommandObjectTarget.cpp), and it's
>> very easy to do the same in other places as needed.
>>
>>
> Makes sense.  Can you try `llvm::line_iterator` then instead of the
> hand-splitting?  See `llvm/Support/LineIterator.h`
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Hi Greg, are you referring to the manual line splitting vs. using
StringRef::split()? This is how it's documented:

/// Split into two substrings around the first occurrence of a separator
/// character.
///
/// If \p Separator is in the string, then the result is a pair (LHS, RHS)
/// such that (*this == LHS + Separator + RHS) is true and RHS is
/// maximal. If \p Separator is not in the string, then the result is a
/// pair (LHS, RHS) where (*this == LHS) and (RHS == "").

Yes, RHS == "" happens to be be implemented as a StringRef with nullptr
data, but this seems to make the implementation even more depended on the
subtle details. My point was that second == "" in two cases: separator is
not in the string AND separator is _last_ character in the string.

Did I get the question right?

On Mon, Sep 18, 2017 at 3:51 PM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> ok. Then back to the "can we use StringRef"? We might be able to check if
> the second.data() is NULL? It might be NULL if the string doesn't end with
> a newline. It it does, it might be empty but contain a pointer to the '\0'
> byte?
>
>
> https://reviews.llvm.org/D37923
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

I agree Jim. I'd like like to build thing incrementally - checking in the
current change as is does not preclude adding the SB APIs while it does
provide the foundation.

I think that going after the scenarios you mention is a significant
increase in scope. Are people even hooking up DispatchInterrupt() from
whatever interactive driver they use?

On Tue, Sep 19, 2017 at 11:27 AM, Jim Ingham  wrote:

> We agreed to forwards compatibility because people write big scripts that
> use the SB API, implement GUI's on top of them (more than just Xcode) etc.
> So we try not to jerk those folks around.  That adds a little more
> responsibility on our part to think carefully about what we add, but the
> notion that we should refrain from making useful functionality available
> because we'd rather not be beholden to our decisions seems really
> wrong-headed to me.
>
> And in this case there's a clear use for this. For instance the xnu macros
> have a bunch of Python based commands that spew out pages and pages of
> output.  Those guys would love to make their commands interruptible.  To do
> that they would need to call WasInterrupted.  So this is 100% something
> that should be available at the SB API layer.
>
> Jim
>
>
>
> > On Sep 19, 2017, at 11:18 AM, Zachary Turner  wrote:
> >
> > Also, it avoids polluting the SB interface with another function that
> probably nobody is ever going to use outside of testing.  Adding to the SB
> API should take an act of God, given that once it gets added it has to stay
> until the end of time.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner 
> wrote:
> > That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
> >
> > If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > I'd really prefer to do this as/along with an SB API test since we also
> need commands made through the SB API to be interruptible, and we should
> test that that also works.  So this kills two birds with one stone.
> >
> > In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > >
> > > zturner added a comment.
> > >
> > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > >
> > >> We should have a test. The test would need to use pexpect or
> something similar. If anyone has any pointer on how to make a test for
> this, please chime in. I was thinking just a pexpect based test.
> > >
> > >
> > > This kind of thing is perfect for a unit test, but I'm not sure how
> easy that would be with the current design.  You'd basically do something
> like:
> > >
> > >  struct MockStream {
> > >explicit MockStream(CommandInterpreter , int
> InterruptAfter)
> > >  : CommandInterpreter(Interpreter),
> InterruptAfter(InterruptAfter) {}
> > >
> > >CommandInterpreter 
> > >const int InterruptAfter;
> > >int Lines = 0;
> > >std::string Buffer;
> > >
> > >void Write(StringRef S) {
> > >  ++Lines;
> > >  if (Lines >= InterruptAfter) {
> > >Interpreter.Interrupt();
> > >return;
> > >  }
> > >  Buffer += S;
> > >}
> > >  };
> > >
> > >  TEST_F(CommandInterruption) {
> > >CommandInterpreter Interpreter;
> > >MockStream Stream(Interpreter, 3);
> > >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> > >EXPECT_EQ(Stream.Lines == 3);
> > >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> > >  }
> > >
> > >
> > > https://reviews.llvm.org/D37923
> > >
> > >
> > >
> >
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

So, how about I look into exposing WasInterrupted() through SB APIs in a
follow up change?

On Tue, Sep 19, 2017 at 11:36 AM, Jim Ingham  wrote:

> Xcode does, I don't know about other UI's.
>
> Jim
>
> > On Sep 19, 2017, at 11:35 AM, Leonard Mosescu 
> wrote:
> >
> > I agree Jim. I'd like like to build thing incrementally - checking in
> the current change as is does not preclude adding the SB APIs while it does
> provide the foundation.
> >
> > I think that going after the scenarios you mention is a significant
> increase in scope. Are people even hooking up DispatchInterrupt() from
> whatever interactive driver they use?
> >
> > On Tue, Sep 19, 2017 at 11:27 AM, Jim Ingham  wrote:
> > We agreed to forwards compatibility because people write big scripts
> that use the SB API, implement GUI's on top of them (more than just Xcode)
> etc.  So we try not to jerk those folks around.  That adds a little more
> responsibility on our part to think carefully about what we add, but the
> notion that we should refrain from making useful functionality available
> because we'd rather not be beholden to our decisions seems really
> wrong-headed to me.
> >
> > And in this case there's a clear use for this. For instance the xnu
> macros have a bunch of Python based commands that spew out pages and pages
> of output.  Those guys would love to make their commands interruptible.  To
> do that they would need to call WasInterrupted.  So this is 100% something
> that should be available at the SB API layer.
> >
> > Jim
> >
> >
> >
> > > On Sep 19, 2017, at 11:18 AM, Zachary Turner 
> wrote:
> > >
> > > Also, it avoids polluting the SB interface with another function that
> probably nobody is ever going to use outside of testing.  Adding to the SB
> API should take an act of God, given that once it gets added it has to stay
> until the end of time.
> > >
> > > On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner 
> wrote:
> > > That would work, but I think it's adding many more pieces to the
> test.  Now there's threads, a Python layer, and multiprocess dotest
> infrastructure in the equation.  Each providing an additional source of
> flakiness and instability.
> > >
> > > If all it is is a pass-through, then just calling the function it
> passes through to is a strictly better test, since it's focusing the test
> on the underlying piece of functionality and removing additional sources of
> failure and instability from the test.
> > >
> > > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > > I'd really prefer to do this as/along with an SB API test since we
> also need commands made through the SB API to be interruptible, and we
> should test that that also works.  So this kills two birds with one stone.
> > >
> > > In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
> > >
> > > Jim
> > >
> > > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > > >
> > > > zturner added a comment.
> > > >
> > > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > > >
> > > >> We should have a test. The test would need to use pexpect or
> something similar. If anyone has any pointer on how to make a test for
> this, please chime in. I was thinking just a pexpect based test.
> > > >
> > > >
> > > > This kind of thing is perfect for a unit test, but I'm not sure how
> easy that would be with the current design.  You'd basically do something
> like:
> > > >
> > > >  struct MockStream {
> > > >explicit MockStream(CommandInterpreter , int
> InterruptAfter)
> > > >  : CommandInterpreter(Interpreter),
> InterruptAfter(InterruptAfter) {}
> > > >
> > > >CommandInterpreter 
> > > >const int InterruptAfter;
> > > >int Lines = 0;
> > > >std::string Buffer;
> > > >
> > > >void Write(StringRef S) {
> > > >  ++Lines;
> > > >  if (Lines >= InterruptAfter) {
> > > >Interpreter.Interrupt();
> > > >return;
> > > >  }
> > > >  Buffer += S;
> > > >}
> > > >  };
> > > >
> > > >  TEST_F(CommandInterruption) {
> > > >CommandInterpreter Interpreter;
> > > >MockStream Stream(Interpreter, 3);
> > > >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> > > >EXPECT_EQ(Stream.Lines == 3);
> > > >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> > > >  }
> > > >
> > > >
> > > > 

[Lldb-commits] [lldb] r314856 - updating svn:ignore

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Tue Oct  3 15:30:02 2017
New Revision: 314856

URL: http://llvm.org/viewvc/llvm-project?rev=314856=rev
Log:
updating svn:ignore

Modified:
lldb/trunk/   (props changed)

Propchange: lldb/trunk/
--
--- svn:ignore (original)
+++ svn:ignore Tue Oct  3 15:30:02 2017
@@ -2,3 +2,5 @@ build
 intermediates
 llvm
 llvm-build
+.vscode
+


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

[Lldb-commits] [lldb] r315037 - Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Thu Oct  5 16:41:28 2017
New Revision: 315037

URL: http://llvm.org/viewvc/llvm-project?rev=315037=rev
Log:
Implement interactive command interruption

The core of this change is the new CommandInterpreter::m_command_state,
which models the state transitions for interactive commands, including
an "interrupted" state transition.

In general, command interruption requires cooperation from the code
executing the command, which needs to poll for interruption requests
through CommandInterpreter::WasInterrupted().

CommandInterpreter::PrintCommandOutput() implements an optionally
interruptible printing of the command output, which for large outputs 
was likely the longest blocking part.
(ex. target modules dump symtab on a complex binary could take 10+ minutes)

Differential Revision: https://reviews.llvm.org/D37923


Added:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/commands.txt
Modified:
lldb/trunk/include/lldb/API/SBCommandInterpreter.h
lldb/trunk/include/lldb/Core/IOHandler.h
lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h

lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/.lldb
lldb/trunk/scripts/interface/SBCommandInterpreter.i
lldb/trunk/source/API/SBCommandInterpreter.cpp
lldb/trunk/source/Commands/CommandObjectTarget.cpp
lldb/trunk/source/Core/Debugger.cpp
lldb/trunk/source/Interpreter/CommandInterpreter.cpp

Modified: lldb/trunk/include/lldb/API/SBCommandInterpreter.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/API/SBCommandInterpreter.h?rev=315037=315036=315037=diff
==
--- lldb/trunk/include/lldb/API/SBCommandInterpreter.h (original)
+++ lldb/trunk/include/lldb/API/SBCommandInterpreter.h Thu Oct  5 16:41:28 2017
@@ -165,6 +165,8 @@ public:
int match_start_point, int max_return_elements,
lldb::SBStringList );
 
+  bool WasInterrupted() const;
+
   // Catch commands before they execute by registering a callback that will
   // get called when the command gets executed. This allows GUI or command
   // line interfaces to intercept a command and stop it from happening

Modified: lldb/trunk/include/lldb/Core/IOHandler.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/IOHandler.h?rev=315037=315036=315037=diff
==
--- lldb/trunk/include/lldb/Core/IOHandler.h (original)
+++ lldb/trunk/include/lldb/Core/IOHandler.h Thu Oct  5 16:41:28 2017
@@ -195,7 +195,7 @@ public:
   enum class Completion { None, LLDBCommand, Expression };
 
   IOHandlerDelegate(Completion completion = Completion::None)
-  : m_completion(completion), m_io_handler_done(false) {}
+  : m_completion(completion) {}
 
   virtual ~IOHandlerDelegate() = default;
 
@@ -296,7 +296,6 @@ public:
 
 protected:
   Completion m_completion; // Support for common builtin completions
-  bool m_io_handler_done;
 };
 
 //--

Modified: lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h?rev=315037=315036=315037=diff
==
--- lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h (original)
+++ lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h Thu Oct  5 
16:41:28 2017
@@ -242,6 +242,8 @@ public:
  bool repeat_on_empty_command = true,
  bool no_context_switching = false);
 
+  bool WasInterrupted() const;
+
   //--
   /// Execute a list of commands in sequence.
   ///
@@ -522,6 +524,25 @@ private:
   StringList _help,
   CommandObject::CommandMap _map);
 
+  // An interruptible wrapper around the stream output
+  void PrintCommandOutput(Stream , llvm::StringRef str);
+
+  // A very simple state machine which models the command handling transitions
+  enum class CommandHandlingState {
+eIdle,
+eInProgress,
+eInterrupted,
+  };
+
+  std::atomic m_command_state{
+  CommandHandlingState::eIdle};
+
+  int m_iohandler_nesting_level = 0;
+
+  void StartHandlingCommand();
+  void FinishHandlingCommand();
+  bool InterruptCommand();
+
   Debugger _debugger; // The debugger session that this interpreter is
 // associated with
   ExecutionContextRef m_exe_ctx_ref; // The current execution context to use

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/.lldb
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/.lldb?rev=315037=315036=315037=diff

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Leonard Mosescu via lldb-commits]

+Mark Mentovai

On Mon, Sep 11, 2017 at 12:24 PM, Leonard Mosescu 
wrote:

> Thanks Jim, I'll give option #1 a try and upload a new patch.
>
>  think your divisions into options is too simplistic.  lldb is built of
>> many different subsystems, and a lot of "inconsistent state" can be
>> confined to one subsystem or other.  So for instance, if one CU's debug is
>> unrecoverably damaged, we can just pretend that that .o file was built w/o
>> debug info and the user can do lots of useful debugging still.  It is much
>> better if we can say "Found irrecoverable errors in this CU, dumping debug
>> info" and continue rather than crashing because one CU that the user may
>> not even care about is all horked up.  Similarly a single JIT execution can
>> get itself all balled up and yet if we can discard the work it did we can
>> continue doing lots of other things.
>
>
> The key here is the recoverable part - if you can isolate the state and
> recover (throw-away, rollback), then I agree, that can be a preferable
> approach. My point is about truly irrecoverable states - WTF states where
> you don't expect to get into, can't do anything meaningful in response and
> will likely cause cascading issues.
>
> As a side note, I would *love* a generic roll-back mechanism in LLDB. Not
> only that it can give us a viable alternative to failing fast, but it can
> also allow things like LLDB-command interruption (which is one thing I'd
> like to take a closer look when I get a chance)
>
> On Mon, Sep 11, 2017 at 10:55 AM, Jim Ingham  wrote:
>
>>
>> > On Sep 11, 2017, at 10:24 AM, Leonard Mosescu 
>> wrote:
>> >
>> > I think everyone is in agreement that input should be sanitized, and
>> ill-formed input should not trigger an abrupt termination if there's a way
>> to recover (which is not always feasible).
>> >
>> > The conversation started around fail-fast for inconsistent and
>> unrecoverable state, where I think the math is very clear in this case.
>> Using one the the comments about the long-lasting debugging sessions as
>> example, let's say that we have a bug and the debugger state gets corrupted
>> after 1h of debugging:
>> > 1. debugger crashes immediately: lost 1h of debugging -> angry bug
>> report (or ideally automated bug report)
>> > 2. debugger limps along for a few more hours behaving erratically (not
>> necessarily in an obvious fashion)
>> > a. developer gives up after many hours or debugging without
>> understanding what's going on
>> > b. developer figures out that the debugger is unreliable
>> >
>> > While it is choosing between two evils, I think #1 is clearly the right
>> choice: #2 results in more lost time and frustration, less chance of
>> getting a bug report and can result in a general impression that the
>> debugger is not to be trusted (which for a developer tool is infinitely
>> worse than an unstable tool)
>>
>> I think your divisions into options is too simplistic.  lldb is built of
>> many different subsystems, and a lot of "inconsistent state" can be
>> confined to one subsystem or other.  So for instance, if one CU's debug is
>> unrecoverably damaged, we can just pretend that that .o file was built w/o
>> debug info and the user can do lots of useful debugging still.  It is much
>> better if we can say "Found irrecoverable errors in this CU, dumping debug
>> info" and continue rather than crashing because one CU that the user may
>> not even care about is all horked up.  Similarly a single JIT execution can
>> get itself all balled up and yet if we can discard the work it did we can
>> continue doing lots of other things.
>>
>> So I think there are lots of places where you can get into a bad state
>> locally, but that doesn't mean #2 is the outcome.  Sometimes you even get
>> into a state where a specific target has done something totally
>> incomprehensible.  In that case, you may have to jettison that target, but
>> that might not be the only target/debugger in this lldb session.  So you
>> still wouldn't want to quit, those targets are probably fine.
>>
>> As you note, this is a little off from the original discussion which was
>> more about setting up and enforcing logic flows using the lldb_private
>> API's.  If you really have to call B only after calling A, and you can
>> clearly signal to yourself that A was called, then it is okay put an assert
>> there.  But in your case, CanResume seems like an optimization - and you
>> are clearly using it as such to avoid having to fix something deeper down
>> in lldb.  But in that case DoResume can error for other reasons, and so the
>> system should deal with it.  So adding another hard requirement was
>> inelegant and exiting if it wasn't met just made that more obvious...
>>
>> >
>> > --
>> >
>> > Going back to the original fix, I think most observations are very
>> pertinent - yes, the result is not ideal. This is because the current
>> logic, 

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Leonard Mosescu via lldb-commits]

Thanks Jim, I'll give option #1 a try and upload a new patch.

 think your divisions into options is too simplistic.  lldb is built of
> many different subsystems, and a lot of "inconsistent state" can be
> confined to one subsystem or other.  So for instance, if one CU's debug is
> unrecoverably damaged, we can just pretend that that .o file was built w/o
> debug info and the user can do lots of useful debugging still.  It is much
> better if we can say "Found irrecoverable errors in this CU, dumping debug
> info" and continue rather than crashing because one CU that the user may
> not even care about is all horked up.  Similarly a single JIT execution can
> get itself all balled up and yet if we can discard the work it did we can
> continue doing lots of other things.


The key here is the recoverable part - if you can isolate the state and
recover (throw-away, rollback), then I agree, that can be a preferable
approach. My point is about truly irrecoverable states - WTF states where
you don't expect to get into, can't do anything meaningful in response and
will likely cause cascading issues.

As a side note, I would *love* a generic roll-back mechanism in LLDB. Not
only that it can give us a viable alternative to failing fast, but it can
also allow things like LLDB-command interruption (which is one thing I'd
like to take a closer look when I get a chance)

On Mon, Sep 11, 2017 at 10:55 AM, Jim Ingham  wrote:

>
> > On Sep 11, 2017, at 10:24 AM, Leonard Mosescu 
> wrote:
> >
> > I think everyone is in agreement that input should be sanitized, and
> ill-formed input should not trigger an abrupt termination if there's a way
> to recover (which is not always feasible).
> >
> > The conversation started around fail-fast for inconsistent and
> unrecoverable state, where I think the math is very clear in this case.
> Using one the the comments about the long-lasting debugging sessions as
> example, let's say that we have a bug and the debugger state gets corrupted
> after 1h of debugging:
> > 1. debugger crashes immediately: lost 1h of debugging -> angry bug
> report (or ideally automated bug report)
> > 2. debugger limps along for a few more hours behaving erratically (not
> necessarily in an obvious fashion)
> > a. developer gives up after many hours or debugging without
> understanding what's going on
> > b. developer figures out that the debugger is unreliable
> >
> > While it is choosing between two evils, I think #1 is clearly the right
> choice: #2 results in more lost time and frustration, less chance of
> getting a bug report and can result in a general impression that the
> debugger is not to be trusted (which for a developer tool is infinitely
> worse than an unstable tool)
>
> I think your divisions into options is too simplistic.  lldb is built of
> many different subsystems, and a lot of "inconsistent state" can be
> confined to one subsystem or other.  So for instance, if one CU's debug is
> unrecoverably damaged, we can just pretend that that .o file was built w/o
> debug info and the user can do lots of useful debugging still.  It is much
> better if we can say "Found irrecoverable errors in this CU, dumping debug
> info" and continue rather than crashing because one CU that the user may
> not even care about is all horked up.  Similarly a single JIT execution can
> get itself all balled up and yet if we can discard the work it did we can
> continue doing lots of other things.
>
> So I think there are lots of places where you can get into a bad state
> locally, but that doesn't mean #2 is the outcome.  Sometimes you even get
> into a state where a specific target has done something totally
> incomprehensible.  In that case, you may have to jettison that target, but
> that might not be the only target/debugger in this lldb session.  So you
> still wouldn't want to quit, those targets are probably fine.
>
> As you note, this is a little off from the original discussion which was
> more about setting up and enforcing logic flows using the lldb_private
> API's.  If you really have to call B only after calling A, and you can
> clearly signal to yourself that A was called, then it is okay put an assert
> there.  But in your case, CanResume seems like an optimization - and you
> are clearly using it as such to avoid having to fix something deeper down
> in lldb.  But in that case DoResume can error for other reasons, and so the
> system should deal with it.  So adding another hard requirement was
> inelegant and exiting if it wasn't met just made that more obvious...
>
> >
> > --
> >
> > Going back to the original fix, I think most observations are very
> pertinent - yes, the result is not ideal. This is because the current
> logic, changing the state in Process::Resume() before the whole operation
> completes successfully is ... unfortunate. So here are the options I see:
> >
> > 1. Fix Process::Resume() so we only change the 

Re: [Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

[Leonard Mosescu via lldb-commits]

I think everyone is in agreement that input should be sanitized, and
ill-formed input should not trigger an abrupt termination if there's a way
to recover (which is not always feasible).

The conversation started around fail-fast for inconsistent and
unrecoverable state, where I think the math is very clear in this case.
Using one the the comments about the long-lasting debugging sessions as
example, let's say that we have a bug and the debugger state gets corrupted
after 1h of debugging:
1. debugger crashes immediately: lost 1h of debugging -> angry bug report
(or ideally automated bug report)
2. debugger limps along for a few more hours behaving erratically (not
necessarily in an obvious fashion)

a. developer gives up after many hours or debugging without understanding
what's going on

b. developer figures out that the debugger is unreliable

While it is choosing between two evils, I think #1 is clearly the right
choice: #2 results in more lost time and frustration, less chance of
getting a bug report and can result in a general impression that the
debugger is not to be trusted (which for a developer tool is infinitely
worse than an unstable tool)

--

Going back to the original fix, I think most observations are very
pertinent - yes, the result is not ideal. This is because the current
logic, changing the state in Process::Resume() before the whole operation
completes successfully is ... unfortunate. So here are the options I see:

1. Fix Process::Resume() so we only change the state late, after everything
succeds
2. Rollback state change if anything fails while resuming
3. Patch the specific case of "resume is not supported"
4. Do nothing

The proposed fix is #3, since it's the less intrusive (ie. doesn't change
the core resume logic) patch for a very specific and narrow case. IMO the
"right" fix is #1, but it seems a more radical change. #2 seems tempting in
terms of code changes, but I find it less compelling overall since
rollbacks are generally fragile if the code is not designed with that in
mind.

So, LLDB experts, what do you think? I'd be happy to go with #1 if someone
with experience in this area can review the change. Anything else that I
missed?

Thanks!






On Sun, Sep 10, 2017 at 1:52 PM, Jim Ingham via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

>
> On Sep 9, 2017, at 7:38 PM, Zachary Turner  wrote:
>
> Sure, but reading a core file involves handling user input. Obviously that
> has to be sanitized and you can't crash on user input. I don't think
> there's ever been any disagreement about that. On the other hand, if you
> just type an expression in the debugger and expect an answer back, after
> clang says the ast is valid, the whole stack should be asserts all the way
> down, because everything is validated
>
>
> Yes, but even that ends up not matching with the facts on the ground,
> because the debug information for types as used by lldb has to be produced
> from debug info, and the debug info can be malformed and not infrequently
> is.  So at some point clang is going to ask for something that it assumes
> must always be available because if it had read the types in from header
> files it would have asserted at an early stage but when it is fed types
> from debug info incrementally - which is the only efficient way the
> debugger can do it - is going to cause something to look wrong at a point
> where that “shouldn’t be possible.”  clang's rigid structure means it can’t
> cope with this and will crash on something the user didn’t themselves get
> wrong.
>
> According to your classification, we should really consider debug
> information “user input” as well.  But that means we’re wiring “user input”
> into a pretty deep place in clang and you’d probably have to weaken your
> strict assert model to handle that. And it makes me wonder if there’s
> anything the debugger consumes that wouldn’t fall into this category?
>
> For instance, we also let plugins provide threads and their stacks, and we
> try pretty hard not to crash and burn if these plugins get it a little
> wrong.  So process state really needs to be treated as “user data” as
> well.  And since we use SPI’s for most of this info that we are pretty much
> the only clients of, they can be expected to be flakey as well, again a
> fact that we should not inflict on users of the debugger if we can help it.
>
> The answer of trying to do everything dangerous out of process I think
> will make a  slow and fragile architecture.  And while I agree with the
> llvm.org decision not to use C++ exceptions because they make reasoning
> about the program more difficult, replacing that with SIGSEGV as the
> exception mechanism of choice seems really weak.  You are not the first
> person to point out that we could use the crash-protected threads, but I’m
> remain very unenthusiastic about this approach...
>
> Moreover, there are a bunch of problematic areas in lldb, but I don’t

Re: [Lldb-commits] [PATCH] D38552: LLDB cmake fix: define LLDB_CONFIGURATION_xxx based on the build type

[Leonard Mosescu via lldb-commits]

Identical, no - note the checks are inverted. Definitely uglier, but it's
defaulting to LLDB_CONFIGURATION_DEBUG if CMAKE_BUILD_TYPE is not defined,
right?

With the current defaulting rule I pointed out both versions should be
equivalent. If anything I prefer the 1st version since I like to avoid
negations in conditional expressions, but it's a minor thing and I'll leave
the final choice up to you.

On Wed, Oct 4, 2017 at 11:22 AM, Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added inline comments.
>
>
> 
> Comment at: CMakeLists.txt:15
> +# Define the LLDB_CONFIGURATION_xxx matching the build type
> +if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
> +  add_definitions( -DLLDB_CONFIGURATION_RELEASE )
> 
> Isn't this identical to the code before?
>
>
> https://reviews.llvm.org/D38552
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r314929 - LLDB cmake fix: define LLDB_CONFIGURATION_xxx based on the build type

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Wed Oct  4 13:23:56 2017
New Revision: 314929

URL: http://llvm.org/viewvc/llvm-project?rev=314929=rev
Log:
LLDB cmake fix: define LLDB_CONFIGURATION_xxx based on the build type

Neither LLDB_CONFIGURATION_DEBUG nor LLDB_CONFIGURATION_RELEASE were ever set 
in the CMake LLDB project.

Also cleaned up a questionable #ifdef in SharingPtr.h, removing all the 
references to LLDB_CONFIGURATION_BUILD_AND_INTEGRATION in the process.

Differential Revision: https://reviews.llvm.org/D38552


Modified:
lldb/trunk/CMakeLists.txt
lldb/trunk/include/lldb/Utility/SharingPtr.h

Modified: lldb/trunk/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/CMakeLists.txt?rev=314929=314928=314929=diff
==
--- lldb/trunk/CMakeLists.txt (original)
+++ lldb/trunk/CMakeLists.txt Wed Oct  4 13:23:56 2017
@@ -11,6 +11,13 @@ include(LLDBStandalone)
 include(LLDBConfig)
 include(AddLLDB)
 
+# Define the LLDB_CONFIGURATION_xxx matching the build type
+if( uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
+  add_definitions( -DLLDB_CONFIGURATION_DEBUG )
+else()
+  add_definitions( -DLLDB_CONFIGURATION_RELEASE )
+endif()
+
 if (CMAKE_SYSTEM_NAME MATCHES "Windows|Android")
   set(LLDB_DEFAULT_DISABLE_LIBEDIT 1)
 else()

Modified: lldb/trunk/include/lldb/Utility/SharingPtr.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/SharingPtr.h?rev=314929=314928=314929=diff
==
--- lldb/trunk/include/lldb/Utility/SharingPtr.h (original)
+++ lldb/trunk/include/lldb/Utility/SharingPtr.h Wed Oct  4 13:23:56 2017
@@ -529,16 +529,7 @@ public:
 
   ~IntrusiveSharingPtr() {
 release_shared();
-#if defined(LLDB_CONFIGURATION_DEBUG) || defined(LLDB_CONFIGURATION_RELEASE)
-// NULL out the pointer in objects which can help with leaks detection.
-// We don't enable this for LLDB_CONFIGURATION_BUILD_AND_INTEGRATION or
-// when none of the LLDB_CONFIGURATION_XXX macros are defined since
-// those would be builds for release. But for debug and release builds
-// that are for development, we NULL out the pointers to catch potential
-// issues.
 ptr_ = nullptr;
-#endif // #if defined (LLDB_CONFIGURATION_DEBUG) || defined
-   // (LLDB_CONFIGURATION_RELEASE)
   }
 
   T *() const { return *ptr_; }


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

Re: [Lldb-commits] [lldb] r334439 - Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via lldb-commits]

>
> > There was no way to find out what's wrong if SBProcess
> SBTarget::LoadCore(const char *core_file) failed.
> > Additionally, the implementation was unconditionally setting sb_process,
> so it wasn't even possible to check if the return SBProcess is valid.
>
> Not terribly important, but:
>
> >>> process = lldb.target.LoadCore("/not/here/no_core")
> >>> process.IsValid()
> False
>
> Pretty much all the SB objects have an IsValid method to test whether the
> call that made them was successful or not.  But IsValid doesn't specify the
> reason why the call didn't work, so having an error parameter is still a
> good thing.


True. In this case the SBTarget::LoadCore() implementation had a bug
though, which was causing it to return a "valid" process even on some
failure situations (when the actual Process::LoadCore() failed).

Full context in the change diff, but the key part is:

...
if (process_sp) {
  error.SetError(process_sp->LoadCore());
*  if (error.Success()) // this was missing*
sb_process.SetSP(process_sp);
}
...

Beyond this small fix, I also wanted the actual error for external
diagnostic purposes (for example it's valuable to know what was the actual
problem: corrupted, truncated or unsupported minidump, etc.)

On Mon, Jun 11, 2018 at 5:47 PM, Jim Ingham  wrote:

>
>
> > On Jun 11, 2018, at 2:19 PM, Leonard Mosescu via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > Author: lemo
> > Date: Mon Jun 11 14:19:26 2018
> > New Revision: 334439
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=334439=rev
> > Log:
> > Add a new SBTarget::LoadCore() overload which surfaces errors if the
> load fails
> >
> > There was no way to find out what's wrong if SBProcess
> SBTarget::LoadCore(const char *core_file) failed.
> > Additionally, the implementation was unconditionally setting sb_process,
> so it wasn't even possible to check if the return SBProcess is valid.
>
> Not terribly important, but:
>
> >>> process = lldb.target.LoadCore("/not/here/no_core")
> >>> process.IsValid()
> False
>
> Pretty much all the SB objects have an IsValid method to test whether the
> call that made them was successful or not.  But IsValid doesn't specify the
> reason why the call didn't work, so having an error parameter is still a
> good thing.
>
> Jim
>
>
> >
> > This change adds a new overload which surfaces the errors and also
> returns a valid SBProcess only if the core load succeeds:
> >
> > SBProcess SBTarget::LoadCore(const char *core_file, SBError );
> >
> > Differential Revision: https://reviews.llvm.org/D48049
> >
> >
> > Modified:
> >lldb/trunk/include/lldb/API/SBTarget.h
> >lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/
> minidump-new/TestMiniDumpNew.py
> >lldb/trunk/scripts/interface/SBTarget.i
> >lldb/trunk/source/API/SBTarget.cpp
> >
> > Modified: lldb/trunk/include/lldb/API/SBTarget.h
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/
> lldb/API/SBTarget.h?rev=334439=334438=334439=diff
> > 
> ==
> > --- lldb/trunk/include/lldb/API/SBTarget.h (original)
> > +++ lldb/trunk/include/lldb/API/SBTarget.h Mon Jun 11 14:19:26 2018
> > @@ -165,6 +165,7 @@ public:
> >  bool stop_at_entry, lldb::SBError );
> >
> >   SBProcess LoadCore(const char *core_file);
> > +  SBProcess LoadCore(const char *core_file, lldb::SBError );
> >
> >   //--
> >   /// Launch a new process with sensible defaults.
> > @@ -718,9 +719,9 @@ public:
> >   // Finds all breakpoints by name, returning the list in bkpt_list.
> Returns
> >   // false if the name is not a valid breakpoint name, true otherwise.
> >   bool FindBreakpointsByName(const char *name, SBBreakpointList
> _list);
> > -
> > +
> >   void GetBreakpointNames(SBStringList );
> > -
> > +
> >   void DeleteBreakpointName(const char *name);
> >
> >   bool EnableAllBreakpoints();
> >
> > Modified: lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/postmortem/minidump-new/TestMiniDumpNew.py
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/
> Python/lldbsuite/test/functionalities/postmortem/
> minidump-new/TestMiniDumpNew.py?rev=334439=334438=334439=diff
> > 
> ==

Re: [Lldb-commits] [lldb] r334439 - Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via lldb-commits]

>
> > There was no way to find out what's wrong if SBProcess
> SBTarget::LoadCore(const char *core_file) failed.
> > Additionally, the implementation was unconditionally setting sb_process,
> so it wasn't even possible to check if the return SBProcess is valid.
>
> Not terribly important, but:
>
> >>> process = lldb.target.LoadCore("/not/here/no_core")
> >>> process.IsValid()
> False
>
> Pretty much all the SB objects have an IsValid method to test whether the
> call that made them was successful or not.  But IsValid doesn't specify the
> reason why the call didn't work, so having an error parameter is still a
> good thing.


True. In this case the SBTarget::LoadCore() implementation had a bug
though, which was causing it to return a "valid" process even on some
failure situations (when the actual Process::LoadCore() failed).

Full context in the change diff, but the key part is:

...
if (process_sp) {
  error.SetError(process_sp->LoadCore());
*  if (error.Success()) // this was missing*
sb_process.SetSP(process_sp);
}
...

Beyond this small fix, I also wanted the actual error for external
diagnostic purposes (for example it's valuable to know what was the actual
problem: corrupted, truncated or unsupported minidump, etc.)

On Mon, Jun 11, 2018 at 5:47 PM, Jim Ingham  wrote:

>
>
> > On Jun 11, 2018, at 2:19 PM, Leonard Mosescu via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >
> > Author: lemo
> > Date: Mon Jun 11 14:19:26 2018
> > New Revision: 334439
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=334439=rev
> > Log:
> > Add a new SBTarget::LoadCore() overload which surfaces errors if the
> load fails
> >
> > There was no way to find out what's wrong if SBProcess
> SBTarget::LoadCore(const char *core_file) failed.
> > Additionally, the implementation was unconditionally setting sb_process,
> so it wasn't even possible to check if the return SBProcess is valid.
>
> Not terribly important, but:
>
> >>> process = lldb.target.LoadCore("/not/here/no_core")
> >>> process.IsValid()
> False
>
> Pretty much all the SB objects have an IsValid method to test whether the
> call that made them was successful or not.  But IsValid doesn't specify the
> reason why the call didn't work, so having an error parameter is still a
> good thing.
>
> Jim
>
>
> >
> > This change adds a new overload which surfaces the errors and also
> returns a valid SBProcess only if the core load succeeds:
> >
> > SBProcess SBTarget::LoadCore(const char *core_file, SBError );
> >
> > Differential Revision: https://reviews.llvm.org/D48049
> >
> >
> > Modified:
> >lldb/trunk/include/lldb/API/SBTarget.h
> >lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/
> minidump-new/TestMiniDumpNew.py
> >lldb/trunk/scripts/interface/SBTarget.i
> >lldb/trunk/source/API/SBTarget.cpp
> >
> > Modified: lldb/trunk/include/lldb/API/SBTarget.h
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/
> lldb/API/SBTarget.h?rev=334439=334438=334439=diff
> > 
> ==
> > --- lldb/trunk/include/lldb/API/SBTarget.h (original)
> > +++ lldb/trunk/include/lldb/API/SBTarget.h Mon Jun 11 14:19:26 2018
> > @@ -165,6 +165,7 @@ public:
> >  bool stop_at_entry, lldb::SBError );
> >
> >   SBProcess LoadCore(const char *core_file);
> > +  SBProcess LoadCore(const char *core_file, lldb::SBError );
> >
> >   //--
> >   /// Launch a new process with sensible defaults.
> > @@ -718,9 +719,9 @@ public:
> >   // Finds all breakpoints by name, returning the list in bkpt_list.
> Returns
> >   // false if the name is not a valid breakpoint name, true otherwise.
> >   bool FindBreakpointsByName(const char *name, SBBreakpointList
> _list);
> > -
> > +
> >   void GetBreakpointNames(SBStringList );
> > -
> > +
> >   void DeleteBreakpointName(const char *name);
> >
> >   bool EnableAllBreakpoints();
> >
> > Modified: lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/postmortem/minidump-new/TestMiniDumpNew.py
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/
> Python/lldbsuite/test/functionalities/postmortem/
> minidump-new/TestMiniDumpNew.py?rev=334439=334438=334439=diff
> > 
> ==

Re: [Lldb-commits] [PATCH] D47991: Improve SBThread's stepping API using SBError parameter.

[Leonard Mosescu via lldb-commits]

>
> Ah I see. That's because the last argument is a C++ default argument. It
> looks like the convention in this file is that the error argument should be
> the last non-defaulted argument.


I think it should be:

void StepOver(lldb::RunMode stop_other_threads, lldb::SBError ); //
no default argument!

Ie. if you want the overload with error you need to pass RunMode
explicitly. Keeping the overload set manageable is a good practice in
general, not just because of SWIG (default arguments + overloaded arguments
can easily get out of hand)



On Mon, Jun 11, 2018 at 5:51 PM, Adrian Prantl via Phabricator <
revi...@reviews.llvm.org> wrote:

> aprantl accepted this revision.
> aprantl added a comment.
>
> Ah I see. That's because the last argument is a C++ default argument. It
> looks like the convention in this file is that the error argument should be
> the last non-defaulted argument.
>
>
> https://reviews.llvm.org/D47991
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

[Leonard Mosescu via lldb-commits]

Doesn't the LIT based test drop the split-function case (originally
produced with PGO)?

Sorry for being late to the party, but it seems beneficial to have both LIT
*and* checked in binaries since in general they are complementary: checking
against freshly built binaries only covers a matching set of toolchain
components (in particular it's hard to cover the cross-targeting scenarios).

Other than the inconvenience with Phabricator, is there a reason not to
include the original tests as well? The size of the binaries?

On Fri, Jun 8, 2018 at 1:27 AM, Pavel Labath via Phabricator via
lldb-commits  wrote:

> labath added a comment.
>
> Thank you for your patience. I am very happy with the final result here.
>
> I personally like to keep number of files in a test minimal (hence I
> inlined the test into the cpp file), but overall that's not really
> important.
>
>
> https://reviews.llvm.org/D47708
>
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

[Leonard Mosescu via lldb-commits]

I agree, checked in binaries are not always pretty. But some coverage
depends checked in binaries (or at very least is dramatically harder to get
the same thing from source)

Are we saying that sacrificing coverage to keep tests smaller should be the
default trade off?


On Fri, Jun 8, 2018 at 10:04 AM, Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added a comment.
>
> In https://reviews.llvm.org/D47708#1126669, @lemo wrote:
>
> > Doesn't the LIT based test drop the split-function case (originally
> >  produced with PGO)?
> >
> > Sorry for being late to the party, but it seems beneficial to have both
> LIT
> >  *and* checked in binaries since in general they are complementary:
> checking
> >  against freshly built binaries only covers a matching set of toolchain
> >  components (in particular it's hard to cover the cross-targeting
> scenarios).
> >
> > Other than the inconvenience with Phabricator, is there a reason not to
> >  include the original tests as well? The size of the binaries?
>
>
> At least when it comes to checked in executables, we trigger virus
> scanners sometimes which is pretty annoying.  I don't mind a checked in
> PDB, but they get pretty big sometimes and you have to go out of your way
> to make them small by specifying things like `/nodefaultlib`.  But if you
> can get PDBs below about 200k, then checking them in might not be so bad.
> But I'd like to avoid checking in executables.
>
>
> https://reviews.llvm.org/D47708
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

[Leonard Mosescu via lldb-commits]

>
> For this particular case, I don't think having a split function is
> particularly interesting.


I agree that conceptually all disjoint ranges are the same. Until they are
not: I haven't looked closely at the code changes but it's easy to
introduce assumptions about the layout hierarchy
(range->function->segment->section->module)

Even if the code doesn't do this today it seems a good idea to have check
to catch attempts to introduce such assumptions in the future.
If these are intended to be regression tests it seems even more important
to do so.

Also, for either regression on integration tests I think it's a good idea
to include the common real world cases in addition to synthetic test cases.

That being said, these are just some drive-by comments and I'm not opposed
to getting this change in.




On Fri, Jun 8, 2018 at 12:23 PM, Zachary Turner  wrote:

> Ahh sorry, I jumped in kinda late and the thread was already quite long so
> I didn’t read everything. It would probably be some overhead to learn how
> to write the asm files but you can probably have clang-cl generate one for
> you and just move the directives around
> On Fri, Jun 8, 2018 at 12:18 PM Pavel Labath  wrote:
>
>> On Fri, 8 Jun 2018 at 19:58, Zachary Turner  wrote:
>> >
>> >
>> >
>> > On Fri, Jun 8, 2018 at 11:52 AM Pavel Labath  wrote:
>> >>
>> >> On Fri, 8 Jun 2018 at 18:48, Leonard Mosescu 
>> wrote:
>> >> >
>> >> > To me, a linker order file (of any linker) sounds like a good
>> >> abstraction level for generating that kind of input (on linux, I might
>> >> have preferred a .s file with hardcoded .loc directives, but that
>> >> doesn't seem to be a thing on windows).
>> >
>> >
>> > Why not?  I think that would work fine (it’s called .cv_loc in codeview
>> though)
>>
>> Because I didn't know that's possible? :D
>>
>> I dropped a remark about .s files several pages back but noone picked
>> it up, so I assumed it's infeasible for some reason. If that is the
>> case, then this really sounds like a good idea. This way we should be
>> even able to generate the split function without relying on PGO or
>> checked-in binaries.
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

[Leonard Mosescu via lldb-commits]

So what's your take on this particular case? As far as I can see (please
correct me if I'm wrong), the LIT-only tests:

1. Don't cover the case where a function is split into disjoint regions,
right?
2. Also don't cover the cross-targeting case (ex. the native PDB reader
hosted on Linux)
3. A bug in LLD might inadvertently make the tests pass w/o testing what
they are supposed to
   (let's say that it incorrectly ingores the hardcoded order and lays
everything contiguously)

On Fri, Jun 8, 2018 at 10:14 AM, Zachary Turner  wrote:

> On Fri, Jun 8, 2018 at 10:10 AM Leonard Mosescu 
> wrote:
>
>> I agree, checked in binaries are not always pretty. But some coverage
>> depends checked in binaries (or at very least is dramatically harder to get
>> the same thing from source)
>>
>> Are we saying that sacrificing coverage to keep tests smaller should be
>> the default trade off?
>>
>
> It's probably worth evaluating on a case by case basis.  Often time there
> are ways to use lower level LLVM tools like llvm-mc, yaml2obj, etc so that
> we can construct binaries on the fly which are reproducible.  In these
> cases we should check in the "meta-inputs" that allow us to reproducibly
> construct the test inputs on the fly.
>
> One can easily imagine the repository growing to many tens of gigabytes
> just due to test inputs, which is not really scalable.
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via lldb-commits]

The intention in the scope of this change is just to check that the new
overload is exposed correctly through the Python API.

In general, guaranteeing specific error codes (messages?) is likely
impractical:

1. It's not always easy to do the proper checks (ex. for 'file-not-found'
you'd actually get a null process, nothing more - see SBTarget::LoadCore()).
2. It's unlikely that many clients need or want to check for specific
errors.
3. Such a strict contract would be very fragile (ex. adding more specific
error condition detection would likely break the contract)

Perhaps ironically, I could take advantage of very specific error codes for
my scenario, but I don't think it's feasible. So for LoadCore() I'd like to
aim for more of a middle ground: accurate success/fail status + an
informative error content that can be used for diagnostics (logging) and
maybe as a weak hint.

What do you think?





On Wed, Jun 13, 2018 at 9:59 AM, Adrian McCarthy via Phabricator <
revi...@reviews.llvm.org> wrote:

> amccarth added inline comments.
>
>
> 
> Comment at: lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/postmortem/minidump-new/TestMiniDumpNew.py:78
> +self.assertFalse(self.process, PROCESS_IS_VALID)
> +self.assertTrue(error.Fail())
> +
> 
> Is it worth checking something more specific here?  That the error
> indicates the file was not found?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D48049
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r334439 - Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Mon Jun 11 14:19:26 2018
New Revision: 334439

URL: http://llvm.org/viewvc/llvm-project?rev=334439=rev
Log:
Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

There was no way to find out what's wrong if SBProcess SBTarget::LoadCore(const 
char *core_file) failed. 
Additionally, the implementation was unconditionally setting sb_process, so it 
wasn't even possible to check if the return SBProcess is valid.

This change adds a new overload which surfaces the errors and also returns a 
valid SBProcess only if the core load succeeds:

SBProcess SBTarget::LoadCore(const char *core_file, SBError );

Differential Revision: https://reviews.llvm.org/D48049


Modified:
lldb/trunk/include/lldb/API/SBTarget.h

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
lldb/trunk/scripts/interface/SBTarget.i
lldb/trunk/source/API/SBTarget.cpp

Modified: lldb/trunk/include/lldb/API/SBTarget.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/API/SBTarget.h?rev=334439=334438=334439=diff
==
--- lldb/trunk/include/lldb/API/SBTarget.h (original)
+++ lldb/trunk/include/lldb/API/SBTarget.h Mon Jun 11 14:19:26 2018
@@ -165,6 +165,7 @@ public:
  bool stop_at_entry, lldb::SBError );
 
   SBProcess LoadCore(const char *core_file);
+  SBProcess LoadCore(const char *core_file, lldb::SBError );
 
   //--
   /// Launch a new process with sensible defaults.
@@ -718,9 +719,9 @@ public:
   // Finds all breakpoints by name, returning the list in bkpt_list.  Returns
   // false if the name is not a valid breakpoint name, true otherwise.
   bool FindBreakpointsByName(const char *name, SBBreakpointList _list);
-  
+
   void GetBreakpointNames(SBStringList );
-  
+
   void DeleteBreakpointName(const char *name);
 
   bool EnableAllBreakpoints();

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py?rev=334439=334438=334439=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 Mon Jun 11 14:19:26 2018
@@ -59,6 +59,24 @@ class MiniDumpNewTestCase(TestBase):
 self.dbg.SetOutputFileHandle(None, False)
 self.dbg.SetErrorFileHandle(None, False)
 
+def test_loadcore_error_status(self):
+"""Test the SBTarget.LoadCore(core, error) overload."""
+self.dbg.CreateTarget(None)
+self.target = self.dbg.GetSelectedTarget()
+error = lldb.SBError()
+self.process = self.target.LoadCore("linux-x86_64.dmp", error)
+self.assertTrue(self.process, PROCESS_IS_VALID)
+self.assertTrue(error.Success())
+
+def test_loadcore_error_status_failure(self):
+"""Test the SBTarget.LoadCore(core, error) overload."""
+self.dbg.CreateTarget(None)
+self.target = self.dbg.GetSelectedTarget()
+error = lldb.SBError()
+self.process = self.target.LoadCore("non-existent.dmp", error)
+self.assertFalse(self.process, PROCESS_IS_VALID)
+self.assertTrue(error.Fail())
+
 def test_process_info_in_minidump(self):
 """Test that lldb can read the process information from the 
Minidump."""
 # target create -c linux-x86_64.dmp

Modified: lldb/trunk/scripts/interface/SBTarget.i
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/scripts/interface/SBTarget.i?rev=334439=334438=334439=diff
==
--- lldb/trunk/scripts/interface/SBTarget.i (original)
+++ lldb/trunk/scripts/interface/SBTarget.i Mon Jun 11 14:19:26 2018
@@ -77,7 +77,7 @@ public:
 
 static const char *
 GetBroadcasterClassName ();
-
+
 bool
 IsValid() const;
 
@@ -145,7 +145,7 @@ public:
 /// An optional listener that will receive all process events.
 /// If \a listener is valid then \a listener will listen to all
 /// process events. If not valid, then this target's debugger
-/// (SBTarget::GetDebugger()) will listen to all process events. 
+/// (SBTarget::GetDebugger()) will listen to all process events.
 ///
 /// @param[in] argv
 /// The argument array.
@@ -175,7 +175,7 @@ public:
 /// The working directory to have the child process run in
 ///
 /// @param[in] launch_flags
-/// Some launch options specified by logical OR'ing 
+/// Some launch options specified by 

Re: [Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

[Leonard Mosescu via lldb-commits]

>
> remove space before (
>

I'd be happy to make the change, but looking at the rest of the file it
seems that almost everything uses the opposite convention: Foo (...).

So, to make sure I'm making the right change, is this how it should look?

lldb::SBProcess
LoadCore(const char *core_file);

lldb::SBProcess
LoadCore(const char *core_file, lldb::SBError );


On Mon, Jun 11, 2018 at 1:47 PM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg accepted this revision.
> clayborg added a comment.
> This revision is now accepted and ready to land.
>
> Remove extra spaces before ( and good to go.
>
>
>
> 
> Comment at: scripts/interface/SBTarget.i:282
>  lldb::SBProcess
> -LoadCore(const char *core_file);
> -
> +LoadCore (const char *core_file);
> +
> 
> remove space before (
>
>
> 
> Comment at: scripts/interface/SBTarget.i:285
> +lldb::SBProcess
> +LoadCore (const char *core_file, lldb::SBError );
> +
> 
> remove space before (
>
>
> 
> Comment at: scripts/interface/SBTarget.i:288
>  lldb::SBProcess
>  Attach (lldb::SBAttachInfo _info, lldb::SBError& error);
>
> 
> remove space before (
>
>
> https://reviews.llvm.org/D48049
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] Support of MSVC function-level linking

[Leonard Mosescu via lldb-commits]

If anyone's working on this I'd suggest adding a test case for the "split
code" case as well (where even a single function is split into multiple
ranges). MSVC with PGO should help produce hot/cold cold split repros.

On Thu, May 31, 2018 at 10:24 AM, Greg Clayton via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

>
>
> On May 31, 2018, at 2:31 AM, Aleksandr Urakov via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
>
> Hello!
>
> I'm Aleksandr from JetBrains. We are working on improving support of
> MSVC-compiled binaries in lldb. We have made several fixes and would like
> to upstream them.
>
> The first patch adds support of function-level linking feature. The
> SymbolFilePDB::ParseCompileUnitLineTable function relies on the fact that
> ranges of compiled source files in the binary are continuous and don't
> intersect with each other. ParseCompileUnitLineTable creates LineSequence
> for each file and inserts it into LineTable, and the implementation of
> LineTable relies on continuity of the sequence. But it's not always true
> when function-level linking is enabled, e.g. in the attached input test
> file test-pdb-function-level-linking.exe there is xstring's
> std__basic_string_char_std__char_traits_char__std__allocator_char_max_size
> (.00454820) between test-pdb-function-level-linking.cpp's foo (.00454770)
> and main (.004548F0). The source is compiled with Microsoft C/C++ compiler
> version 19.14.26429.4 for x86.
>
> To fix the problem we propose to renew the sequence on each address gap.
>
>
> That is what DWARF does as well. A line table can have many sequences
> where each sequence is a vector of rows whose addresses must always
> increase or stay the same. The line tables we have in LLDB mimic the DWARF
> line tables in many ways.
>
>
>
> The link to the patch and related files is: https://drive.google.com/
> open?id=1ozp06jyqugjLGT-6wuJKS1UhRuXFsixf
>
> Thanks!
>
> --
> Aleksandr Urakov
> Software Developer
> JetBrains
> http://www.jetbrains.com
> The Drive to Develop
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

[Leonard Mosescu via lldb-commits]

>
> However, during parsing you need to know the meaning of a "...0" UUID.
> In a MachO file (at least based on the comments in the code) this value is
> used to denote the fact that the object file has no UUID. For elf, a
> "000..0" build-id is a perfectly valid identifier (and the lack of a
> build-id is denoted by the absence of the build-id section). The extra
> constructor argument is my way of trying to support both scenarios. The
> other possibility I see is to have a some kind of a factory function for
> one of the options (or both). I don't really have a preference between the
> two.


 One solution might be to encapsulate the MachO convention in the MachO
code: check in there (maybe through a helper function) if the UUID is
"000...0" and map it to the empty UUID in that case. The UUID interface
would not have to know/care about this convention. Would this work?

On Fri, Jun 22, 2018 at 12:02 PM, Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added a comment.
>
> In https://reviews.llvm.org/D48479#1140927, @lemo wrote:
>
> > > The slight complication here is that
> > >  some clients (MachO) actually use the all-zero notation to mean "no
> UUID
> > >  has been set". To keep this use case working, I have introduced an
> > >  additional argument to the UUID constructor, which specifies whether
> an
> > >  all-zero vector should be considered a valid UUID. For the usages
> where
> > >  the UUID data comes from a MachO file, I set this argument to false.
> >
> > What is the distinction between "no UUID has been set" and "invalid
> UUID"? I find this subtlety confusing, can we simplify it to just
> valid/not-valid UUIDs?
>
>
> That is what I am trying to do (although not completely successfully, it
> seems ;) ). Once you have a UUID object around there should be no
> distinction. You either have a valid uuid (for now, consisting of 16 or 20
> bytes), or you don't.
>
> However, during parsing you need to know the meaning of a "...0" UUID.
> In a MachO file (at least based on the comments in the code) this value is
> used to denote the fact that the object file has no UUID. For elf, a
> "000..0" build-id is a perfectly valid identifier (and the lack of a
> build-id is denoted by the absence of the build-id section). The extra
> constructor argument is my way of trying to support both scenarios. The
> other possibility I see is to have a some kind of a factory function for
> one of the options (or both). I don't really have a preference between the
> two.
>
>
>
> 
> Comment at: include/lldb/Utility/UUID.h:31
>// Most UUIDs are 16 bytes, but some Linux build-ids (SHA1) are 20.
>typedef uint8_t ValueType[20];
>
> 
> lemo wrote:
> > switch to llvm::SmallVector as suggested by Greg?
> I'm deliberately keeping that for a separate patch. Here, I just want to
> prepare the ground by defining the "invalid" UUID more clearly. The part
> with the arbitrary UUID length will come after that.
>
>
> 
> Comment at: include/lldb/Utility/UUID.h:42
>
> -  const UUID =(const UUID );
> +  UUID =(const UUID ) = default;
>
> 
> lemo wrote:
> > If we define this we should define at least the copy constructor as well
> (rule of 3/5). In this case the cleanest solution may be to allow the
> implicit definitions (which would also allow the implicit move operations -
> defining assignment operator as defaulted would inhibit the implicit move
> SMFs)
> Good point. I've deleted the copy constructor altogether.
>
>
> 
> Comment at: include/lldb/Utility/UUID.h:52
>
> -  bool IsValid() const;
> +  explicit operator bool() const { return IsValid(); }
> +  bool IsValid() const { return m_num_uuid_bytes > 0; }
> 
> lemo wrote:
> > is this really needed? I'd prefer the truly explicit (pun intended)
> IsValid()
> My main reason for an `operator bool` is that it allows the if-declaration
> syntax (`if (UUID u = getUUID())  doSomethingUsefulWith(u);` The most
> llvm-y solution would be not have neither of these methods and use
> Optional when you don't know if you have one, but as far as operator
> bool vs. isValid goes, both styles are used in llvm (and lldb).
>
>
> https://reviews.llvm.org/D48479
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

[Leonard Mosescu via lldb-commits]

>
> I'm not sure if there is a suitable place for that function. This is
> needed in  "ObjectFileMachO" and two dynamic loader plugins.


Then your factory idea may be the next best thing. While we're at it, maybe
we can remove the UUID::SetBytes() from the public interface, and make the
UUID an immutable value type:

Ex. instead of:

UUID uuid;
...
uuid.SetBytes(...)

We'd have:

UUID uuid;

uuid = UUID(...);
// or
uuid = { ... };
// or
uuid = UUID::factory(...);

What do you think?

On Fri, Jun 22, 2018 at 12:29 PM, Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added a comment.
>
> In https://reviews.llvm.org/D48479#1141067, @lemo wrote:
>
> >   One solution might be to encapsulate the MachO convention in the MachO
> >
> > code: check in there (maybe through a helper function) if the UUID is
> >  "000...0" and map it to the empty UUID in that case. The UUID interface
> >  would not have to know/care about this convention. Would this work?
>
>
> I'm not sure if there is a suitable place for that function. This is
> needed in  "ObjectFileMachO" and two dynamic loader plugins.
>
>
> https://reviews.llvm.org/D48479
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r331394 - Use the UUID from the minidump's CodeView Record for placeholder modules

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Wed May  2 13:06:17 2018
New Revision: 331394

URL: http://llvm.org/viewvc/llvm-project?rev=331394=rev
Log:
Use the UUID from the minidump's CodeView Record for placeholder modules

This change adds support for two types of Minidump CodeView records:

PDB70 (reference: 
https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html)
This is by far the most common record type.

ELF BuildID (found in Breakpad/Crashpad generated minidumps)
This would set a proper UUID for placeholder modules, in turn enabling
an accurate match with local module images.

Differential Revision: https://reviews.llvm.org/D46292


Modified:
lldb/trunk/include/lldb/Core/Module.h
lldb/trunk/include/lldb/Core/ModuleSpec.h

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
lldb/trunk/source/Commands/CommandObjectTarget.cpp
lldb/trunk/source/Core/Module.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.h
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp

Modified: lldb/trunk/include/lldb/Core/Module.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/Module.h?rev=331394=331393=331394=diff
==
--- lldb/trunk/include/lldb/Core/Module.h (original)
+++ lldb/trunk/include/lldb/Core/Module.h Wed May  2 13:06:17 2018
@@ -745,7 +745,7 @@ public:
   TypeList *GetTypeList();
 
   //--
-  /// Get a pointer to the UUID value contained in this object.
+  /// Get a reference to the UUID value contained in this object.
   ///
   /// If the executable image file doesn't not have a UUID value built into
   /// the file format, an MD5 checksum of the entire file, or slice of the
@@ -,7 +,7 @@ protected:
 
   std::atomic m_did_load_objfile{false};
   std::atomic m_did_load_symbol_vendor{false};
-  std::atomic m_did_parse_uuid{false};
+  std::atomic m_did_set_uuid{false};
   mutable bool m_file_has_changed : 1,
   m_first_file_changed_log : 1; /// See if the module was modified after it
 /// was initially opened.
@@ -1161,6 +1161,8 @@ protected:
 
   bool SetArchitecture(const ArchSpec _arch);
 
+  void SetUUID(const lldb_private::UUID );
+
   SectionList *GetUnifiedSectionList();
 
   friend class ModuleList;

Modified: lldb/trunk/include/lldb/Core/ModuleSpec.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ModuleSpec.h?rev=331394=331393=331394=diff
==
--- lldb/trunk/include/lldb/Core/ModuleSpec.h (original)
+++ lldb/trunk/include/lldb/Core/ModuleSpec.h Wed May  2 13:06:17 2018
@@ -34,9 +34,9 @@ public:
 m_object_name(), m_object_offset(0), m_object_size(0),
 m_source_mappings() {}
 
-  ModuleSpec(const FileSpec _spec)
+  ModuleSpec(const FileSpec _spec, const UUID& uuid = UUID())
   : m_file(file_spec), m_platform_file(), m_symbol_file(), m_arch(),
-m_uuid(), m_object_name(), m_object_offset(0),
+m_uuid(uuid), m_object_name(), m_object_offset(0),
 m_object_size(file_spec.GetByteSize()), m_source_mappings() {}
 
   ModuleSpec(const FileSpec _spec, const ArchSpec )

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py?rev=331394=331393=331394=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 Wed May  2 13:06:17 2018
@@ -77,9 +77,49 @@ class MiniDumpNewTestCase(TestBase):
 self.target = self.dbg.GetSelectedTarget()
 self.process = self.target.LoadCore("linux-x86_64.dmp")
 self.assertTrue(self.process, PROCESS_IS_VALID)
-self.assertEqual(self.target.GetNumModules(), 9)
-for module in self.target.modules:
+expected_modules = [
+{
+'filename' : 'linux-gate.so',
+'uuid' : '4EAD28F8-88EF-3520-872B-73C6F2FE7306-C41AF22F',
+},
+{
+'filename' : 'libm-2.19.so',
+'uuid' : 'D144258E-6149-00B2-55A3-1F3FD2283A87-8670D5BC',
+},
+{
+'filename' : 'libstdc++.so.6.0.19',
+'uuid' : 

Re: [Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Leonard Mosescu via lldb-commits]

FYI, Breakpad & Crashpad will start generating the Microsoft flavor of ARM
minidumps soon.

On Wed, Jul 25, 2018 at 9:44 AM, Leonard Mosescu via Phabricator <
revi...@reviews.llvm.org> wrote:

> lemo added inline comments.
>
>
> 
> Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.
> cpp:51
> +  reg_fpscr,
> +  reg_d0,   reg_d1,  reg_d2,  reg_d3,  reg_d4,  reg_d5,  reg_d6,  reg_d7,
> +  reg_d8,   reg_d9, reg_d10, reg_d11, reg_d12, reg_d13, reg_d14, reg_d15,
> 
> Pavel's comment reminded me: what about the S registers (32bit fp) and Q
> registers (128bit Neon)?
>
>
> https://reviews.llvm.org/D49750
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

[Leonard Mosescu via lldb-commits]

The new test cases did catch a real bug:

[ RUN  ] VMRange.CollectionContains
llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure
Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104))
  Actual: false
Expected: true


class RangeInRangeUnaryPredicate {
public:
RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that
_range binds to a temporary!
bool operator()(const VMRange ) const {
return range.Contains(_range);
}
const VMRange &_range;
};

I just sent out a review for the fix (https://reviews.llvm.org/D50290)


On Tue, Jul 24, 2018 at 4:53 PM, Raphael Isemann via Phabricator via
lldb-commits  wrote:

> This revision was not accepted when it landed; it landed in state "Needs
> Review".
> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL337873: Add unit tests for VMRange (authored by
> teemperor, committed by ).
> Herald added a subscriber: llvm-commits.
>
> Changed prior to commit:
>   https://reviews.llvm.org/D49415?vs=157172=157173#toc
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D49415
>
> Files:
>   lldb/trunk/unittests/Utility/CMakeLists.txt
>   lldb/trunk/unittests/Utility/VMRangeTest.cpp
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

[Leonard Mosescu via lldb-commits]

Thanks Zach. I can't find llvm::contains() though, any pointers to it?

On Fri, Aug 3, 2018 at 5:58 PM, Zachary Turner via Phabricator via
lldb-commits  wrote:

> zturner added a subscriber: lemo.
> zturner added a comment.
>
> I think we have llvm::contains() which would allow you to make this
> slightly better. Other than that, good find!
>
>
> Repository:
>   rLLDB LLDB
>
> https://reviews.llvm.org/D50290
>
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Leonard Mosescu via lldb-commits]

>
> I never *ran LLDB tests*, not sure where they are and what they are.


I hope you're planning to look into this before submitting the change :)


On Fri, Jul 27, 2018 at 11:28 AM, Eugene Birukov via Phabricator <
revi...@reviews.llvm.org> wrote:

> EugeneBi added a comment.
>
> Hmm... I never thought I can do that :)
> Anyway, with the change as it is now, LLDB would try to load executable in
> the sysroot, fail that, then open it without the sysroot.
>
>
> https://reviews.llvm.org/D49685
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via lldb-commits]

>
> The LLDB MI plugin didn't work very well as was quite flaky when I tested
> it a while back.
>

Just curious, what was the flaky part, the debug adapter or the LLDB MI
interface?

On Wed, Aug 8, 2018 at 8:40 AM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> In https://reviews.llvm.org/D50365#1192447, @zturner wrote:
>
> > To elaborate, if you install the C/C++ plugin, and you go to Debug ->
> >  Configurations, and you go down to the MICmdMode property, it is by
> default
> >  set to "gdb".  But you can change this to "lldb" and it works out of the
> >  box.
>
>
> It is a different protocol. The LLDB MI plugin didn't work very well as
> was quite flaky when I tested it a while back. Then I grabbed the CodeLLDB
> plugin by Vadim Chugunov and it worked very well. When I looked more
> closely at this plugin, it was using a javascript/typescript plug-in to
> launch a LLDB instance and then ran a python script that received all of
> the javascript packets from the javascript/typescript based plug-in and ran
> the debug session using the python interpreter. It worked very well and was
> rock solid stable. So I then created this plug-in for Nuclide for use at
> Facebook as they switched all of the debugging plug-in over to use the
> VSCode debug adaptor protocols. It works event better than the CodeLLDB
> plugin with Visual Studio Code and it also doesn't stop you from using the
> python interpreter. The issue I had with the CodeLLDB is that is uses the
> python interpreter to run the debug session thus it isn't available to you
> as a LLDB user.
>
> So long story short: our IDE at Facebook uses the VSCode protocol, MI is
> clunky and doesn't work that well and was flaky, so this tool was created.
> This also provides a really nice way to do remote debugging where the
> lldb-vscode is run remotely on other systems. This removes the need for
> copying files from a remote host up onto the system that is doing the
> debugging. So we use this at Facebook and it also provides the best way to
> use LLDB to debug using Visual Studio Code or Nuclide.
>
>
> https://reviews.llvm.org/D50365
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r338949 - Fix a bug in VMRange

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Fri Aug  3 19:15:26 2018
New Revision: 338949

URL: http://llvm.org/viewvc/llvm-project?rev=338949=rev
Log:
Fix a bug in VMRange

I noticed a suspicious failure:

[ RUN ] VMRange.CollectionContains
llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure
Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104))

Actual: false
Expected: true

Looking at the code, it is a very real bug:

class RangeInRangeUnaryPredicate {
public:
  RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that 
_range binds to a temporary!
  bool operator()(const VMRange ) const {
return range.Contains(_range);
  }
  const VMRange &_range;
};

This change fixes the bug.

Differential Revision: https://reviews.llvm.org/D50290


Modified:
lldb/trunk/include/lldb/Utility/VMRange.h
lldb/trunk/source/Utility/VMRange.cpp

Modified: lldb/trunk/include/lldb/Utility/VMRange.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/VMRange.h?rev=338949=338948=338949=diff
==
--- lldb/trunk/include/lldb/Utility/VMRange.h (original)
+++ lldb/trunk/include/lldb/Utility/VMRange.h Fri Aug  3 19:15:26 2018
@@ -87,24 +87,6 @@ public:
   void Dump(Stream *s, lldb::addr_t base_addr = 0,
 uint32_t addr_width = 8) const;
 
-  class ValueInRangeUnaryPredicate {
-  public:
-ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_value);
-}
-lldb::addr_t _value;
-  };
-
-  class RangeInRangeUnaryPredicate {
-  public:
-RangeInRangeUnaryPredicate(VMRange range) : _range(range) {}
-bool operator()(const VMRange ) const {
-  return range.Contains(_range);
-}
-const VMRange &_range;
-  };
-
   static bool ContainsValue(const VMRange::collection ,
 lldb::addr_t value);
 

Modified: lldb/trunk/source/Utility/VMRange.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/VMRange.cpp?rev=338949=338948=338949=diff
==
--- lldb/trunk/source/Utility/VMRange.cpp (original)
+++ lldb/trunk/source/Utility/VMRange.cpp Fri Aug  3 19:15:26 2018
@@ -24,14 +24,16 @@ using namespace lldb_private;
 
 bool VMRange::ContainsValue(const VMRange::collection ,
 lldb::addr_t value) {
-  ValueInRangeUnaryPredicate in_range_predicate(value);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(value);
+ }) != coll.end();
 }
 
 bool VMRange::ContainsRange(const VMRange::collection ,
 const VMRange ) {
-  RangeInRangeUnaryPredicate in_range_predicate(range);
-  return llvm::find_if(coll, in_range_predicate) != coll.end();
+  return llvm::find_if(coll, [&](const VMRange ) {
+   return r.Contains(range);
+ }) != coll.end();
 }
 
 void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const {


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

[Lldb-commits] [lldb] r339161 - Misc module/dwarf logging improvements

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Tue Aug  7 11:00:30 2018
New Revision: 339161

URL: http://llvm.org/viewvc/llvm-project?rev=339161=rev
Log:
Misc module/dwarf logging improvements

This change improves the logging for the lldb.module category to note a few 
interesting cases:

1. Local object file found, but specs not matching
2. Local object file not found, using a placeholder module

The handling and logging for the cases wehre we fail to load compressed dwarf
symbols is also improved.

Differential Revision: https://reviews.llvm.org/D50274


Modified:
lldb/trunk/lit/Modules/compressed-sections.yaml
lldb/trunk/source/Core/Module.cpp
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/trunk/source/Target/Process.cpp

Modified: lldb/trunk/lit/Modules/compressed-sections.yaml
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/compressed-sections.yaml?rev=339161=339160=339161=diff
==
--- lldb/trunk/lit/Modules/compressed-sections.yaml (original)
+++ lldb/trunk/lit/Modules/compressed-sections.yaml Tue Aug  7 11:00:30 2018
@@ -28,5 +28,4 @@ Sections:
 # CHECK-NEXT: Type: regular
 # CHECK-NEXT: VM size: 0
 # CHECK-NEXT: File size: 8
-# CHECK-NEXT: Data:
-# CHECK-NEXT: DEADBEEF BAADF00D
+# CHECK-NEXT: Data: ()

Modified: lldb/trunk/source/Core/Module.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Module.cpp?rev=339161=339160=339161=diff
==
--- lldb/trunk/source/Core/Module.cpp (original)
+++ lldb/trunk/source/Core/Module.cpp Tue Aug  7 11:00:30 2018
@@ -162,9 +162,13 @@ Module::Module(const ModuleSpec _
   // fill any ivars in so we don't accidentally grab the wrong file later since
   // they don't match...
   ModuleSpec matching_module_spec;
-  if (modules_specs.FindMatchingModuleSpec(module_spec, matching_module_spec) 
==
-  0)
+  if (!modules_specs.FindMatchingModuleSpec(module_spec,
+matching_module_spec)) {
+if (log) {
+  log->Printf("Found local object file but the specs didn't match");
+}
 return;
+  }
 
   if (module_spec.GetFileSpec())
 m_mod_time = FileSystem::GetModificationTime(module_spec.GetFileSpec());

Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=339161=339160=339161=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Tue Aug  7 
11:00:30 2018
@@ -1390,7 +1390,7 @@ ObjectFileELF::RefineModuleDetailsFromNo
 arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS)
   // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some
   // cases (e.g. compile with -nostdlib) Hence set OS to Linux
-  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); 
+  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
   }
 }
 
@@ -1494,7 +1494,7 @@ size_t ObjectFileELF::GetSectionHeaderIn
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-
+
 // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is
 // determined based on EI_OSABI flag and the info extracted from ELF notes
 // (see RefineModuleDetailsFromNote). However in some cases that still
@@ -3385,8 +3385,6 @@ size_t ObjectFileELF::ReadSectionData(Se
   if (section->GetObjectFile() != this)
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
-  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-
   size_t result = ObjectFile::ReadSectionData(section, section_data);
   if (result == 0 || !section->Test(SHF_COMPRESSED))
 return result;
@@ -3397,20 +3395,27 @@ size_t ObjectFileELF::ReadSectionData(Se
size_t(section_data.GetByteSize())},
   GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8);
   if (!Decompressor) {
-LLDB_LOG_ERROR(log, Decompressor.takeError(),
-   "Unable to initialize decompressor for section {0}",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Unable to initialize decompressor for section '%s': %s",
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+section_data.Clear();
+return 0;
   }
+
   auto buffer_sp =
   std::make_shared(Decompressor->getDecompressedSize(), 0);
-  if (auto Error = Decompressor->decompress(
+  if (auto error = 

Re: [Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via lldb-commits]

Really cool! Are you planning to add some documentation for it? (set up
instructions, etc...)

On Tue, Aug 7, 2018 at 9:24 AM, Greg Clayton via Phabricator via
lldb-commits  wrote:

> clayborg updated this revision to Diff 159526.
> clayborg added a comment.
>
> Removed dead code from python files.
>
>
> https://reviews.llvm.org/D50365
>
> Files:
>   lldb.xcodeproj/project.pbxproj
>   packages/Python/lldbsuite/test/dotest.py
>   packages/Python/lldbsuite/test/lldbtest.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/.categories
>   packages/Python/lldbsuite/test/tools/lldb-vscode/attach/Makefile
>   packages/Python/lldbsuite/test/tools/lldb-vscode/attach/
> TestVSCode_attach.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/attach/main.c
>   packages/Python/lldbsuite/test/tools/lldb-vscode/breakpoint/Makefile
>   packages/Python/lldbsuite/test/tools/lldb-vscode/breakpoint/TestVSCode_
> setBreakpoints.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/breakpoint/TestVSCode_
> setExceptionBreakpoints.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/breakpoint/TestVSCode_
> setFunctionBreakpoints.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/breakpoint/main.cpp
>   packages/Python/lldbsuite/test/tools/lldb-vscode/launch/Makefile
>   packages/Python/lldbsuite/test/tools/lldb-vscode/launch/
> TestVSCode_launch.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/launch/main.c
>   packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/stackTrace/Makefile
>   packages/Python/lldbsuite/test/tools/lldb-vscode/stackTrace/TestVSCode_
> stackTrace.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/stackTrace/main.c
>   packages/Python/lldbsuite/test/tools/lldb-vscode/step/Makefile
>   packages/Python/lldbsuite/test/tools/lldb-vscode/step/TestVSCode_step.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/step/main.cpp
>   packages/Python/lldbsuite/test/tools/lldb-vscode/variables/Makefile
>   packages/Python/lldbsuite/test/tools/lldb-vscode/variables/TestVSCode_
> variables.py
>   packages/Python/lldbsuite/test/tools/lldb-vscode/variables/main.cpp
>   packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
>   tools/CMakeLists.txt
>   tools/debugserver/source/JSON.cpp
>   tools/debugserver/source/JSON.h
>   tools/lldb-vscode/CMakeLists.txt
>   tools/lldb-vscode/lldb-vscode-Info.plist
>   tools/lldb-vscode/lldb-vscode.cpp
>   tools/lldb-vscode/package.json
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via lldb-commits]

It's an interesting idea, thanks! I don't object moving code around if
there's a strong case for it, but I'd like to keep the fix small and simple
for now, but it's worth considering if the current minidump loading path
will need more flexibility.


On Thu, Aug 23, 2018 at 1:27 PM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> In https://reviews.llvm.org/D51176#1211433, @jingham wrote:
>
> > The other option would be to move the code that populates the section
> load list into the mini dump dynamic loader.  That has the benefit of
> keeping this consistent with the other process plugins, but OTOH is just
> moving code around...
>
>
> Yes the dynamic loader plug-ins aren't hard to write and the code already
> exists in the ProcessMinidump. Then you would request the plug-in by name
> instead of passing a nullptr as the name in ProcessMinidump::
> GetDynamicLoader().
>
>
> Repository:
>   rLLDB LLDB
>
> https://reviews.llvm.org/D51176
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r340578 - Restrict the set of plugins used for ProcessMinidump

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Thu Aug 23 14:34:33 2018
New Revision: 340578

URL: http://llvm.org/viewvc/llvm-project?rev=340578=rev
Log:
Restrict the set of plugins used for ProcessMinidump

1. The dynamic loaders should not be needed for loading minidumps
and they may create problems (ex. the macOS loader resets the list of
loaded sections, which for minidumps are already set up during minidump loading)

2. In general, the extra plugins can do extraneous work which hurts performance
(ex. trying to set up implicit symbolic breakpoints, which in turn will trigger
extra debug information loading)

Differential Revision: https://reviews.llvm.org/D51176


Modified:
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.h

Modified: lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp?rev=340578=340577=340578=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp Thu Aug 23 
14:34:33 2018
@@ -16,7 +16,7 @@
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
-#include "lldb/Target/DynamicLoader.h"
+#include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
@@ -201,12 +201,6 @@ Status ProcessMinidump::DoLoadCore() {
   return error;
 }
 
-DynamicLoader *ProcessMinidump::GetDynamicLoader() {
-  if (m_dyld_ap.get() == nullptr)
-m_dyld_ap.reset(DynamicLoader::FindPlugin(this, nullptr));
-  return m_dyld_ap.get();
-}
-
 ConstString ProcessMinidump::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ProcessMinidump::GetPluginVersion() { return 1; }
@@ -401,3 +395,14 @@ bool ProcessMinidump::GetProcessInfo(Pro
   }
   return true;
 }
+
+// For minidumps there's no runtime generated code so we don't need 
JITLoader(s)
+// Avoiding them will also speed up minidump loading since JITLoaders normally
+// try to set up symbolic breakpoints, which in turn may force loading more
+// debug information than needed.
+JITLoaderList ::GetJITLoaders() {
+  if (!m_jit_loaders_ap) {
+m_jit_loaders_ap = llvm::make_unique();
+  }
+  return *m_jit_loaders_ap;
+}

Modified: lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.h?rev=340578=340577=340578=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.h (original)
+++ lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.h Thu Aug 23 
14:34:33 2018
@@ -55,7 +55,7 @@ public:
 
   Status DoLoadCore() override;
 
-  DynamicLoader *GetDynamicLoader() override;
+  DynamicLoader *GetDynamicLoader() override { return nullptr; }
 
   ConstString GetPluginName() override;
 
@@ -102,6 +102,8 @@ protected:
 
   void ReadModuleList();
 
+  JITLoaderList () override;
+
 private:
   FileSpec m_core_file;
   llvm::ArrayRef m_thread_list;


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

Re: [Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via lldb-commits]

Great timing! ARM support would be most welcome. Are you planning to
support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark
Mentovai just reminded me that the ARM support was added independently and
some of the structures are different)

Regarding the invalid minidumps, I used my favorite hex editor
 to manually corrupt the minidump
streams directory.

On Tue, Jul 17, 2018 at 10:36 AM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> I have an upcoming patch for adding ARM and ARM64 support to the minidump
> parser and i was curious how you created the invalid minidump files that
> are part of this patch?
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D49202
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via lldb-commits]

The problem is not returning an error from Minidump::Create() - if that was
the case this could easily be improved indeed. The two-phase initialization
is a consequence of the LLDB plugin lookup:

1. Target::CreateProcess() calls Process::FindPlugin()
2. ProcessMinidump::CreateInstance() then has to inspect the core file to
see if it's a minidump
2b. ... if it is a minidump, we need to create a ProcessMinidump (which
calls MinidumpParser::Create())
3. once the plugin is selected, Process::LoadCore() is finally called and
this the earliest we can do minidump-specific error checking

Note that at step 2b. we don't have a way to propagate the error since
we're just doing the plugin lookup (the most we can pass on the lookup to
the rest of the plugins). We can't easily defer the
MinidumpParser::Create() as step 2b either since that only morphs into a
different kind of two-stage initialization (having a ProcessMinidump w/o a
parser).

I agree that it would be nicer with a one step initialization but overall
changing the LLDB plugin lookup is too intrusive for the relatively small
benefit. If you have any suggestions I'd love to hear them.


On Mon, Jul 16, 2018 at 9:04 AM, Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added a comment.
>
> I don't agree with the two-stage initialization of the MinidumpParser
> class being introduced here. We deliberately introduced the `Create` static
> function to avoid this. If this `Initialize` function in checking
> invariants which are assumed to be hold by other parser methods, then it
> should be done by the `Create` function. Ideally this would be done before
> even constructing the parser object, but if this is impractical for some
> reason then you can make the `Initialize` function private and call it
> directly from `Create`. This way a user will never be able to see an
> malformed parser object. To make sure you propagate the error, you can
> change the return type of `Create` to `llvm::Expected (the only reason we did not do this back then was that there was no
> precedent for using `Expected` in LLDB, but this is no longer the case).
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D49202
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via lldb-commits]

That sounds reasonable to me. I'll make a note to revisit this (I don't the
the cycles to do it right away but I'm planning a few more changes in the
area soon).

On Mon, Jul 16, 2018 at 10:36 AM, Pavel Labath  wrote:

> Ok, I see what you mean now.
>
> Looking at the other core file plugins (elf, mach-o), it looks like
> they do only very basic verification before the accept the file. The
> pretty much just check the magic numbers, which would be more-or-less
> equivalent to our `MinidumpHeader::Parse` call. As this does not
> require creating a parser object, maybe we could delay the parser
> creation until `LoadCore` gets called (at which point you can easily
> report errors).
>
> This will leave us with a nice MinidumpParser interface.
> ProcessMinidump will still use two-stage initialization, but this is
> nothing new, and this change will make it easier for us to change the
> initialization method of the Process objects in the future.
>
> WDYT?
>
> On Mon, 16 Jul 2018 at 18:16, Leonard Mosescu  wrote:
> >
> > The problem is not returning an error from Minidump::Create() - if that
> was the case this could easily be improved indeed. The two-phase
> initialization is a consequence of the LLDB plugin lookup:
> >
> > 1. Target::CreateProcess() calls Process::FindPlugin()
> > 2. ProcessMinidump::CreateInstance() then has to inspect the core file
> to see if it's a minidump
> > 2b. ... if it is a minidump, we need to create a ProcessMinidump (which
> calls MinidumpParser::Create())
> > 3. once the plugin is selected, Process::LoadCore() is finally called
> and this the earliest we can do minidump-specific error checking
> >
> > Note that at step 2b. we don't have a way to propagate the error since
> we're just doing the plugin lookup (the most we can pass on the lookup to
> the rest of the plugins). We can't easily defer the
> MinidumpParser::Create() as step 2b either since that only morphs into a
> different kind of two-stage initialization (having a ProcessMinidump w/o a
> parser).
> >
> > I agree that it would be nicer with a one step initialization but
> overall changing the LLDB plugin lookup is too intrusive for the relatively
> small benefit. If you have any suggestions I'd love to hear them.
> >
> >
> > On Mon, Jul 16, 2018 at 9:04 AM, Pavel Labath via Phabricator <
> revi...@reviews.llvm.org> wrote:
> >>
> >> labath added a comment.
> >>
> >> I don't agree with the two-stage initialization of the MinidumpParser
> class being introduced here. We deliberately introduced the `Create` static
> function to avoid this. If this `Initialize` function in checking
> invariants which are assumed to be hold by other parser methods, then it
> should be done by the `Create` function. Ideally this would be done before
> even constructing the parser object, but if this is impractical for some
> reason then you can make the `Initialize` function private and call it
> directly from `Create`. This way a user will never be able to see an
> malformed parser object. To make sure you propagate the error, you can
> change the return type of `Create` to `llvm::Expected (the only reason we did not do this back then was that there was no
> precedent for using `Expected` in LLDB, but this is no longer the case).
> >>
> >>
> >> Repository:
> >>   rL LLVM
> >>
> >> https://reviews.llvm.org/D49202
> >>
> >>
> >>
> >
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

[Leonard Mosescu via lldb-commits]

>
> The problem is that shared libraries differ on these machines and
> LLDB either fails to load some libraries *or loads wrong ones*.
>

Not finding the modules is not surprising but the latter (loading the wrong
modules) is a bit concerning. Do you know why the module build-id is not
used when searching for the local binary?


On Tue, Jul 24, 2018 at 7:19 AM, Greg Clayton via Phabricator via
lldb-commits  wrote:

> clayborg requested changes to this revision.
> clayborg added a comment.
> This revision now requires changes to proceed.
>
> I think doing this in the module list is not the right place. Why? Some
> platforms might have multiple sysroot to check. iOS for instance has a
> directory for each device that Xcode has connected to which can be checked.
> I am fine with adding this ability to lldb_private::Platform, but I would
> just do it in there. Try GetRemoteSharedModule with the spec, if it fails,
> try again after modifying the spec to prepend the sysroot path. Possible
> even just check the sysroot path + path first if m_sdk_sysroot is filled
> in. I don't see the need to change ModuleList itself.
>
>
>
> 
> Comment at: include/lldb/Core/ModuleList.h:544-545
>  bool *did_create_ptr,
> -bool always_create = false);
> +bool always_create = false,
> +const char* sysroot = nullptr);
>static bool RemoveSharedModule(lldb::ModuleSP _sp);
> 
> Revert this? See my main comment
>
>
> 
> Comment at: source/Core/ModuleList.cpp:710-714
> +   bool *did_create_ptr, bool
> always_create,
> +   const char* sysroot) {
> +  // Make sure no one else can try and get or create a module while this
> +  // function is actively working on it by doing an extra lock on the
> +  // global mutex list.
> 
> Revert
>
>
> 
> Comment at: source/Core/ModuleList.cpp:766-770
> +  if (sysroot != nullptr)
> +resolved_module_spec.GetFileSpec().PrependPathComponent(sysroot);
> +
> +  module_sp.reset(new Module(resolved_module_spec));
>// Make sure there are a module and an object file since we can specify
> 
> Revert
>
>
> 
> Comment at: source/Target/Platform.cpp:228
>  module_spec, module_sp, module_search_paths_ptr,
> old_module_sp_ptr,
> -did_create_ptr, false);
> +did_create_ptr, false, m_sdk_sysroot.AsCString());
>return GetRemoteSharedModule(module_spec, process, module_sp,
> 
> Revert
>
>
> 
> Comment at: source/Target/Platform.cpp:230
>return GetRemoteSharedModule(module_spec, process, module_sp,
> [&](const ModuleSpec ) {
>   Status error =
> ModuleList::GetSharedModule(
> 
> Here just make a module spec that has m_sdk_sysroot prepended if
> m_sdk_sysroot is set, and check for that file first. If that succeeds,
> return that, else do the same code as before.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D49685
>
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files

[Leonard Mosescu via lldb-commits]

I'm curious too: where did the PDB70 age create matching problems?

On a related note, I just noticed that ObjectFilePECOFF::GetUUID() doesn't
have a real implementation (just returns false). How do we extract module
UUID for PE/COFF files?

On Wed, Aug 29, 2018 at 10:28 AM, Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added a subscriber: clayborg.
> zturner added a comment.
>
> For PE/COFF files, the Age is also in the executable and Guid+Age actually
> constitute a 20-byte UUID. Is this not the case on Apple? What object file
> format are you dealing with?
>
>
> https://reviews.llvm.org/D51442
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] r341080 - Remove redundant initialization

[Leonard Mosescu via lldb-commits]

Just curious, what prompted this change? (compiler diagnostic? forcing
value initialization in the member initializer list is harmless in this
case)

Also, if we want to do this kind of cleanup, m_compiler initialization is
also redundant.

On Thu, Aug 30, 2018 at 8:39 AM, Adrian Prantl via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> Author: adrian
> Date: Thu Aug 30 08:39:08 2018
> New Revision: 341080
>
> URL: http://llvm.org/viewvc/llvm-project?rev=341080=rev
> Log:
> Remove redundant initialization
>
> Modified:
> lldb/trunk/source/Plugins/ExpressionParser/Clang/
> ClangExpressionParser.cpp
>
> Modified: lldb/trunk/source/Plugins/ExpressionParser/Clang/
> ClangExpressionParser.cpp
> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/
> Plugins/ExpressionParser/Clang/ClangExpressionParser.
> cpp?rev=341080=341079=341080=diff
> 
> ==
> --- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
> (original)
> +++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
> Thu Aug 30 08:39:08 2018
> @@ -222,7 +222,7 @@ ClangExpressionParser::ClangExpressionPa
>   Expression ,
>   bool generate_debug_info)
>  : ExpressionParser(exe_scope, expr, generate_debug_info),
> m_compiler(),
> -  m_code_generator(), m_pp_callbacks(nullptr) {
> +  m_pp_callbacks(nullptr) {
>Log *log(lldb_private::GetLogIfAllCategoriesSet(
> LIBLLDB_LOG_EXPRESSIONS));
>
>// We can't compile expressions without a target.  So if the exe_scope
> is
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r336918 - Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Thu Jul 12 10:27:18 2018
New Revision: 336918

URL: http://llvm.org/viewvc/llvm-project?rev=336918=rev
Log:
Restructure the minidump loading path and add early & explicit consistency 
checks

Corrupted minidumps was leading to unpredictable behavior.

This change adds explicit consistency checks for the minidump early on. The
checks are not comprehensive but they should catch obvious structural 
violations:

streams with type == 0
duplicate streams (same type)
overlapping streams
truncated minidumps

Another early check is to make sure we actually support the minidump 
architecture
instead of crashing at a random place deep inside LLDB.

Differential Revision: https://reviews.llvm.org/D49202


Added:
lldb/trunk/unittests/Process/minidump/Inputs/bad_duplicate_streams.dmp   
(with props)
lldb/trunk/unittests/Process/minidump/Inputs/bad_overlapping_streams.dmp   
(with props)
Modified:
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/trunk/unittests/Process/minidump/CMakeLists.txt
lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp?rev=336918=336917=336918=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp Thu Jul 12 
10:27:18 2018
@@ -14,10 +14,13 @@
 
 // Other libraries and framework includes
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Utility/LLDBAssert.h"
 
 // C includes
 // C++ includes
+#include 
 #include 
+#include 
 
 using namespace lldb_private;
 using namespace minidump;
@@ -27,46 +30,11 @@ MinidumpParser::Create(const lldb::DataB
   if (data_buf_sp->GetByteSize() < sizeof(MinidumpHeader)) {
 return llvm::None;
   }
-
-  llvm::ArrayRef header_data(data_buf_sp->GetBytes(),
-  sizeof(MinidumpHeader));
-  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
-
-  if (header == nullptr) {
-return llvm::None;
-  }
-
-  lldb::offset_t directory_list_offset = header->stream_directory_rva;
-  // check if there is enough data for the parsing of the directory list
-  if ((directory_list_offset +
-   sizeof(MinidumpDirectory) * header->streams_count) >
-  data_buf_sp->GetByteSize()) {
-return llvm::None;
-  }
-
-  const MinidumpDirectory *directory = nullptr;
-  Status error;
-  llvm::ArrayRef directory_data(
-  data_buf_sp->GetBytes() + directory_list_offset,
-  sizeof(MinidumpDirectory) * header->streams_count);
-  llvm::DenseMap directory_map;
-
-  for (uint32_t i = 0; i < header->streams_count; ++i) {
-error = consumeObject(directory_data, directory);
-if (error.Fail()) {
-  return llvm::None;
-}
-directory_map[static_cast(directory->stream_type)] =
-directory->location;
-  }
-
-  return MinidumpParser(data_buf_sp, std::move(directory_map));
+  return MinidumpParser(data_buf_sp);
 }
 
-MinidumpParser::MinidumpParser(
-const lldb::DataBufferSP _buf_sp,
-llvm::DenseMap &_map)
-: m_data_sp(data_buf_sp), m_directory_map(directory_map) {}
+MinidumpParser::MinidumpParser(const lldb::DataBufferSP _buf_sp)
+: m_data_sp(data_buf_sp) {}
 
 llvm::ArrayRef MinidumpParser::GetData() {
   return llvm::ArrayRef(m_data_sp->GetBytes(),
@@ -480,3 +448,109 @@ MinidumpParser::GetMemoryRegionInfo(lldb
   // appear truncated.
   return info;
 }
+
+Status MinidumpParser::Initialize() {
+  Status error;
+
+  lldbassert(m_directory_map.empty());
+
+  llvm::ArrayRef header_data(m_data_sp->GetBytes(),
+  sizeof(MinidumpHeader));
+  const MinidumpHeader *header = MinidumpHeader::Parse(header_data);
+  if (header == nullptr) {
+error.SetErrorString("invalid minidump: can't parse the header");
+return error;
+  }
+
+  // A minidump without at least one stream is clearly ill-formed
+  if (header->streams_count == 0) {
+error.SetErrorString("invalid minidump: no streams present");
+return error;
+  }
+
+  struct FileRange {
+uint32_t offset = 0;
+uint32_t size = 0;
+
+FileRange(uint32_t offset, uint32_t size) : offset(offset), size(size) {}
+uint32_t end() const { return offset + size; }
+  };
+
+  const uint32_t file_size = m_data_sp->GetByteSize();
+
+  // Build a global minidump file map, checking for:
+  // - overlapping streams/data structures
+  // - truncation (streams pointing past the end of file)
+  std::vector minidump_map;
+
+  // Add the minidump header to the file map
+  if (sizeof(MinidumpHeader) > file_size) {
+   

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

[Leonard Mosescu via lldb-commits]

Greg/Pavel, does the latest revision look good to you? Thanks!

On Wed, Apr 18, 2018 at 10:31 AM, Leonard Mosescu via Phabricator <
revi...@reviews.llvm.org> wrote:

> lemo marked an inline comment as done.
> lemo added a comment.
>
> In https://reviews.llvm.org/D45700#1070491, @amccarth wrote:
>
> > LGTM, but consider highlighting the side effect to `target` when the
> factory makes a Placeholder module.
>
>
> Good observation: taking a step back, the factory introduces too much
> coupling, especially if we want to extend this placeholder module approach
> to other uses. Because of this, I reverted back to the standalone
> PlaceholderModule::CreateImageSection() approach. Thanks Adrian!
>
>
>
> 
> Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:70
> +  // Creates a synthetic module section covering the whole module image
> +  void CreateImageSection(const MinidumpModule *module, Target& target) {
> +const ConstString section_name(".module_image");
> 
> amccarth wrote:
> > I didn't notice before that target is a non-const ref.  Maybe the
> comment should explain why target is modified (and/or maybe in
> PlaceholderModule::Create).
> Updated the function comment. This is similar to how other places set the
> section load address (ex. ObjectFileELF::SetLoadAddress)
>
>
> https://reviews.llvm.org/D45700
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Leonard Mosescu via lldb-commits]

>
> It looks like nobody except me is worried about the
> module-without-an-object-file situation, so I guess we can try this out and
> see how it goes.
>

Sorry Pavel, I meant to respond to this: most of the code seems to
explicitly handle this case (module-without-object-file), I just had to fix
a couple of cases. It's possible that more fixes will be required, but the
intention seems to be to accommodate for missing object files so at least
in this area I didn't have to break new ground.

I also considered creating placeholder object files, but it proved a bit
more intrusive since there are numerous places where it's assumed that
object files map to a real file which can be read and written to. Maybe at
some point we'll need to reconsider this (placeholder object files) but for
the initial iteration the placeholder modules seems to be sufficient. The
only downside I noticed is mostly cosmetic, for example things like "target
modules dump objfile" may print empty lines for the missing object files.

The test you've added here has been failing on windows though. I've tried
> to fix this in r330314, but it meant modifying the module.file.fullpath
> output expectations. I'm not sure where you're going with the minidump
> support, but if you are bothered by module.file.fullpath containing a
> forward slash, you may want to look into fixing the SBFileSpec behavior.
>

Thanks!


On Thu, Apr 19, 2018 at 2:47 AM, Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath added a comment.
>
> It looks like nobody except me is worried about the
> module-without-an-object-file situation, so I guess we can try this out and
> see how it goes.
>
> The test you've added here has been failing on windows though. I've tried
> to fix this in r330314, but it meant modifying the module.file.fullpath
> output expectations. I'm not sure where you're going with the minidump
> support, but if you are bothered by module.file.fullpath containing a
> forward slash, you may want to look into fixing the SBFileSpec behavior.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D45700
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] r330314 - Attempt to fix TestMiniDump on windows

[Leonard Mosescu via lldb-commits]

The mix of backward and forward slashes doesn't impact my current project
but it would be nice to have a consistent path syntax (both within a single
path and also cross platforms).


> Leonard, is it reasonable to assume that all paths in the minidumps will be
> absolute (and thus resolving is pointless anyway)?


Yes. Mostly :) For non-Windows minidumps the way we capture module names is
a bit fuzzy (we depend on loader data structures and things like
/proc/self/maps).

What exactly does "resolving the path" means here? Breaking down into path
components and re-assembling it doesn't seem particularly interesting.

We should probably fix the "fullpath" property to do something smarter in
> the future.


I agree, it would be nice to avoid hard coding path separators.

On Thu, Apr 19, 2018 at 9:51 AM, Pavel Labath  wrote:

> Yes, I noticed that as well, but I did not want to change it, as it wasn't
> related to the problem I was trying to fix. I agree that not resolving the
> path sounds like a better idea.
>
> Leonard, is it reasonable to assume that all paths in the minidumps will be
> absolute (and thus resolving is pointless anyway)?
> On Thu, 19 Apr 2018 at 17:20, Greg Clayton  wrote:
>
> > Any reason we are trying to resolve the path with the 2nd true argument
> to FileSpec? If you still want to resolve the path, I would suggest
> checking if GetArchitecture() is compatible with any host architectures
> before passing true.
>
>
> > > On Apr 19, 2018, at 2:38 AM, Pavel Labath via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> > >
> > > Author: labath
> > > Date: Thu Apr 19 02:38:42 2018
> > > New Revision: 330314
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=330314=rev
> > > Log:
> > > Attempt to fix TestMiniDump on windows
> > >
> > > It was failing because the modules names were coming out as
> > > C:\Windows\System32/MSVCP120D.dll (last separator is a forward slash)
> on
> > > windows.
> > >
> > > There are two issues at play here:
> > > - the first problem is that the paths in minidump were being parsed as
> a
> > >  host path. This meant that on posix systems the whole path was
> > >  interpreted as a file name.
> > > - on windows the path was split into a directory-filename pair
> > >  correctly, but then when it was reconsituted, the last separator ended
> > >  up being a forward slash because SBFileSpec.fullpath was joining them
> > >  with '/' unconditionally.
> > >
> > > I fix the first issue by parsing the minidump paths according to the
> > > path syntax of the host which produced the dump, which should make the
> > > test behavior on posix identical. The last path will still be a
> > > forward slash because of the second issue. We should probably fix the
> > > "fullpath" property to do something smarter in the future.
> > >
> > > Modified:
> > >
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/
> minidump/TestMiniDump.py
> > >lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
> > >
> > > Modified:
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/
> minidump/TestMiniDump.py
> > > URL:
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/
> Python/lldbsuite/test/functionalities/postmortem/
> minidump/TestMiniDump.py?rev=330314=330313=330314=diff
> > >
> 
> ==
> > > ---
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/
> minidump/TestMiniDump.py
> (original)
> > > +++
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/
> minidump/TestMiniDump.py
> Thu Apr 19 02:38:42 2018
> > > @@ -49,12 +49,12 @@ class MiniDumpTestCase(TestBase):
> > > self.process = self.target.LoadCore("fizzbuzz_no_heap.dmp")
> > > self.assertTrue(self.process, PROCESS_IS_VALID)
> > > expected_modules = [
> > > -r"C:\Windows\System32\MSVCP120D.dll",
> > > -r"C:\Windows\SysWOW64\kernel32.dll",
> > > -r"C:\Users\amccarth\Documents\Visual Studio
> 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe",
> > > -r"C:\Windows\System32\MSVCR120D.dll",
> > > -r"C:\Windows\SysWOW64\KERNELBASE.dll",
> > > -r"C:\Windows\SysWOW64\ntdll.dll",
> > > +r"C:\Windows\System32/MSVCP120D.dll",
> > > +r"C:\Windows\SysWOW64/kernel32.dll",
> > > +r"C:\Users\amccarth\Documents\Visual Studio
> 2013\Projects\fizzbuzz\Debug/fizzbuzz.exe",
> > > +r"C:\Windows\System32/MSVCR120D.dll",
> > > +r"C:\Windows\SysWOW64/KERNELBASE.dll",
> > > +r"C:\Windows\SysWOW64/ntdll.dll",
> > > ]
> > > self.assertEqual(self.target.GetNumModules(),
> len(expected_modules))
> > > for module, expected in zip(self.target.modules,
> expected_modules):
> > >
> > > Modified: lldb/trunk/source/Plugins/Process/minidump/
> ProcessMinidump.cpp
> > > URL:
> 

Re: [Lldb-commits] [PATCH] D45628: [LLDB] Support compressed debug info sections (.zdebug*)

[Leonard Mosescu via lldb-commits]

Hi Erik, the review is still marked as requiring changes. Once that is
sorted out I'd be happy to submit this on your behalf (what is the base SVN
revision for the latest patch?)

Davide Italiano, is all the CR feedback addressed in the latest revision?

On Tue, Apr 24, 2018 at 1:38 PM, Erik Welander via Phabricator via
lldb-commits  wrote:

> alur added a comment.
>
> Friendly ping, is there anything else I need to do for this to get
> submitted?
>
>
> https://reviews.llvm.org/D45628
>
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r330302 - Improve LLDB's handling of non-local minidumps

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Wed Apr 18 16:10:46 2018
New Revision: 330302

URL: http://llvm.org/viewvc/llvm-project?rev=330302=rev
Log:
Improve LLDB's handling of non-local minidumps

Normally, LLDB is creating a high-fidelity representation of a live
process, including a list of modules and sections, with the 
associated memory address ranges. In order to build the module and
section map LLDB tries to locate the local module image (object file)
and will parse it.

This does not work for postmortem debugging scenarios where the crash
dump (minidump in this case) was captured on a different machine.

Fortunately the minidump format encodes enough information about
each module's memory range to allow us to create placeholder modules.
This enables most LLDB functionality involving address-to-module
translations.

Also, we may want to completly disable the search for matching
local object files if we load minidumps unless we can prove that the
local image matches the one from the crash origin.
(not part of this change, see: llvm.org/pr35193)

Example: Identify the module from a stack frame PC:

Before:
  thread #1, stop reason = Exception 0xc005 encountered at address 0x164d14
frame #0: 0x00164d14
frame #1: 0x00167c79
frame #2: 0x00167e6d
frame #3: 0x7510336a
frame #4: 0x77759882
frame #5: 0x77759855

After:
  thread #1, stop reason = Exception 0xc005 encountered at address 0x164d14
frame #0: 0x00164d14 C:\Users\amccarth\Documents\Visual Studio 
2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #1: 0x00167c79 C:\Users\amccarth\Documents\Visual Studio 
2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #2: 0x00167e6d C:\Users\amccarth\Documents\Visual Studio 
2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #3: 0x7510336a C:\Windows\SysWOW64\kernel32.dll
frame #4: 0x77759882 C:\Windows\SysWOW64\ntdll.dll
frame #5: 0x77759855 C:\Windows\SysWOW64\ntdll.dll

Example: target modules list

Before:
error: the target has no associated executable images

After:
[ 0] C:\Windows\System32\MSVCP120D.dll 
[ 1] C:\Windows\SysWOW64\kernel32.dll 
[ 2] C:\Users\amccarth\Documents\Visual Studio 
2013\Projects\fizzbuzz\Debug\fizzbuzz.exe 
[ 3] C:\Windows\System32\MSVCR120D.dll 
[ 4] C:\Windows\SysWOW64\KERNELBASE.dll 
[ 5] C:\Windows\SysWOW64\ntdll.dll

NOTE: the minidump format also includes the debug info GUID, so we can
fill-in the module UUID from it, but this part was excluded from this change
to keep the changes simple (the LLDB UUID is hardcoded to be either 16 or
20 bytes, while the CodeView GUIDs are normally 24 bytes)

Differential Revision: https://reviews.llvm.org/D45700


Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py
lldb/trunk/source/Commands/CommandObjectTarget.cpp
lldb/trunk/source/Core/Module.cpp
lldb/trunk/source/Core/Section.cpp
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.h

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py?rev=330302=330301=330302=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
 Wed Apr 18 16:10:46 2018
@@ -70,6 +70,17 @@ class MiniDumpNewTestCase(TestBase):
 self.assertEqual(self.process.GetProcessID(), self._linux_x86_64_pid)
 self.check_state()
 
+def test_modules_in_mini_dump(self):
+"""Test that lldb can read the list of modules from the minidump."""
+# target create -c linux-x86_64.dmp
+self.dbg.CreateTarget(None)
+self.target = self.dbg.GetSelectedTarget()
+self.process = self.target.LoadCore("linux-x86_64.dmp")
+self.assertTrue(self.process, PROCESS_IS_VALID)
+self.assertEqual(self.target.GetNumModules(), 9)
+for module in self.target.modules:
+self.assertTrue(module.IsValid())
+
 def test_thread_info_in_minidump(self):
 """Test that lldb can read the thread information from the Minidump."""
 # target create -c linux-x86_64.dmp
@@ -100,6 +111,7 @@ class MiniDumpNewTestCase(TestBase):
 self.assertEqual(thread.GetNumFrames(), 2)
 frame = thread.GetFrameAtIndex(0)
 self.assertTrue(frame.IsValid())
+self.assertTrue(frame.GetModule().IsValid())
 pc = frame.GetPC()
 eip = frame.FindRegister("pc")
 self.assertTrue(eip.IsValid())

Modified: 

Re: [Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via lldb-commits]

Hi Greg, looking at request_evaluate() I noticed that it will evaluate the
string as a lldb command if prefixed by ` .

This is a great feature (it allows building REPL consoles on top of DAP),
but I'm curious how you picked up this convention? For example I believe
that the gdb DAP uses -exec 'command' instead.

On Thu, Aug 16, 2018 at 11:01 AM, Phabricator via Phabricator <
revi...@reviews.llvm.org> wrote:

> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL339911: Add a new tool named lldb-vscode
> that implements the Visual Studio Code Debug… (authored by gclayton,
> committed by ).
> Herald added a subscriber: llvm-commits.
>
> Changed prior to commit:
>   https://reviews.llvm.org/D50365?vs=161058=161067#toc
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D50365
>
> Files:
>   lldb/trunk/lldb.xcodeproj/project.pbxproj
>   lldb/trunk/packages/Python/lldbsuite/test/dotest.py
>   lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/.categories
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/attach/Makefile
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/attach/TestVSCode_attach.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/attach/main.c
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/breakpoint/Makefile
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/breakpoint/TestVSCode_setBreakpoints.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/breakpoint/TestVSCode_setExceptionBreakpoints.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/breakpoint/TestVSCode_setFunctionBreakpoints.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/breakpoint/main.cpp
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/launch/Makefile
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/launch/TestVSCode_launch.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/launch/main.c
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/lldbvscode_testcase.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/stackTrace/Makefile
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/stackTrace/TestVSCode_stackTrace.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/stackTrace/main.c
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/step/Makefile
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/step/TestVSCode_step.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/step/main.cpp
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/variables/Makefile
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/variables/TestVSCode_variables.py
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-
> vscode/variables/main.cpp
>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
>   lldb/trunk/tools/CMakeLists.txt
>   lldb/trunk/tools/lldb-vscode/BreakpointBase.cpp
>   lldb/trunk/tools/lldb-vscode/BreakpointBase.h
>   lldb/trunk/tools/lldb-vscode/CMakeLists.txt
>   lldb/trunk/tools/lldb-vscode/ExceptionBreakpoint.cpp
>   lldb/trunk/tools/lldb-vscode/ExceptionBreakpoint.h
>   lldb/trunk/tools/lldb-vscode/FunctionBreakpoint.cpp
>   lldb/trunk/tools/lldb-vscode/FunctionBreakpoint.h
>   lldb/trunk/tools/lldb-vscode/JSONUtils.cpp
>   lldb/trunk/tools/lldb-vscode/JSONUtils.h
>   lldb/trunk/tools/lldb-vscode/LLDBUtils.cpp
>   lldb/trunk/tools/lldb-vscode/LLDBUtils.h
>   lldb/trunk/tools/lldb-vscode/README.md
>   lldb/trunk/tools/lldb-vscode/SourceBreakpoint.cpp
>   lldb/trunk/tools/lldb-vscode/SourceBreakpoint.h
>   lldb/trunk/tools/lldb-vscode/SourceReference.h
>   lldb/trunk/tools/lldb-vscode/VSCode.cpp
>   lldb/trunk/tools/lldb-vscode/VSCode.h
>   lldb/trunk/tools/lldb-vscode/VSCodeForward.h
>   lldb/trunk/tools/lldb-vscode/lldb-vscode-Info.plist
>   lldb/trunk/tools/lldb-vscode/lldb-vscode.cpp
>   lldb/trunk/tools/lldb-vscode/package.json
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via lldb-commits]

Great. Do you think that having an abstracted stream I/O tunneled through
DAP would lose anything compared to the direct file handle? I can see a few
problems using a native platform file handle:

1. It's specific to the platform (with subtle differences between platforms)
2. It assumes a local setup, where the consumer and DAP implementation are
running on the same machine, likely in the context of the same user.

If DAP had some basic read/write stream packets you can easily build a
flexible remote setup. What do you think?

On Fri, Sep 21, 2018 at 10:39 AM, Greg Clayton  wrote:

>
>
> On Sep 21, 2018, at 10:31 AM, Leonard Mosescu  wrote:
>
> The solution I would love to see is to have the initialize packet return
>> something via the DAP that says "I have a command line interpreter, please
>> send a packet with a file handle (slave path to slave side of pseudo
>> terminal maybe)". VS Code and Nuclide both emulated tty already, so we
>> could have a true command line that exposes the "(gdb)" prompt for GDB and
>> "(lldb)" for lldb and all the power that comes with it.
>>
>
> I agree, that's the main reason I asked. Having a per-DAP convention seems
> unfortunate. Also, with the current DAP messages it doesn't seem we can do
> things like auto-complete or passing control characters to the debugger
> first (ex Ctrl-C).
>
> I like your idea to manage an extra file handle. What if DAP offered
> "console stream" packets? Yet another option would be going around DAP
> completely for the pseudo terminal functionality, although it would be nice
> if everything can be built on top of DAP.
>
>
> Yeah there are two solutions to get a command line:
> 1 - have initialize packet return "I need a console named 'LLDB Commands'"
> and then have the IDE be able to switch from "Debugger Console" (which
> evaluates expressions over to "LLDB Commands" and it repurposes the
> existing debugger console to just send LLDB commands. The user would be
> able to switch between the two.
> 2 - Use a file handle to the terminals that are supported in the IDEs. The
> benefit here is the ability to due curses style output where you can
> control the cursor position and emit color control codes to colorize output.
>
>
> Not urgent but perhaps we can use LLDB to design and prototype such a DAP
> extension, then propose it to the vscode team? Is this something you'd be
> interested in?
>
>
> I would be yes. I would love the ability to due approach #2 by adding
> something to the reply to the initialize packet and if the IDE supports it,
> they will send down a file handle that could be used. Right now their TTY
> support seems to be centered around running a sub process only (like
> /bin/sh or other command line executable), not around, just give me a
> terminal and a file handle I can read and write to.
>
> Greg Clayton
>
>
>
> On Fri, Sep 21, 2018 at 9:56 AM, Greg Clayton  wrote:
>
>>
>>
>> On Sep 20, 2018, at 3:05 PM, Leonard Mosescu  wrote:
>>
>> Hi Greg, looking at request_evaluate() I noticed that it will evaluate
>> the string as a lldb command if prefixed by ` .
>>
>> This is a great feature (it allows building REPL consoles on top of DAP),
>> but I'm curious how you picked up this convention? For example I believe
>> that the gdb DAP uses -exec 'command' instead.
>>
>>
>> ` is an illegal expression character so it won't stop you from evaluating
>> any possible expression. The gdb prefix "-exec" stops you from being able
>> to negate a local variable named "exec". Not a huge deal.  So I just picked
>> a good prefix character that wouldn't stop anyone from evaluating any valid
>> expression (at least in C/C++/ObjC/Swift).
>>
>> The solution I would love to see is to have the initialize packet return
>> something via the DAP that says "I have a command line interpreter, please
>> send a packet with a file handle (slave path to slave side of pseudo
>> terminal maybe)". VS Code and Nuclide both emulated tty already, so we
>> could have a true command line that exposes the "(gdb)" prompt for GDB and
>> "(lldb)" for lldb and all the power that comes with it.
>>
>> I needed something that could run LLDB commands when things go wrong to
>> do trouble shooting (enable logging, run commands to dump vital
>> information) so I hacked it in with `
>>
>> Greg
>>
>>
>> On Thu, Aug 16, 2018 at 11:01 AM, Phabricator via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>>
>>> This revision was automatically updated to reflect the committed changes.
>>> Closed by commit rL339911: Add a new tool named lldb-vscode
>>> that implements the Visual Studio Code Debug… (authored by gclayton,
>>> committed by ).
>>> Herald added a subscriber: llvm-commits.
>>>
>>> Changed prior to commit:
>>>   https://reviews.llvm.org/D50365?vs=161058=161067#toc
>>>
>>> Repository:
>>>   rL LLVM
>>>
>>> https://reviews.llvm.org/D50365
>>>
>>> Files:
>>>   lldb/trunk/lldb.xcodeproj/project.pbxproj
>>>   lldb/trunk/packages/Python/lldbsuite/test/dotest.py
>>>   

Re: [Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via lldb-commits]

Thanks Greg, this is what I had in mind.

I have some ideas which involve this kind of debugger console. We'll likely
start with a basic version built on top of evaluate `cmd and once we get
around to building a full console I'll ping you.

On Sat, Sep 22, 2018 at 8:50 AM, Greg Clayton  wrote:

>
>
> On Sep 21, 2018, at 1:20 PM, Leonard Mosescu  wrote:
>
> Great. Do you think that having an abstracted stream I/O tunneled through
> DAP would lose anything compared to the direct file handle? I can see a few
> problems using a native platform file handle:
>
> 1. It's specific to the platform (with subtle differences between
> platforms)
> 2. It assumes a local setup, where the consumer and DAP implementation are
> running on the same machine, likely in the context of the same user.
>
> If DAP had some basic read/write stream packets you can easily build a
> flexible remote setup. What do you think?
>
>
> Yeah, I forgot about remote. Maybe best to set it all up through DAP and
> just tell the IDE that you have a command interpreter and maybe what its
> prompt should be.
>
> The init packet would be sent:
>
> {"command":"initialize","type":"request","seq":1, "arguments":{...}}
>
> Then in the response we would say we have an interpreter:
>
> {"command":"initialize","type":"response", "request_seq":1,"seq":0,"
> success":true,"body":{
> "supportsCommandInterpreter":true,
> "interpreterPrompt":"(lldb)"
>   }
> }
>
>
> Then the IDE would need to support output going into (STDIN) that would go
> to the command interpreter somehow, and accept output coming from the
> command interpreter via the "output" event by adding support for a new
> output type "interpreter":
>
> {"event":"output","seq":0,"type":"event", 
> "body":{"category":"interpreter","output":"output
> text here"}}
>
> Then the IDE would only need either have another window for the
> interpreter, or allow the standard debugger console to be switched over
> into interpreter mode and show both outputs there.
>
>
>
> On Fri, Sep 21, 2018 at 10:39 AM, Greg Clayton  wrote:
>
>>
>>
>> On Sep 21, 2018, at 10:31 AM, Leonard Mosescu  wrote:
>>
>> The solution I would love to see is to have the initialize packet return
>>> something via the DAP that says "I have a command line interpreter, please
>>> send a packet with a file handle (slave path to slave side of pseudo
>>> terminal maybe)". VS Code and Nuclide both emulated tty already, so we
>>> could have a true command line that exposes the "(gdb)" prompt for GDB and
>>> "(lldb)" for lldb and all the power that comes with it.
>>>
>>
>> I agree, that's the main reason I asked. Having a per-DAP convention
>> seems unfortunate. Also, with the current DAP messages it doesn't seem we
>> can do things like auto-complete or passing control characters to the
>> debugger first (ex Ctrl-C).
>>
>> I like your idea to manage an extra file handle. What if DAP offered
>> "console stream" packets? Yet another option would be going around DAP
>> completely for the pseudo terminal functionality, although it would be nice
>> if everything can be built on top of DAP.
>>
>>
>> Yeah there are two solutions to get a command line:
>> 1 - have initialize packet return "I need a console named 'LLDB
>> Commands'" and then have the IDE be able to switch from "Debugger Console"
>> (which evaluates expressions over to "LLDB Commands" and it repurposes the
>> existing debugger console to just send LLDB commands. The user would be
>> able to switch between the two.
>> 2 - Use a file handle to the terminals that are supported in the IDEs.
>> The benefit here is the ability to due curses style output where you can
>> control the cursor position and emit color control codes to colorize output.
>>
>>
>> Not urgent but perhaps we can use LLDB to design and prototype such a DAP
>> extension, then propose it to the vscode team? Is this something you'd be
>> interested in?
>>
>>
>> I would be yes. I would love the ability to due approach #2 by adding
>> something to the reply to the initialize packet and if the IDE supports it,
>> they will send down a file handle that could be used. Right now their TTY
>> support seems to be centered around running a sub process only (like
>> /bin/sh or other command line executable), not around, just give me a
>> terminal and a file handle I can read and write to.
>>
>> Greg Clayton
>>
>>
>>
>> On Fri, Sep 21, 2018 at 9:56 AM, Greg Clayton  wrote:
>>
>>>
>>>
>>> On Sep 20, 2018, at 3:05 PM, Leonard Mosescu  wrote:
>>>
>>> Hi Greg, looking at request_evaluate() I noticed that it will evaluate
>>> the string as a lldb command if prefixed by ` .
>>>
>>> This is a great feature (it allows building REPL consoles on top of
>>> DAP), but I'm curious how you picked up this convention? For example I
>>> believe that the gdb DAP uses -exec 'command' instead.
>>>
>>>
>>> ` is an illegal expression character so it won't stop you from
>>> evaluating any possible expression. The gdb prefix "-exec" stops you from
>>> 

Re: [Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Leonard Mosescu via lldb-commits]

>
> The solution I would love to see is to have the initialize packet return
> something via the DAP that says "I have a command line interpreter, please
> send a packet with a file handle (slave path to slave side of pseudo
> terminal maybe)". VS Code and Nuclide both emulated tty already, so we
> could have a true command line that exposes the "(gdb)" prompt for GDB and
> "(lldb)" for lldb and all the power that comes with it.
>

I agree, that's the main reason I asked. Having a per-DAP convention seems
unfortunate. Also, with the current DAP messages it doesn't seem we can do
things like auto-complete or passing control characters to the debugger
first (ex Ctrl-C).

I like your idea to manage an extra file handle. What if DAP offered
"console stream" packets? Yet another option would be going around DAP
completely for the pseudo terminal functionality, although it would be nice
if everything can be built on top of DAP.

Not urgent but perhaps we can use LLDB to design and prototype such a DAP
extension, then propose it to the vscode team? Is this something you'd be
interested in?


On Fri, Sep 21, 2018 at 9:56 AM, Greg Clayton  wrote:

>
>
> On Sep 20, 2018, at 3:05 PM, Leonard Mosescu  wrote:
>
> Hi Greg, looking at request_evaluate() I noticed that it will evaluate the
> string as a lldb command if prefixed by ` .
>
> This is a great feature (it allows building REPL consoles on top of DAP),
> but I'm curious how you picked up this convention? For example I believe
> that the gdb DAP uses -exec 'command' instead.
>
>
> ` is an illegal expression character so it won't stop you from evaluating
> any possible expression. The gdb prefix "-exec" stops you from being able
> to negate a local variable named "exec". Not a huge deal.  So I just picked
> a good prefix character that wouldn't stop anyone from evaluating any valid
> expression (at least in C/C++/ObjC/Swift).
>
> The solution I would love to see is to have the initialize packet return
> something via the DAP that says "I have a command line interpreter, please
> send a packet with a file handle (slave path to slave side of pseudo
> terminal maybe)". VS Code and Nuclide both emulated tty already, so we
> could have a true command line that exposes the "(gdb)" prompt for GDB and
> "(lldb)" for lldb and all the power that comes with it.
>
> I needed something that could run LLDB commands when things go wrong to do
> trouble shooting (enable logging, run commands to dump vital information)
> so I hacked it in with `
>
> Greg
>
>
> On Thu, Aug 16, 2018 at 11:01 AM, Phabricator via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> This revision was automatically updated to reflect the committed changes.
>> Closed by commit rL339911: Add a new tool named lldb-vscode
>> that implements the Visual Studio Code Debug… (authored by gclayton,
>> committed by ).
>> Herald added a subscriber: llvm-commits.
>>
>> Changed prior to commit:
>>   https://reviews.llvm.org/D50365?vs=161058=161067#toc
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D50365
>>
>> Files:
>>   lldb/trunk/lldb.xcodeproj/project.pbxproj
>>   lldb/trunk/packages/Python/lldbsuite/test/dotest.py
>>   lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/.categories
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> attach/Makefile
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> attach/TestVSCode_attach.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> attach/main.c
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> breakpoint/Makefile
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> breakpoint/TestVSCode_setBreakpoints.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> breakpoint/TestVSCode_setExceptionBreakpoints.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> breakpoint/TestVSCode_setFunctionBreakpoints.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> breakpoint/main.cpp
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> launch/Makefile
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> launch/TestVSCode_launch.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> launch/main.c
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> lldbvscode_testcase.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> stackTrace/Makefile
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> stackTrace/TestVSCode_stackTrace.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> stackTrace/main.c
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> step/Makefile
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> step/TestVSCode_step.py
>>   lldb/trunk/packages/Python/lldbsuite/test/tools/lldb-vscode/
>> 

Re: [Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

[Leonard Mosescu via lldb-commits]

>
> After some internal discussion, it seems that the situation with the
> all-zero UUIDs is as follows:
> - breakpad symbol files do not attach a special meaning to a zero UUID -
> if a file does not have a build-id, the dump_syms tool will use a hash of
> the first page of the text section (or something equally silly)
> - minidump files may treat the missing build-id by replacing it with
> zeroes depending on the tool used to produce the minidump: breakpad doesn't
> do that (it does the same hash as above), crashpad does.
> So it seems like there is nothing to do here. Maybe the UUID reading code
> in ProcessMinidump needs revising though.
>

I agree. Sorry for mixing the minidump UUID case with the Breakpad symbol
files UUIDs.

On Wed, Jan 23, 2019 at 1:23 PM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath marked 5 inline comments as done.
> labath added a comment.
>
> After some internal discussion, it seems that the situation with the
> all-zero UUIDs is as follows:
>
> - breakpad symbol files do not attach a special meaning to a zero UUID -
> if a file does not have a build-id, the dump_syms tool will use a hash of
> the first page of the text section (or something equally silly)
> - minidump files may treat the missing build-id by replacing it with
> zeroes depending on the tool used to produce the minidump: breakpad doesn't
> do that (it does the same hash as above), crashpad does.
>
> So it seems like there is nothing to do here. Maybe the UUID reading code
> in ProcessMinidump needs revising though.
>
>
>
> 
> Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:69
> +  llvm::StringRef chunk = str.take_front(hex_digits());
> +  uintmax_t t;
> +  if (!to_integer(chunk, t, 16))
> 
> lemo wrote:
> > = 0; ?
> That is not necessary, as to_integer initializes it. Perhaps more
> importantly, not initializing this allows tools like msan and valgrind to
> actually detect the cases when you end up using an uninitialized value.
>
>
> 
> Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:48
>
>  class ModuleRecord : public Record {
>  public:
> 
> lemo wrote:
> > coding-convention-wise: should these definitions use struct instead of
> class?
> I don't think we have a strict rule about this case, but personally, when
> something starts using inheritance, I tend to think of it as a class.
>
>
> 
> Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:59
>
> -bool operator==(const ModuleRecord , const ModuleRecord );
> +bool operator==(const ModuleRecord , const ModuleRecord ) {
> +  return L.OS == R.OS && L.Arch == R.Arch && L.ID == R.ID;
> 
> lemo wrote:
> > const method qualifier?
> This is a free function, not a method. :)
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D57037/new/
>
> https://reviews.llvm.org/D57037
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://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

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

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

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

[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, 

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

[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

[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

[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

[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

[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

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

[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] D53368: [Symbol] Search symbols with name and type in a symbol file

[Leonard Mosescu via lldb-commits]

Hi Aleksandr, yes, no objections to this patch.

I was responding to Pavel's comments, which I also assume are
forward-looking as well, not strictly related to this patch.

On Thu, Nov 29, 2018 at 12:08 PM Aleksandr Urakov <
aleksandr.ura...@jetbrains.com> wrote:

> Yes, I agree that the current model of object files - symbol files -
> symtabs is not clear and orthogonal for cases like PDB, and I think that it
> requires some redesign too.
>
> But can we commit this patch now to proceed with several dependent (and
> not directly related to the theme) patches?
>
> When a new model of work with symbol files will be approved I'm ready to
> help with fixing the PDB part to conform it.
>
> Am Do., 29. Nov. 2018, 22:18 hat Leonard Mosescu 
> geschrieben:
>
>> Great observations Pavel! I think it's really important to have
>> orthogonal/composable abstractions here: the symbols should be decoupled
>> from the container format IMO.
>>
>> You know more about the ObjectFile than me so I can't say if
>> ObjectFileBreakpad is the best interface, but here are my initial
>> observations (in a somewhat random order):
>>
>> 1. We need clear and separate abstractions for a container (ELF, PE file,
>> Breakpad symbols) vs. the content (debug Information).
>>
>> 2. We need to be able to consume symbols when the corresponding module
>> binary is not available. This is common for postmortem debugging (ex.
>> having a minidump + PDBs, but not all the .DLLs or EXE files).
>>
>> 3. I'm not a fan of the PDB model, where the symbols are searched in both
>> the symtabs then in the symbol files. I'd rather like to see the symtab an
>> interface for symbols regardless of where they come from.
>>(Zach expressed some efficiency concerns if we'd need to build a
>> symtab from a PDB for example as opposed to accessing the PDB symfile
>> directly, although I think we can design to address this - ie. multiple
>> concrete symtab implementations, some of which are *internally* aware of
>> the source container, but w/o leaking this through the interface)
>>
>> 4. The symbol vendor observation is very important. Right now LLDB has
>> basic support for looking up DWARF symbols and none for PDBs. (for example,
>> IMO LLDB could greatly benefit from something like Microsoft's symsrv - I'm
>> actually planning to look into it soon)
>>   (Whatever we do, this should be one of the key requirements IMO)
>>
>> I have a local change to address #2 specifically for PDBs (I'll try to
>> send out a code review soon).
>>
>> On Thu, Nov 29, 2018 at 10:55 AM Pavel Labath via Phabricator via
>> lldb-commits  wrote:
>>
>>> labath added a comment.
>>>
>>> I've recently started looking at adding a new symbol file format
>>> (breakpad symbols). While researching the best way to achieve that, I
>>> started comparing the operation of PDB and DWARF symbol files. I noticed a
>>> very important difference there, and I think that is the cause of our
>>> problems here. In the DWARF implementation, a symbol file is an overlay on
>>> top of an object file - it takes the data contained by the object file and
>>> presents it in a more structured way.
>>>
>>> However, that is not the case with PDB (both implementations). These
>>> take the debug information from a completely different file, which is not
>>> backed by an ObjectFile instance, and then present that. Since the
>>> SymbolFile interface requires them to be backed by an object file, they
>>> both pretend they are backed by the original EXE file, but in reality the
>>> data comes from elsewhere.
>>>
>>> If we had an ObjectFilePDB (which not also not ideal, though in a way it
>>> is a better fit to the current lldb organization), then this could expose
>>> the PDB symtab via the existing ObjectFile interface and we could reuse the
>>> existing mechanism for merging symtabs from two object files.
>>>
>>> I am asking this because now I am facing a choice in how to implement
>>> breakpad symbols. I could go the PDB way, and read the symbols without an
>>> intervening object file, or I could create an ObjectFileBreakpad and then
>>> (possibly) a SymbolFileBreakpad sitting on top of that.
>>>
>>> The drawbacks of the PDB approach I see are:
>>>
>>> - I lose the ability to do matching of the (real) object file via symbol
>>> vendors. The PDB symbol files now basically implement their own little
>>> symbol vendors inside them, which is mostly fine if you just need to find
>>> the PDB next to the exe file. However, things could get a bit messy if you
>>> wanted to implement some more complex searching on multiple paths, or
>>> downloading them from the internet.
>>> - I'll hit issues when attempting to unwind (which is the real meat of
>>> the breakpad symbols), because unwind info is currently provided via the
>>> ObjectFile interface (ObjectFile::GetUnwindTable).
>>>
>>> The drawbacks of the ObjectFile approach are:
>>>
>>> - more code  - it needs a new ObjectFile and a new SymbolFile class
>>> (possibly also a 

Re: [Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Leonard Mosescu via lldb-commits]

Great observations Pavel! I think it's really important to have
orthogonal/composable abstractions here: the symbols should be decoupled
from the container format IMO.

You know more about the ObjectFile than me so I can't say if
ObjectFileBreakpad is the best interface, but here are my initial
observations (in a somewhat random order):

1. We need clear and separate abstractions for a container (ELF, PE file,
Breakpad symbols) vs. the content (debug Information).

2. We need to be able to consume symbols when the corresponding module
binary is not available. This is common for postmortem debugging (ex.
having a minidump + PDBs, but not all the .DLLs or EXE files).

3. I'm not a fan of the PDB model, where the symbols are searched in both
the symtabs then in the symbol files. I'd rather like to see the symtab an
interface for symbols regardless of where they come from.
   (Zach expressed some efficiency concerns if we'd need to build a symtab
from a PDB for example as opposed to accessing the PDB symfile directly,
although I think we can design to address this - ie. multiple concrete
symtab implementations, some of which are *internally* aware of the source
container, but w/o leaking this through the interface)

4. The symbol vendor observation is very important. Right now LLDB has
basic support for looking up DWARF symbols and none for PDBs. (for example,
IMO LLDB could greatly benefit from something like Microsoft's symsrv - I'm
actually planning to look into it soon)
  (Whatever we do, this should be one of the key requirements IMO)

I have a local change to address #2 specifically for PDBs (I'll try to send
out a code review soon).

On Thu, Nov 29, 2018 at 10:55 AM Pavel Labath via Phabricator via
lldb-commits  wrote:

> labath added a comment.
>
> I've recently started looking at adding a new symbol file format (breakpad
> symbols). While researching the best way to achieve that, I started
> comparing the operation of PDB and DWARF symbol files. I noticed a very
> important difference there, and I think that is the cause of our problems
> here. In the DWARF implementation, a symbol file is an overlay on top of an
> object file - it takes the data contained by the object file and presents
> it in a more structured way.
>
> However, that is not the case with PDB (both implementations). These take
> the debug information from a completely different file, which is not backed
> by an ObjectFile instance, and then present that. Since the SymbolFile
> interface requires them to be backed by an object file, they both pretend
> they are backed by the original EXE file, but in reality the data comes
> from elsewhere.
>
> If we had an ObjectFilePDB (which not also not ideal, though in a way it
> is a better fit to the current lldb organization), then this could expose
> the PDB symtab via the existing ObjectFile interface and we could reuse the
> existing mechanism for merging symtabs from two object files.
>
> I am asking this because now I am facing a choice in how to implement
> breakpad symbols. I could go the PDB way, and read the symbols without an
> intervening object file, or I could create an ObjectFileBreakpad and then
> (possibly) a SymbolFileBreakpad sitting on top of that.
>
> The drawbacks of the PDB approach I see are:
>
> - I lose the ability to do matching of the (real) object file via symbol
> vendors. The PDB symbol files now basically implement their own little
> symbol vendors inside them, which is mostly fine if you just need to find
> the PDB next to the exe file. However, things could get a bit messy if you
> wanted to implement some more complex searching on multiple paths, or
> downloading them from the internet.
> - I'll hit issues when attempting to unwind (which is the real meat of the
> breakpad symbols), because unwind info is currently provided via the
> ObjectFile interface (ObjectFile::GetUnwindTable).
>
> The drawbacks of the ObjectFile approach are:
>
> - more code  - it needs a new ObjectFile and a new SymbolFile class
> (possibly also a SymbolVendor)
> - it will probably look a bit weird because Breakpad files (and PDBs)
> aren't really object files
>
> I'd like to hear your thoughts on this, if you have any.
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D53368/new/
>
> https://reviews.llvm.org/D53368
>
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r350546 - Use the minidump exception record if present

[Leonard Mosescu via lldb-commits]

Author: lemo
Date: Mon Jan  7 09:55:42 2019
New Revision: 350546

URL: http://llvm.org/viewvc/llvm-project?rev=350546=rev
Log:
Use the minidump exception record if present

If the minidump contains a saved exception record use it automatically.

Differential Revision: https://reviews.llvm.org/D56293


Added:
lldb/trunk/lit/Minidump/Windows/
lldb/trunk/lit/Minidump/Windows/Sigsegv/
lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/
lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp   (with props)
lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb   (with props)
lldb/trunk/lit/Minidump/Windows/Sigsegv/sigsegv.test
Modified:
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp

Added: lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp?rev=350546=auto
==
--- lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp (added)
+++ lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp Mon Jan  7 
09:55:42 2019
@@ -0,0 +1,40 @@
+
+// nodefaultlib build: cl -Zi sigsegv.cpp /link /nodefaultlib
+
+#ifdef USE_CRT
+#include 
+#else
+int main();
+extern "C"
+{
+int _fltused;
+void mainCRTStartup() { main(); }
+void printf(const char*, ...) {}
+}
+#endif
+
+void crash(bool crash_self)
+{
+printf("Before...\n");
+if(crash_self)
+{
+printf("Crashing in 3, 2, 1 ...\n");
+*(volatile int*)nullptr = 0;
+}
+printf("After...\n");
+}
+
+int foo(int x, float y, const char* msg)
+{
+bool flag = x > y;
+if(flag)
+printf("x = %d, y = %f, msg = %s\n", x, y, msg);
+crash(flag);
+return x << 1;
+}
+
+int main()
+{
+foo(10, 3.14, "testing");
+}
+

Added: lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp?rev=350546=auto
==
Binary file - no diff available.

Propchange: lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
--
svn:mime-type = application/octet-stream

Added: lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit?rev=350546=auto
==
--- lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit (added)
+++ lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit Mon Jan  7 
09:55:42 2019
@@ -0,0 +1,2 @@
+bt all
+dis

Added: lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb?rev=350546=auto
==
Binary file - no diff available.

Propchange: lldb/trunk/lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
--
svn:mime-type = application/octet-stream

Added: lldb/trunk/lit/Minidump/Windows/Sigsegv/sigsegv.test
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Minidump/Windows/Sigsegv/sigsegv.test?rev=350546=auto
==
--- lldb/trunk/lit/Minidump/Windows/Sigsegv/sigsegv.test (added)
+++ lldb/trunk/lit/Minidump/Windows/Sigsegv/sigsegv.test Mon Jan  7 09:55:42 
2019
@@ -0,0 +1,13 @@
+// RUN: cd %p/Inputs
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 \
+// RUN:   %lldb -c sigsegv.dmp -s sigsegv.lldbinit | FileCheck %s
+
+CHECK: * thread #1, stop reason = Exception 0xc005 encountered at address 
0x7ff7a13110d9
+CHECK:   * frame #0: 0x7ff7a13110d9 sigsegv.exe
+
+CHECK: ->  0x7ff7a13110d9: movl   $0x0, 0x0
+CHECK: 0x7ff7a13110e4: leaq   0x1f45(%rip), %rcx
+CHECK: 0x7ff7a13110eb: callq  0x7ff7a1311019
+CHECK: 0x7ff7a13110f0: addq   $0x28, %rsp
+CHECK: 0x7ff7a13110f4: retq
+CHECK: 0x7ff7a13110f5: int3

Modified: lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp?rev=350546=350545=350546=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp (original)
+++ lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp Mon Jan  7 
09:55:42 2019
@@ 

Re: [Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

[Leonard Mosescu via lldb-commits]

>
> ouldn’t we have lldb generate the mini dump itself as the first step of
> the test?
>

How would this work cross-platform?

On Fri, Jan 4, 2019 at 8:48 AM Zachary Turner  wrote:

> For those kinds of cases, we could use obj2yaml and check in yaml right?
> Fwiw I tried to round-trip an exe through obj->yaml->obj recently and the
> resulting exe was incorrect but it was close, so I think there’s only some
> small fixes needed.
>
> In regards to your previous response, couldn’t we have lldb generate the
> mini dump itself as the first step of the test?
>
> On Thu, Jan 3, 2019 at 11:59 PM Pavel Labath via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> labath added a comment.
>>
>> In D56293#1345790 , @zturner
>> wrote:
>>
>> > I don't think we can check in an executable file, we should try to
>> compile it on the spot.  We have 1-2 existing unit tests that check in an
>> exe and we occasionally get reports that peoples' virus scanners flag them
>> as trojans, even though they obviously aren't.  In any case, I've been
>> meaning to remove those tests, so I think we should set a precedent that
>> executable binaries are never checked in.
>>
>>
>> While I agree that a checked-in exe shouldn't be needed in this (and most
>> other) cases, I am not sure about the policy in general. For example, I can
>> see a case for having a bunch of badly corrupted binaries (things like
>> corrupted section headers, overlapping sections in the file; things that
>> even yaml2obj will have trouble generating) and then a test that makes sure
>> we do something reasonable (e.g., not crash) when opening them. These are
>> exactly the kind of files that make paranoid virus scanners sound the alarm.
>>
>>
>> Repository:
>>   rLLDB LLDB
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D56293/new/
>>
>> https://reviews.llvm.org/D56293
>>
>>
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

[Leonard Mosescu via lldb-commits]

>
> I have a minidump generator if you need me to make any specific minidump
> files for you.
>

Maybe not in this case, but it seems an interesting idea. What are the
capabilities of this generator tool?

On Thu, Jan 3, 2019 at 3:49 PM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> I have a minidump generator if you need me to make any specific minidump
> files for you.
>
>
> Repository:
>   rLLDB LLDB
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D56293/new/
>
> https://reviews.llvm.org/D56293
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

[Leonard Mosescu via lldb-commits]

Great! I can see how we can put this to good use.

In the meantime, I'd like to submit this change as is - the included input
files are intended to be reused for future test cases as well (they are
extracted from my larger change to add support for the native PDB reader +
minidumps).

On Fri, Jan 4, 2019 at 12:39 PM Greg Clayton  wrote:

>
>
> On Jan 4, 2019, at 12:37 PM, Leonard Mosescu  wrote:
>
> Sounds very useful. Are you planning to add it to the LLDB repository?
>
>
> Yes
>
>
>
> On Fri, Jan 4, 2019 at 10:56 AM Greg Clayton  wrote:
>
>>
>>
>> On Jan 4, 2019, at 9:45 AM, Leonard Mosescu  wrote:
>>
>> I have a minidump generator if you need me to make any specific minidump
>>> files for you.
>>>
>>
>> Maybe not in this case, but it seems an interesting idea. What are the
>> capabilities of this generator tool?
>>
>>
>> I can generate threads contexts, any of the textual directory streams,
>> memory regions (32 and 64), module lists, and more.
>>
>> Example code from my python "minidump" module that shows generation of a
>> minidump. Not all of this goes together (example code for ARM and ARM64),
>> but it shows what you can easily do:
>>
>> system_info = minidump.SystemInfo(
>> ProcessorArchitecture=minidump.PROCESSOR_ARCHITECTURE_ARM64,
>> PlatformId=minidump.VER_PLATFORM_LINUX,
>> CSDVersion=minidump.String('15E216'))
>>
>> md = minidump.Generator(system_info, ProcessId=123)
>>
>> x = []
>> v = []
>> for i in range(32):
>> x.append(i+1 | i+2 << 32 | i+3 << 48)
>> for i in range(32):
>> for j in range(16):
>> v.append(i+j)
>>
>> thread = minidump.Thread(ThreadId=0x1000,
>> Registers=minidump.ThreadContext_ARM64(
>> x=x, pc=0x1000, cpsr=0x11223344,
>> fpsr=0x55667788, fpcr=0x99AABBCC, v=v))
>>
>> system_info = minidump.SystemInfo(
>> ProcessorArchitecture=minidump.PROCESSOR_ARCHITECTURE_ARM,
>> PlatformId=minidump.VER_PLATFORM_MACOSX,
>> CSDVersion=minidump.String('ABC123'))
>>
>> md = minidump.Generator(system_info, ProcessId=123)
>>
>> r = []
>> d = []
>> extra = []
>> for i in range(1, 17):
>> r.append(i)
>> for i in range(1, 33):
>> d.append(i | i << 8 | i << 32 | i << 48)
>> for i in range(8):
>> extra.append(i | i << 16)
>>
>> thread = minidump.Thread(ThreadId=0x1000,
>> Registers=minidump.ThreadContext_ARM(
>> r=r, cpsr=0x11223344,
>> fpscr=0x55667788AABBCCDD, d=d,
>> extra=extra))
>> md.add_thread(thread)
>> md.add_thread(minidump.Thread(ThreadId=0x55667788))
>>
>> md.add_module(minidump.Module(BaseOfImage=0x1,
>> SizeOfImage=0x2000,
>> CheckSum=0,
>> TimeDateStamp=0,
>> ModuleName="/tmp/b",
>> VersionInfo=None,
>> CvRecord=None,
>> MiscRecord=None,
>> Reserved0=0,
>> Reserved1=0))
>>
>> md.add_module(minidump.Module(BaseOfImage=0x2000,
>> SizeOfImage=0x1000,
>> CheckSum=0,
>> TimeDateStamp=0,
>> ModuleName="/tmp/a",
>> VersionInfo=None,
>> CvRecord=None,
>> MiscRecord=None,
>> Reserved0=0,
>> Reserved1=0))
>>
>> md.add_module(minidump.Module(BaseOfImage=0x1000,
>> SizeOfImage=0x1000,
>> CheckSum=0,
>> TimeDateStamp=0,
>> ModuleName="/tmp/b",
>> VersionInfo=None,
>> CvRecord=None,
>> MiscRecord=None,
>> Reserved0=0,
>> Reserved1=0))
>>
>>
>> md.add_module(minidump.Module(BaseOfImage=0x5000,
>> SizeOfImage=0x3000,
>> CheckSum=0,
>> TimeDateStamp=0,
>> ModuleName="/tmp/b",
>> VersionInfo=None,
>> CvRecord=None,
>> MiscRecord=None,
>> Reserved0=0,
>> Reserved1=0))
>> md.add_memory(minidump.MemoryDescriptor(StartOfMemoryRange=0x8000,
>> Bytes='Hello world!'))
>> md.add_memory(minidump.MemoryDescriptor(StartOfMemoryRange=0x8010,
>>

Re: [Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

[Leonard Mosescu via lldb-commits]

Sounds very useful. Are you planning to add it to the LLDB repository?


On Fri, Jan 4, 2019 at 10:56 AM Greg Clayton  wrote:

>
>
> On Jan 4, 2019, at 9:45 AM, Leonard Mosescu  wrote:
>
> I have a minidump generator if you need me to make any specific minidump
>> files for you.
>>
>
> Maybe not in this case, but it seems an interesting idea. What are the
> capabilities of this generator tool?
>
>
> I can generate threads contexts, any of the textual directory streams,
> memory regions (32 and 64), module lists, and more.
>
> Example code from my python "minidump" module that shows generation of a
> minidump. Not all of this goes together (example code for ARM and ARM64),
> but it shows what you can easily do:
>
> system_info = minidump.SystemInfo(
> ProcessorArchitecture=minidump.PROCESSOR_ARCHITECTURE_ARM64,
> PlatformId=minidump.VER_PLATFORM_LINUX,
> CSDVersion=minidump.String('15E216'))
>
> md = minidump.Generator(system_info, ProcessId=123)
>
> x = []
> v = []
> for i in range(32):
> x.append(i+1 | i+2 << 32 | i+3 << 48)
> for i in range(32):
> for j in range(16):
> v.append(i+j)
>
> thread = minidump.Thread(ThreadId=0x1000,
> Registers=minidump.ThreadContext_ARM64(
> x=x, pc=0x1000, cpsr=0x11223344,
> fpsr=0x55667788, fpcr=0x99AABBCC, v=v))
>
> system_info = minidump.SystemInfo(
> ProcessorArchitecture=minidump.PROCESSOR_ARCHITECTURE_ARM,
> PlatformId=minidump.VER_PLATFORM_MACOSX,
> CSDVersion=minidump.String('ABC123'))
>
> md = minidump.Generator(system_info, ProcessId=123)
>
> r = []
> d = []
> extra = []
> for i in range(1, 17):
> r.append(i)
> for i in range(1, 33):
> d.append(i | i << 8 | i << 32 | i << 48)
> for i in range(8):
> extra.append(i | i << 16)
>
> thread = minidump.Thread(ThreadId=0x1000,
> Registers=minidump.ThreadContext_ARM(
> r=r, cpsr=0x11223344,
> fpscr=0x55667788AABBCCDD, d=d,
> extra=extra))
> md.add_thread(thread)
> md.add_thread(minidump.Thread(ThreadId=0x55667788))
>
> md.add_module(minidump.Module(BaseOfImage=0x1,
> SizeOfImage=0x2000,
> CheckSum=0,
> TimeDateStamp=0,
> ModuleName="/tmp/b",
> VersionInfo=None,
> CvRecord=None,
> MiscRecord=None,
> Reserved0=0,
> Reserved1=0))
>
> md.add_module(minidump.Module(BaseOfImage=0x2000,
> SizeOfImage=0x1000,
> CheckSum=0,
> TimeDateStamp=0,
> ModuleName="/tmp/a",
> VersionInfo=None,
> CvRecord=None,
> MiscRecord=None,
> Reserved0=0,
> Reserved1=0))
>
> md.add_module(minidump.Module(BaseOfImage=0x1000,
> SizeOfImage=0x1000,
> CheckSum=0,
> TimeDateStamp=0,
> ModuleName="/tmp/b",
> VersionInfo=None,
> CvRecord=None,
> MiscRecord=None,
> Reserved0=0,
> Reserved1=0))
>
>
> md.add_module(minidump.Module(BaseOfImage=0x5000,
> SizeOfImage=0x3000,
> CheckSum=0,
> TimeDateStamp=0,
> ModuleName="/tmp/b",
> VersionInfo=None,
> CvRecord=None,
> MiscRecord=None,
> Reserved0=0,
> Reserved1=0))
> md.add_memory(minidump.MemoryDescriptor(StartOfMemoryRange=0x8000,
> Bytes='Hello world!'))
> md.add_memory(minidump.MemoryDescriptor(StartOfMemoryRange=0x8010,
> Bytes='Goodbye moon...'))
> md.add_memory64(minidump.MemoryDescriptor64(StartOfMemoryRange=0x1000,
> Bytes='1' * 16))
> md.add_memory64(minidump.MemoryDescriptor64(StartOfMemoryRange=0x2000,
> Bytes='3' * 32))
>
> md.add_memory(minidump.MemoryDescriptor(StartOfMemoryRange=0x1000,
> Bytes='1' * 16))
> md.add_memory(minidump.MemoryDescriptor(StartOfMemoryRange=0x2000,
>