[Lldb-commits] [lldb] r304379 - Forgot to mention rewriting CommandObject::DoExecute

[Jim Ingham via lldb-commits]

Author: jingham
Date: Wed May 31 20:05:30 2017
New Revision: 304379

URL: http://llvm.org/viewvc/llvm-project?rev=304379=rev
Log:
Forgot to mention rewriting CommandObject::DoExecute 
using the SB API's not the lldb_private API's.

Modified:
lldb/trunk/www/projects.html

Modified: lldb/trunk/www/projects.html
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/www/projects.html?rev=304379=304378=304379=diff
==
--- lldb/trunk/www/projects.html (original)
+++ lldb/trunk/www/projects.html Wed May 31 20:05:30 2017
@@ -244,6 +244,25 @@
 
 
 
+  Reimplement the command 
interpreter commands using the SB API
+  
+Currently, all the 
CommandObject::DoExecute methods are implemented 
+using the lldb_private API's.  
That generally means that there's code
+that gets duplicated between 
the CommandObject and the SB API that does
+roughly the same thing.  We 
would reduce this code duplication, present a
+single coherent face to the 
users of lldb, and keep
+ourselves more honest about 
what we need in the SB API's if we implemented
+the CommandObjects::DoExecute 
methods using the SB API's.
+  
+  
+BTW, it is only the way it was 
much easier to develop lldb if it had a functioning
+command-line early on.  So we 
did that first, and developed the SB API's when lldb
+was more mature.  There's no 
good technical reason to have the commands use the
+lldb_private API's.
+  
+
+
+
   Documentation and better examples
 
   


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

Re: [Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Jim Ingham via lldb-commits]

> On May 31, 2017, at 2:02 PM, Zachary Turner  wrote:
> 
> This hypothetical DSL could still run SB API statements.  I don't necessarily 
> think it *should*, since what the SB API does and what the commands do are 
> very different (for example SBTarget.Create() does something entirely 
> different than "target create", so you're not actually testing what the 
> debugger does when you create a target, you're only testing what this API 
> does that is not really used outside of IDE integration and scripting 
> scenarios.  But it could.  

I don't have hard data, but I am pretty sure the majority of users of lldb use 
it through Xcode.  We command line users have moral superiority, but I don't 
think we have numerical superiority.  So in my mind, testing the SB API's 
throughly is at least as, if not more, important than testing lldb command line 
behavior.

If you are worried about splitting the testing between CLI lldb & the SB API's, 
the obvious solution is to rewrite all the lldb commands to use the SB API's. 
That is independently a useful project in lldb.  It would clean up its surface 
area and make the higher layers coherent.  It isn't that way now purely for 
historical reasons - we needed something we could start to poke at long before 
we were ready to design the outward-facing API's.  But it leads to code 
duplication and introduces an unnecessary bifurcation between "how the debugger 
works" from a command line user's perspective and "how the debugger works" as 
it presents itself to most of the users of lldb which is not good long term.

> 
> I mean in theory you could have a test that looks like:
> 
> # input_file: foo.cpp
> # int main(int argc, char **argv) {
> #return 0;
> # }
> bp1 = set_breakpoint(foo.cpp:2)
> bp2 = set_breakpoint(foo.cpp@_main)
> run
> expect(bp1.hits == 1)
> expect(bp2.hits == 1)  
> 
> A test consisting of 1 file and 9 lines is a lot easier to understand to me 
> than a test consisting of a python file, a Makefile, and a source file, where 
> the python file is a minimum of 20 lines with complicated setup and 
> initialization steps that are mostly boilerplate.
> 

That's pretty much the inline tests except for the inline tests you have to do 
obvious modifications on a 10 line Python file and for your example I have to 
learn an API set "e.g. set_breakpoint" that will do me no good in any other 
area of my work.


> Note that I'm not necessarily even advocating for something that looks like 
> the above, it just is the first thing that came to mind.
> 
> As a first pass, you could literally just have lit be the thing that walks 
> the directory tree scanning for test files, then invoking Make and running 
> the existing .py files.  That would already be a big improvement.  1000 lines 
> of python code that many people understand and actively maintain / improve is 
> better than 2000 lines of python code that nobody understands and nobody 
> really maintains / improves.

When we've talked about using lit before the argument always went "Lit can do 
anything, it can certainly do this."  Then after a bit "well it doesn't do 
exactly this but we can make it do this..."  So the "this" would be a bunch of 
new lines of new python code that do things most of its current users haven't 
needed to do - on top of the 1000 that people understand.  I'm unconvinced that 
that new code would be trivial or trouble-free, or that the effort to implement 
and make it trouble-free it would not be more productively spent fixing up the 
extant suite.

But that's just my guess.  The free effort of other developers is indeed 
free...  

As a practical concern, however - since we're relying on the lldb testsuite for 
PR's on github, and of course internally since we have product to ship - I 
would really prefer that folks doing intrusive work on the testsuite either do 
it on a branch, or as a parallel mechanism, rather than trying to splice it 
into the extant suite.  I'd really rather not have to field test a new 
testsuite while needing to get stable & useful results out of it.

Jim


> 
> On Wed, May 31, 2017 at 1:45 PM Jim Ingham  wrote:
> 
> > On May 31, 2017, at 12:22 PM, Zachary Turner  wrote:
> >
> > Writing tests that are
> >
> > A) as easy as possible to write and understand
> 
> I've never understood why you consider the tests in our test case hard to 
> write or understand.  Till I added the example directory, you had to go 
> search around for an exemplar to work from, which was a bit of a pain.  But 
> now you copy whichever of the examples you want into place, and you have a 
> process stopped somewhere and ready to poke at.  You do need to learn some SB 
> API's but on that see below...
> 
> > B) built on a multiprocessing infrastructure that is battle tested and 
> > known to not be flaky
> 
> We haven't come across failures in the tests caused by the test 
> infrastructure in a while now.  The multiprocessing 

Re: [Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Zachary Turner via lldb-commits]

And once you start annotating source code with statements, things become
even more concise.  For example, you could write the above test:

# input_file: foo.cpp
# int main(int argc, char **argv)  {  // bp1 = BREAK_HERE
#   return 0;// bp2 = BREAK_HERE
# }
# // VERIFY(bp1.hits == 1)
# // VERIFY(bp2.hits == 1)

You can have cross references, conditionals, variables, etc.  The only real
limit is your imagination.  Anyone can mentally digest this entire test in
about 5 seconds without having to read through multiple files and groking
Python code and an a separate API.

On Wed, May 31, 2017 at 2:02 PM Zachary Turner  wrote:

> This hypothetical DSL could still run SB API statements.  I don't
> necessarily think it *should*, since what the SB API does and what the
> commands do are very different (for example SBTarget.Create() does
> something entirely different than "target create", so you're not actually
> testing what the debugger does when you create a target, you're only
> testing what this API does that is not really used outside of IDE
> integration and scripting scenarios.  But it could.
>
> I mean in theory you could have a test that looks like:
>
> # input_file: foo.cpp
> # int main(int argc, char **argv) {
> #return 0;
> # }
> bp1 = set_breakpoint(foo.cpp:2)
> bp2 = set_breakpoint(foo.cpp@_main)
> run
> expect(bp1.hits == 1)
> expect(bp2.hits == 1)
>
> A test consisting of 1 file and 9 lines is a lot easier to understand to
> me than a test consisting of a python file, a Makefile, and a source file,
> where the python file is a minimum of 20 lines with complicated setup and
> initialization steps that are mostly boilerplate.
>
> Note that I'm not necessarily even advocating for something that looks
> like the above, it just is the first thing that came to mind.
>
> As a first pass, you could literally just have lit be the thing that walks
> the directory tree scanning for test files, then invoking Make and running
> the existing .py files.  That would already be a big improvement.  1000
> lines of python code that many people understand and actively maintain /
> improve is better than 2000 lines of python code that nobody understands
> and nobody really maintains / improves.
>
> On Wed, May 31, 2017 at 1:45 PM Jim Ingham  wrote:
>
>>
>> > On May 31, 2017, at 12:22 PM, Zachary Turner 
>> wrote:
>> >
>> > Writing tests that are
>> >
>> > A) as easy as possible to write and understand
>>
>> I've never understood why you consider the tests in our test case hard to
>> write or understand.  Till I added the example directory, you had to go
>> search around for an exemplar to work from, which was a bit of a pain.  But
>> now you copy whichever of the examples you want into place, and you have a
>> process stopped somewhere and ready to poke at.  You do need to learn some
>> SB API's but on that see below...
>>
>> > B) built on a multiprocessing infrastructure that is battle tested and
>> known to not be flaky
>>
>> We haven't come across failures in the tests caused by the test
>> infrastructure in a while now.  The multiprocessing flakiness I've seen is
>> because you are trying to run many tests in parallel, and some of those
>> tests require complex setup and that times out when machines are heavily
>> loaded.  And for the most part we solve that by running the tests that
>> require timeouts serially.  That seems a solved problem as well.
>>
>> > C) Familiar to other llvm developers, so as to not discourage subject
>> matter experts from other areas to make relevant improvements to LLDB
>> >
>>
>> If I were a developer coming new to lldb now, and had to write a test
>> case, I would have to learn something about how the SB API's work (that and
>> a little Python.)  The test case part is pretty trivial, especially when
>> copying from the sample tests.  Learning the SB API's is a bit of a pain,
>> but having done that the next time I wanted to write some little breakpoint
>> command to do something clever when doing my own debugging, I now have some
>> skills at my fingertips that are going to come in really handy.
>>
>> If we change over the tests so what I did instead was learn some DSL to
>> describe lldb tests (which would probably not be entirely trivial, and
>> likely not much easier than the SB API's which are very straight-forward) I
>> have learned nothing useful outside of writing tests.  Not sure how that's
>> a benefit.
>>
>> For the tests that can be decoupled from running processes, that have
>> simple data in and out and don't require much other state - again the frame
>> unwinder tests are a perfect example - writing separable tests for that
>> seems great.  But in those cases you are generally poking API's and I don't
>> see how coming up with some DSL that is general enough to represent this is
>> any advantage over how those tests are currently written with gtest.
>>
>> >
>> > For example, I assume you are on board at 

Re: [Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Zachary Turner via lldb-commits]

This hypothetical DSL could still run SB API statements.  I don't
necessarily think it *should*, since what the SB API does and what the
commands do are very different (for example SBTarget.Create() does
something entirely different than "target create", so you're not actually
testing what the debugger does when you create a target, you're only
testing what this API does that is not really used outside of IDE
integration and scripting scenarios.  But it could.

I mean in theory you could have a test that looks like:

# input_file: foo.cpp
# int main(int argc, char **argv) {
#return 0;
# }
bp1 = set_breakpoint(foo.cpp:2)
bp2 = set_breakpoint(foo.cpp@_main)
run
expect(bp1.hits == 1)
expect(bp2.hits == 1)

A test consisting of 1 file and 9 lines is a lot easier to understand to me
than a test consisting of a python file, a Makefile, and a source file,
where the python file is a minimum of 20 lines with complicated setup and
initialization steps that are mostly boilerplate.

Note that I'm not necessarily even advocating for something that looks like
the above, it just is the first thing that came to mind.

As a first pass, you could literally just have lit be the thing that walks
the directory tree scanning for test files, then invoking Make and running
the existing .py files.  That would already be a big improvement.  1000
lines of python code that many people understand and actively maintain /
improve is better than 2000 lines of python code that nobody understands
and nobody really maintains / improves.

On Wed, May 31, 2017 at 1:45 PM Jim Ingham  wrote:

>
> > On May 31, 2017, at 12:22 PM, Zachary Turner  wrote:
> >
> > Writing tests that are
> >
> > A) as easy as possible to write and understand
>
> I've never understood why you consider the tests in our test case hard to
> write or understand.  Till I added the example directory, you had to go
> search around for an exemplar to work from, which was a bit of a pain.  But
> now you copy whichever of the examples you want into place, and you have a
> process stopped somewhere and ready to poke at.  You do need to learn some
> SB API's but on that see below...
>
> > B) built on a multiprocessing infrastructure that is battle tested and
> known to not be flaky
>
> We haven't come across failures in the tests caused by the test
> infrastructure in a while now.  The multiprocessing flakiness I've seen is
> because you are trying to run many tests in parallel, and some of those
> tests require complex setup and that times out when machines are heavily
> loaded.  And for the most part we solve that by running the tests that
> require timeouts serially.  That seems a solved problem as well.
>
> > C) Familiar to other llvm developers, so as to not discourage subject
> matter experts from other areas to make relevant improvements to LLDB
> >
>
> If I were a developer coming new to lldb now, and had to write a test
> case, I would have to learn something about how the SB API's work (that and
> a little Python.)  The test case part is pretty trivial, especially when
> copying from the sample tests.  Learning the SB API's is a bit of a pain,
> but having done that the next time I wanted to write some little breakpoint
> command to do something clever when doing my own debugging, I now have some
> skills at my fingertips that are going to come in really handy.
>
> If we change over the tests so what I did instead was learn some DSL to
> describe lldb tests (which would probably not be entirely trivial, and
> likely not much easier than the SB API's which are very straight-forward) I
> have learned nothing useful outside of writing tests.  Not sure how that's
> a benefit.
>
> For the tests that can be decoupled from running processes, that have
> simple data in and out and don't require much other state - again the frame
> unwinder tests are a perfect example - writing separable tests for that
> seems great.  But in those cases you are generally poking API's and I don't
> see how coming up with some DSL that is general enough to represent this is
> any advantage over how those tests are currently written with gtest.
>
> >
> > For example, I assume you are on board at least to some extent with
> lldbinline tests.
>
> The part of the inline tests that allow you to express the tests you want
> to run next to the code you stop in is fine till it doesn't work - at which
> point debugging them becomes a real PITN.  But they still use the common
> currencies of the command line or SB API's to actually perform the tests.
> I'd be less interested if this was some special purpose language of our own
> devising.  I can't at all see wanting to learn that if the only use it
> would be to me is for writing tests.
>
> > After all, they're simpler than the other style of test. Now, suppose
> there were some hypothetical DSL that allowed every test to be an inline
> test but still test equally complicated scenarios in half the code. Then
> 

Re: [Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Jim Ingham via lldb-commits]

> On May 31, 2017, at 12:22 PM, Zachary Turner  wrote:
> 
> Writing tests that are
> 
> A) as easy as possible to write and understand

I've never understood why you consider the tests in our test case hard to write 
or understand.  Till I added the example directory, you had to go search around 
for an exemplar to work from, which was a bit of a pain.  But now you copy 
whichever of the examples you want into place, and you have a process stopped 
somewhere and ready to poke at.  You do need to learn some SB API's but on that 
see below...

> B) built on a multiprocessing infrastructure that is battle tested and known 
> to not be flaky

We haven't come across failures in the tests caused by the test infrastructure 
in a while now.  The multiprocessing flakiness I've seen is because you are 
trying to run many tests in parallel, and some of those tests require complex 
setup and that times out when machines are heavily loaded.  And for the most 
part we solve that by running the tests that require timeouts serially.  That 
seems a solved problem as well.

> C) Familiar to other llvm developers, so as to not discourage subject matter 
> experts from other areas to make relevant improvements to LLDB
> 

If I were a developer coming new to lldb now, and had to write a test case, I 
would have to learn something about how the SB API's work (that and a little 
Python.)  The test case part is pretty trivial, especially when copying from 
the sample tests.  Learning the SB API's is a bit of a pain, but having done 
that the next time I wanted to write some little breakpoint command to do 
something clever when doing my own debugging, I now have some skills at my 
fingertips that are going to come in really handy.

If we change over the tests so what I did instead was learn some DSL to 
describe lldb tests (which would probably not be entirely trivial, and likely 
not much easier than the SB API's which are very straight-forward) I have 
learned nothing useful outside of writing tests.  Not sure how that's a benefit.

For the tests that can be decoupled from running processes, that have simple 
data in and out and don't require much other state - again the frame unwinder 
tests are a perfect example - writing separable tests for that seems great.  
But in those cases you are generally poking API's and I don't see how coming up 
with some DSL that is general enough to represent this is any advantage over 
how those tests are currently written with gtest.

> 
> For example, I assume you are on board at least to some extent with 
> lldbinline tests. 

The part of the inline tests that allow you to express the tests you want to 
run next to the code you stop in is fine till it doesn't work - at which point 
debugging them becomes a real PITN.  But they still use the common currencies 
of the command line or SB API's to actually perform the tests.  I'd be less 
interested if this was some special purpose language of our own devising.  I 
can't at all see wanting to learn that if the only use it would be to me is for 
writing tests.

> After all, they're simpler than the other style of test. Now, suppose there 
> were some hypothetical DSL that allowed every test to be an inline test but 
> still test equally complicated scenarios in half the code. Then suppose it 
> also ran on a more robust multiprocessing infrastructure than dotest.py. 
> That's all we're really talking about

Thanks for the clarification.

Jim



> On Wed, May 31, 2017 at 12:06 PM Jim Ingham via lldb-commits 
>  wrote:
> Before we get past "it's hard" to "just to do it", it would help me to be 
> clear first on what you are actually trying to achieve with this proposal.  
> It's not clear to me what problem are people trying to solve here?  If it is 
> writing tests for the decomposable parts of lldb - like the tests Jason wrote 
> for the unwinder recently - why was the gtest path not a good way to do this? 
>  If it is rewriting the parts of the testsuite that exercise the debugger on 
> live targets what would a lit-based suite do that we can't do with the 
> current testsuite?  Or maybe you are thinking of some other good I'm missing?
> 
> Jim
> 
> 
> > On May 31, 2017, at 10:37 AM, Zachary Turner via Phabricator via 
> > lldb-commits  wrote:
> >
> > zturner added a comment.
> >
> > In https://reviews.llvm.org/D32930#767820, @beanz wrote:
> >
> >> One small comment below. In general I agree with the thoughts here, and I 
> >> think that this is a huge step forward for testing the debug server 
> >> components.
> >>
> >> I also agree with Zachary in principal that it would be nice to come up 
> >> with lit-based test harnesses for more parts of LLDB, although I'm 
> >> skeptical about whether or not that is actually the best way to test the 
> >> debug server pieces. Either way, this is a huge step forward from what we 
> >> have today, so we should go with it.
> >
> 

Re: [Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Zachary Turner via lldb-commits]

Writing tests that are

A) as easy as possible to write and understand
B) built on a multiprocessing infrastructure that is battle tested and
known to not be flaky
C) Familiar to other llvm developers, so as to not discourage subject
matter experts from other areas to make relevant improvements to LLDB


For example, I assume you are on board at least to some extent with
lldbinline tests. After all, they're simpler than the other style of test.
Now, suppose there were some hypothetical DSL that allowed every test to be
an inline test but still test equally complicated scenarios in half the
code. Then suppose it also ran on a more robust multiprocessing
infrastructure than dotest.py. That's all we're really talking about
On Wed, May 31, 2017 at 12:06 PM Jim Ingham via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> Before we get past "it's hard" to "just to do it", it would help me to be
> clear first on what you are actually trying to achieve with this proposal.
> It's not clear to me what problem are people trying to solve here?  If it
> is writing tests for the decomposable parts of lldb - like the tests Jason
> wrote for the unwinder recently - why was the gtest path not a good way to
> do this?  If it is rewriting the parts of the testsuite that exercise the
> debugger on live targets what would a lit-based suite do that we can't do
> with the current testsuite?  Or maybe you are thinking of some other good
> I'm missing?
>
> Jim
>
>
> > On May 31, 2017, at 10:37 AM, Zachary Turner via Phabricator via
> lldb-commits  wrote:
> >
> > zturner added a comment.
> >
> > In https://reviews.llvm.org/D32930#767820, @beanz wrote:
> >
> >> One small comment below. In general I agree with the thoughts here, and
> I think that this is a huge step forward for testing the debug server
> components.
> >>
> >> I also agree with Zachary in principal that it would be nice to come up
> with lit-based test harnesses for more parts of LLDB, although I'm
> skeptical about whether or not that is actually the best way to test the
> debug server pieces. Either way, this is a huge step forward from what we
> have today, so we should go with it.
> >
> >
> > It would be nice if, at some point, we could move past "It's hard" and
> start getting into the details of what's hard about it.  (Note this goes
> for LLDB client as well as lldb server).  I see a lot of general hand-wavy
> comments about how conditionals are needed, or variables, etc, but that
> doesn't really do anything to convince me that it's hard.  After all, we
> wrote a C++ compiler!  And I'm pretty sure that the compiler-rt and
> sanitizer test suite is just as complicated as, if not more complicated
> than any hypothetical lldb test suite.  And those have been solved.
> >
> > What *would* help would be to ignore how difficult it may or may not be,
> and just take a couple of tests and re-write them in some DSL that you
> invent specifically for this purpose that is as concise as possible yet as
> expressive as you need, and we go from there.  I did this with a couple of
> fairly hairy tests a few months ago and it didn't seem that bad to me.
> >
> > The thing is, the set of people who are experts on the client side of
> LLDB and the set of people who are experts on the client side of
> LLVM/lit/etc are mostly disjoint, so nothing is ever going to happen
> without some sort of collaboration.  For example, I'm more than willing to
> help out writing the lit bits of it, but I would need a specification of
> what the test language needs to look like to support all of the use cases.
> And someone else has to provide that since we want to get the design
> right.  Even if implementing the language is hard, deciding what it needs
> to look like is supposed to be the easy part!
> >
> >
> > https://reviews.llvm.org/D32930
> >
> >
> >
> > ___
> > 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] D32930: New framework for lldb client-server communication tests.

[Jim Ingham via lldb-commits]

Before we get past "it's hard" to "just to do it", it would help me to be clear 
first on what you are actually trying to achieve with this proposal.  It's not 
clear to me what problem are people trying to solve here?  If it is writing 
tests for the decomposable parts of lldb - like the tests Jason wrote for the 
unwinder recently - why was the gtest path not a good way to do this?  If it is 
rewriting the parts of the testsuite that exercise the debugger on live targets 
what would a lit-based suite do that we can't do with the current testsuite?  
Or maybe you are thinking of some other good I'm missing?

Jim


> On May 31, 2017, at 10:37 AM, Zachary Turner via Phabricator via lldb-commits 
>  wrote:
> 
> zturner added a comment.
> 
> In https://reviews.llvm.org/D32930#767820, @beanz wrote:
> 
>> One small comment below. In general I agree with the thoughts here, and I 
>> think that this is a huge step forward for testing the debug server 
>> components.
>> 
>> I also agree with Zachary in principal that it would be nice to come up with 
>> lit-based test harnesses for more parts of LLDB, although I'm skeptical 
>> about whether or not that is actually the best way to test the debug server 
>> pieces. Either way, this is a huge step forward from what we have today, so 
>> we should go with it.
> 
> 
> It would be nice if, at some point, we could move past "It's hard" and start 
> getting into the details of what's hard about it.  (Note this goes for LLDB 
> client as well as lldb server).  I see a lot of general hand-wavy comments 
> about how conditionals are needed, or variables, etc, but that doesn't really 
> do anything to convince me that it's hard.  After all, we wrote a C++ 
> compiler!  And I'm pretty sure that the compiler-rt and sanitizer test suite 
> is just as complicated as, if not more complicated than any hypothetical lldb 
> test suite.  And those have been solved.
> 
> What *would* help would be to ignore how difficult it may or may not be, and 
> just take a couple of tests and re-write them in some DSL that you invent 
> specifically for this purpose that is as concise as possible yet as 
> expressive as you need, and we go from there.  I did this with a couple of 
> fairly hairy tests a few months ago and it didn't seem that bad to me.
> 
> The thing is, the set of people who are experts on the client side of LLDB 
> and the set of people who are experts on the client side of LLVM/lit/etc are 
> mostly disjoint, so nothing is ever going to happen without some sort of 
> collaboration.  For example, I'm more than willing to help out writing the 
> lit bits of it, but I would need a specification of what the test language 
> needs to look like to support all of the use cases.  And someone else has to 
> provide that since we want to get the design right.  Even if implementing the 
> language is hard, deciding what it needs to look like is supposed to be the 
> easy part!
> 
> 
> https://reviews.llvm.org/D32930
> 
> 
> 
> ___
> 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] D33426: Introduce new command: thread backtrace unique

[Jim Ingham via lldb-commits]

SBThread::Resume instructs lldb to set the resume state on a thread to 
"eStateRunning", meaning that means the next time you continue the process, 
that thread will get a chance to run.  It has no effect on the StopReason for 
the thread (it doesn't even start it running except maybe in the not well 
supported "thread-centric" debugging mode.)  

In the normal case, nothing is going to happen till the process is resumed 
(with SBProcess::Continue).  At that point, the thread you just allowed to run 
might or might not actually get scheduled to run.  And if it does actually run, 
it might or might not have stopped for an interesting reason by the time the 
process stopped.  So getting eStopReasonNone in this case is not at all 
unexpected.  If NONE of the threads in the program had an interesting stop 
reason, that would be weird, and worth looking at.  But that you might have to 
wait around for a while for any particular thread to stop for some reason is 
not unexpected.

Jim



> On May 31, 2017, at 3:44 AM, Pavel Labath via Phabricator 
>  wrote:
> 
> labath added a comment.
> 
> In https://reviews.llvm.org/D33426#766525, @bgianfo wrote:
> 
>> Address Pavel and Greg's feedback on Diff 100365.
>> 
>> Pavel: I took your suggestions to make the test case more readable,
>> I really appreciate the guidance. I did have to tweak some of the
>> functionality to make the test case pass reliably, as there were
>> still some races possible. I also saw that SBThread.Resume() seems
>> to occasionally result in a StopReason of eStopReasonNon. So I worked
>> around that by only including threads int expected output that the Resume
>> resulted in making it to our breakpoint. I have verified the test is 
>> consistently passes by executing it on repeat 100 times,
> 
> 
> Thanks. The fact that we are not able to rely on the operation of Resume in 
> this case sounds like a bug. Obviously we can't condition the acceptance of 
> this patch by fixing that issue, but we should at least track it. Can you 
> create a bug on bugs.llvm.org, and reference it in your workaround. BTW, 
> what's the platform you are testing this on?
> 
> 
> https://reviews.llvm.org/D33426
> 
> 
> 

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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:738-742
+StartProcessorTracing(tid, m_pt_process_config, traceMonitor);
+if (traceMonitor.get() != nullptr) {
+  traceMonitor->setUserID(m_pt_process_uid);
+  m_pt_traced_thread_group.insert(tid);
+}

This function returns a `Status`.  Can't we assume that `traceMonitor` will be 
valid if and only if the returned `Status` is a success condition?  And if it's 
not a success condition, shouldn't you log the error?



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2517-2522
+  auto iter = m_processor_trace_monitor.begin();
+  for (; iter != m_processor_trace_monitor.end(); iter++) {
+if (uid == iter->second->getUID() &&
+(thread == iter->first || thread == LLDB_INVALID_THREAD_ID))
+  return iter->second;
+  }

Please use a range based for here.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2626
+  lldb::user_id_t uid = LLDB_INVALID_UID;
+  if (config.getType() == lldb::TraceType::eTraceTypeProcessorTrace) {
+if (m_pt_process_uid == LLDB_INVALID_UID) {

Can you do an early return here if the condition is not true?



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2627
+  if (config.getType() == lldb::TraceType::eTraceTypeProcessorTrace) {
+if (m_pt_process_uid == LLDB_INVALID_UID) {
+  m_pt_process_config = config;

And here



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2638-2642
+llvm::StringRef intel_custom_params_key("intel-pt");
+llvm::StringRef cpu_family_key("cpu_family");
+llvm::StringRef cpu_model_key("cpu_model");
+llvm::StringRef cpu_stepping_key("cpu_stepping");
+llvm::StringRef cpu_vendor_intel_key("cpu_vendor_intel");

Nothing specifically wrong with this, but it's implicitly convertible, so if 
you like it you can simply just pass these strings into the `AddIntegerItem` 
function as string literals.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2717
+  m_processor_trace_monitor.insert(
+  std::pair(thread, traceMonitor));
+

`std::make_pair(thread, traceMonitor)` might allow this to fit on one line.  
`m_processor_trace_monitor.emplace(thread, traceMonitor);` almost definitely 
would.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2744
+  error = getCPUType(cpu_family, model, stepping, vendor_id);
+  if (error.Success()) {
+StructuredData::Dictionary *intel_params = new 
StructuredData::Dictionary();

Early return here.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2745
+  if (error.Success()) {
+StructuredData::Dictionary *intel_params = new 
StructuredData::Dictionary();
+

Who is responsible for freeing this memory?



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2794-2795
+
+  switch (trace_type) {
+  case lldb::TraceType::eTraceTypeProcessorTrace:
+error =

Seems like this would be more straightforward to just say:

```
if (trace_type != eTraceTypeProcessorTrace)
  return NativeProcessProtocol::StartTrace(config, error);
```



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2870-2871
+   lldb::tid_t thread) {
+  Status error;
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));
+

Bunch of opportunities in this function for early returning on error conditions.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2876-2877
+bool uid_found = false;
+for (; iter != m_processor_trace_monitor.end(); iter++) {
+  if (iter->second->getUID() == uid) {
+// Stopping a trace instance for an individual thread

Range based for with an inverted conditional and early continue inside the loop.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2931-2938
+static uint64_t ComputeFloorLog2(uint64_t input) {
+  uint64_t prev = input;
+  while ((input & (input - 1)) != 0) {
+input &= (input - 1);
+prev = input;
+  }
+  return prev;

Delete and replace callsites with `llvm::Log2_64`



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2940-2943
+Status NativeProcessLinux::ProcessorTraceMonitor::StartTrace(
+lldb::pid_t pid, lldb::tid_t tid, TraceOptions ) {
+  Status error;
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));

What happens if you call this function twice in a row?  Or from different 
threads?  Is that something you 

[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In https://reviews.llvm.org/D32930#767820, @beanz wrote:

> One small comment below. In general I agree with the thoughts here, and I 
> think that this is a huge step forward for testing the debug server 
> components.
>
> I also agree with Zachary in principal that it would be nice to come up with 
> lit-based test harnesses for more parts of LLDB, although I'm skeptical about 
> whether or not that is actually the best way to test the debug server pieces. 
> Either way, this is a huge step forward from what we have today, so we should 
> go with it.


It would be nice if, at some point, we could move past "It's hard" and start 
getting into the details of what's hard about it.  (Note this goes for LLDB 
client as well as lldb server).  I see a lot of general hand-wavy comments 
about how conditionals are needed, or variables, etc, but that doesn't really 
do anything to convince me that it's hard.  After all, we wrote a C++ compiler! 
 And I'm pretty sure that the compiler-rt and sanitizer test suite is just as 
complicated as, if not more complicated than any hypothetical lldb test suite.  
And those have been solved.

What *would* help would be to ignore how difficult it may or may not be, and 
just take a couple of tests and re-write them in some DSL that you invent 
specifically for this purpose that is as concise as possible yet as expressive 
as you need, and we go from there.  I did this with a couple of fairly hairy 
tests a few months ago and it didn't seem that bad to me.

The thing is, the set of people who are experts on the client side of LLDB and 
the set of people who are experts on the client side of LLVM/lit/etc are mostly 
disjoint, so nothing is ever going to happen without some sort of 
collaboration.  For example, I'm more than willing to help out writing the lit 
bits of it, but I would need a specification of what the test language needs to 
look like to support all of the use cases.  And someone else has to provide 
that since we want to get the design right.  Even if implementing the language 
is hard, deciding what it needs to look like is supposed to be the easy part!


https://reviews.llvm.org/D32930



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

[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Chris Bieneman via Phabricator via lldb-commits]

beanz added inline comments.



Comment at: unittests/tools/CMakeLists.txt:1
+if(UNIX AND NOT APPLE)
+  add_subdirectory(lldb-server)

labath wrote:
> beanz wrote:
> > labath wrote:
> > > This is not what I meant. The only targets (at least until we have 
> > > debugserver support) that can realistically pass these tests are linux, 
> > > android, and netbsd. The other targets (right now, I guess that would 
> > > mean freebsd) don't even pretend to support debugging via lldb-server, so 
> > > we should not fail their build because of that. Check for usages of 
> > > CMAKE_SYSTEM_NAME to see how to discriminate those.
> > Darwin pretends to support lldb-server in several places, it would be nice 
> > to be able to run these tests on Darwin if they work. One of my big goals 
> > for the future of testing on LLDB is to get to the point where the only 
> > differences in test coverage when running tests on different hosts is truly 
> > platform-specific code. Today we are nowhere near that.
> > 
> > Also, as Pavel pointed out in email, the lldb-server tests are also run 
> > against debugserver, so we need to make sure that still works too.
> Which lldb-server support do you refer to here?
> 
> There is some llgs (debugging) support in lldb-server, but I have no idea 
> what's the state of it -- it was added by Todd during his week of code as an 
> "NFC" commit, and it hasn't been touched since. I'd like to avoid this 
> keeping the build red if there is no intention of working on it. 
> 
> The "platform" mode of lldb-server should work on darwin afaik, and we 
> definitely want to be able to run it there. It's not what we are focusing on 
> now though. We'd like to migrate the "debug" tests first (there are no 
> "platform" tests), so the old ones can be removed. 
> 
> In any case, I think of the apple exclusion part as a temporary thing, so we 
> can check this in without breaking the build, we will pretty soon want to 
> include it as well, so that we can run debugserver tests, at least. (At which 
> point we will need a different way of disabling unsupported tests).
I don't know the state of this stuff on Darwin either. I had spent a little 
time a few weeks ago trying to get lldb to use lldb-server on Darwin to see if 
I could answer that question, but I didn't get very far before I had to stop.

As long as the Apple exclusion is temporary I'm fine with this as-is. I'll see 
if I can free up some time this summer to look more closely at lldb-server on 
Darwin and figure out what state it is in.


https://reviews.llvm.org/D32930



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

[Lldb-commits] [lldb] r304314 - Added a testcase for local/namespaced name conflicts.

[Sean Callanan via lldb-commits]

Author: spyffe
Date: Wed May 31 12:18:10 2017
New Revision: 304314

URL: http://llvm.org/viewvc/llvm-project?rev=304314=rev
Log:
Added a testcase for local/namespaced name conflicts.

This works on SVN but is a bit fragile on the Swift branch.
I'm adding the test to both, so we have this path covered.



Added:
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/Makefile

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/TestNamespaceConflicts.py

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/main.cpp

Added: 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/Makefile
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/Makefile?rev=304314=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/Makefile 
(added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/Makefile 
Wed May 31 12:18:10 2017
@@ -0,0 +1,3 @@
+LEVEL = ../../../make
+CXX_SOURCES := main.cpp
+include $(LEVEL)/Makefile.rules

Added: 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/TestNamespaceConflicts.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/TestNamespaceConflicts.py?rev=304314=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/TestNamespaceConflicts.py
 (added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/TestNamespaceConflicts.py
 Wed May 31 12:18:10 2017
@@ -0,0 +1,7 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(
+__file__, globals(), [
+decorators.expectedFailureAll(
+oslist=["windows"], bugnumber="llvm.org/pr24764")])

Added: 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/main.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/main.cpp?rev=304314=auto
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/main.cpp 
(added)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/namespace_conflicts/main.cpp 
Wed May 31 12:18:10 2017
@@ -0,0 +1,29 @@
+//===-- main.cpp *- C++ 
-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+namespace n {
+struct D {
+int i;
+static int anInt() { return 2; }
+int dump() { return i; }
+};
+}
+
+using namespace n;
+
+int foo(D* D) {
+return D->dump(); //% self.expect("expression -- D->dump()", 
DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["int", "2"])
+}
+
+int main (int argc, char const *argv[])
+{
+D myD { D::anInt() };
+foo();
+return 0; 
+}


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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

[Greg Clayton via Phabricator via lldb-commits]

clayborg resigned from this revision.
clayborg added a comment.

I trust Pavel to review this since it is in the Linux native plugins mostly.


https://reviews.llvm.org/D33674



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

[Lldb-commits] [PATCH] D33426: Introduce new command: thread backtrace unique

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.

Looks like a good starting point. Thanks for the changes.


https://reviews.llvm.org/D33426



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

[Lldb-commits] [PATCH] D32930: New framework for lldb client-server communication tests.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for the support, @beanz.




Comment at: unittests/tools/CMakeLists.txt:1
+if(UNIX AND NOT APPLE)
+  add_subdirectory(lldb-server)

beanz wrote:
> labath wrote:
> > This is not what I meant. The only targets (at least until we have 
> > debugserver support) that can realistically pass these tests are linux, 
> > android, and netbsd. The other targets (right now, I guess that would mean 
> > freebsd) don't even pretend to support debugging via lldb-server, so we 
> > should not fail their build because of that. Check for usages of 
> > CMAKE_SYSTEM_NAME to see how to discriminate those.
> Darwin pretends to support lldb-server in several places, it would be nice to 
> be able to run these tests on Darwin if they work. One of my big goals for 
> the future of testing on LLDB is to get to the point where the only 
> differences in test coverage when running tests on different hosts is truly 
> platform-specific code. Today we are nowhere near that.
> 
> Also, as Pavel pointed out in email, the lldb-server tests are also run 
> against debugserver, so we need to make sure that still works too.
Which lldb-server support do you refer to here?

There is some llgs (debugging) support in lldb-server, but I have no idea 
what's the state of it -- it was added by Todd during his week of code as an 
"NFC" commit, and it hasn't been touched since. I'd like to avoid this keeping 
the build red if there is no intention of working on it. 

The "platform" mode of lldb-server should work on darwin afaik, and we 
definitely want to be able to run it there. It's not what we are focusing on 
now though. We'd like to migrate the "debug" tests first (there are no 
"platform" tests), so the old ones can be removed. 

In any case, I think of the apple exclusion part as a temporary thing, so we 
can check this in without breaking the build, we will pretty soon want to 
include it as well, so that we can run debugserver tests, at least. (At which 
point we will need a different way of disabling unsupported tests).


https://reviews.llvm.org/D32930



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

[Lldb-commits] [PATCH] D33426: Introduce new command: thread backtrace unique

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D33426#766525, @bgianfo wrote:

> Address Pavel and Greg's feedback on Diff 100365.
>
> Pavel: I took your suggestions to make the test case more readable,
>  I really appreciate the guidance. I did have to tweak some of the
>  functionality to make the test case pass reliably, as there were
>  still some races possible. I also saw that SBThread.Resume() seems
>  to occasionally result in a StopReason of eStopReasonNon. So I worked
>  around that by only including threads int expected output that the Resume
>  resulted in making it to our breakpoint. I have verified the test is 
>  consistently passes by executing it on repeat 100 times,


Thanks. The fact that we are not able to rely on the operation of Resume in 
this case sounds like a bug. Obviously we can't condition the acceptance of 
this patch by fixing that issue, but we should at least track it. Can you 
create a bug on bugs.llvm.org, and reference it in your workaround. BTW, what's 
the platform you are testing this on?


https://reviews.llvm.org/D33426



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

[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D33035#767029, @abhishek.aggarwal wrote:

> In https://reviews.llvm.org/D33035#754640, @labath wrote:
>
> > I don't really like that we are adding a public shared library for every 
> > tiny intel feature. Could we at least merge this "plugin" with the existing 
> > "intel-mpx plugin" to create one "intel support" library?
> >
> > Also, adding an external dependency probably deserves a discussion on 
> > lldb-dev.
>
>
> Hi Paval ... Before starting the development of this whole feature, we had 
> submitted the full proposal to lldb dev list 1.5 years ago. During the 
> discussions, it was proposed to keep the external dependency outside LLDB 
> (i.e. not to be bundled in liblldb shared library). The External dependency 
> required for this tool is not and will never be a part of lldb repository. 
> Users who are interested to use this tool, will download this external 
> dependency separately.


Yes I remember that. But, as you say, that was 1.5 years ago, and we haven't 
heard anything since. Honestly, I had assumed you abandoned that work until you 
reappeared last month. :)
So I think it's worth updating that thread, as new things may have come up 
since then (for example, the decision whether to merge the intel stuff into a 
single library).


https://reviews.llvm.org/D33035



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

[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

First batch of comments from me, I think I will have some more once I gain more 
insight into this. The main one is about the constructor/initialize, 
destructor/destroy duality, which we should abolish. The rest is mostly 
stylistic.




Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:618
+traceMonitor);
+  if (traceMonitor.get() != nullptr) {
+traceMonitor->setUserID(m_pt_process_uid);

`if(traceMonitor)`



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:619
+  if (traceMonitor.get() != nullptr) {
+traceMonitor->setUserID(m_pt_process_uid);
+m_pt_traced_thread_group.insert(thread_sp->GetID());

As far as, I can tell, every call to `StartProcessorTracing` is followed by 
setUserId. Can we move the logic of setting the id inside the 
`StartProcessorTracing` function



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2483
+template 
+static Status ReadFromFile(const char *filename, T ,
+  std::ios_base::fmtflags mode = (std::ios_base::hex |

Please use llvm file API for this (see how `getProcFile` works -- it's just a 
very thin wrapper around the llvm functions)



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2510
+  if (thread == LLDB_INVALID_THREAD_ID && uid == m_pt_process_uid) {
+if (log)
+  log->Printf("NativeProcessLinux %s_thread not specified: %" PRIu64,

Please use LLDB_LOG (here and everywhere else in this patch)



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2847
+
+Status NativeProcessLinux::StopProcessorTracingOnProcess(lldb::user_id_t uid) {
+  Status ret_error;

You are  calling this function with uid == m_pt_process_uid, which means this 
parameter is redundant, and leads to redundant sanity checks. Please remove it.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2965
+
+  config.setTraceBufferSize(bufsize);
+  config.setMetaDataBufferSize(metabufsize);

You are modifying the config, but the caller is ignoring these modifications. 
If you don't need these,  we could remove the modifications, make the config 
argument `const` and end up with a much cleaner interface.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3056
+
+  auto BufferOrError = getProcFile("cpuinfo");
+  if (!BufferOrError) {

I don't see a getProcFile overload with this signature (although I am not 
opposed to adding one). Are you sure this compiles?



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3064
+std::tie(Line, Rest) = Rest.split('\n');
+{
+  SmallVector columns;

Is this scope necessary? I find it just confuses the reader...



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3070
+continue; // continue searching
+
+  if (log)

add: `columns[1] = columns[1].trim();`
Then you can skip calling trim everywhere else.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3075
+
+  if ((columns[0].find("cpu family") != std::string::npos) &&
+  columns[1].trim(" ").getAsInteger(10, cpu_family))

columns[0].contains(foo)



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3100
+  (stepping != static_cast(-1)) && (!vendor_id.empty()))
+break; // we are done
+}

return here. Then you can avoid duplicating the check outside the loop.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:156
+  // -
+  static size_t ReadCyclicBuffer(void *buf, size_t buf_size, void *cyc_buf,
+ size_t cyc_buf_size, size_t cyc_start,

How about ` void ReadCyclicBuffer(ArrayRef buffer, size_t position, 
MutableArrayRef )`



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:160
+  enum PTErrorCode {
+FileNotFound = 0x23,
+ThreadNotTraced,

This seems suspicious. How did you come to choose this number?



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:317
+  // -
+  class ProcessorTraceMonitor {
+int m_fd;

Please move this into a separate file. NativeProcessLinux.cpp is big enough as 
it is.



Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:331
+  public:
+ProcessorTraceMonitor()
+: m_fd(-1), m_mmap_data(nullptr), m_mmap_aux(nullptr),

This would be much cleaner, if 

[Lldb-commits] [PATCH] D33426: Introduce new command: thread backtrace unique

[Brian Gianforcaro via Phabricator via lldb-commits]

bgianfo updated this revision to Diff 100830.
bgianfo added a comment.

Fix bug in "unique" backtrace output that Greg pointed out.

Introduced a new format for unique frames and plumbed that
through the stacks to be able to toggle between them both
depending on the calling arguments.


https://reviews.llvm.org/D33426

Files:
  include/lldb/Core/Debugger.h
  include/lldb/Target/StackFrame.h
  include/lldb/Target/StackFrameList.h
  include/lldb/Target/Thread.h
  
packages/Python/lldbsuite/test/functionalities/thread/num_threads/TestNumThreads.py
  packages/Python/lldbsuite/test/functionalities/thread/num_threads/main.cpp
  source/Commands/CommandObjectThread.cpp
  source/Core/Debugger.cpp
  source/Target/StackFrame.cpp
  source/Target/StackFrameList.cpp
  source/Target/Thread.cpp

Index: source/Target/Thread.cpp
===
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -1913,47 +1913,50 @@
 
 size_t Thread::GetStatus(Stream , uint32_t start_frame,
  uint32_t num_frames, uint32_t num_frames_with_source,
- bool stop_format) {
-  ExecutionContext exe_ctx(shared_from_this());
-  Target *target = exe_ctx.GetTargetPtr();
-  Process *process = exe_ctx.GetProcessPtr();
-  size_t num_frames_shown = 0;
-  strm.Indent();
-  bool is_selected = false;
-  if (process) {
-if (process->GetThreadList().GetSelectedThread().get() == this)
-  is_selected = true;
-  }
-  strm.Printf("%c ", is_selected ? '*' : ' ');
-  if (target && target->GetDebugger().GetUseExternalEditor()) {
-StackFrameSP frame_sp = GetStackFrameAtIndex(start_frame);
-if (frame_sp) {
-  SymbolContext frame_sc(
-  frame_sp->GetSymbolContext(eSymbolContextLineEntry));
-  if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
-Host::OpenFileInExternalEditor(frame_sc.line_entry.file,
-   frame_sc.line_entry.line);
+ bool stop_format, bool only_stacks) {
+
+  if (!only_stacks) {
+ExecutionContext exe_ctx(shared_from_this());
+Target *target = exe_ctx.GetTargetPtr();
+Process *process = exe_ctx.GetProcessPtr();
+strm.Indent();
+bool is_selected = false;
+if (process) {
+  if (process->GetThreadList().GetSelectedThread().get() == this)
+is_selected = true;
+}
+strm.Printf("%c ", is_selected ? '*' : ' ');
+if (target && target->GetDebugger().GetUseExternalEditor()) {
+  StackFrameSP frame_sp = GetStackFrameAtIndex(start_frame);
+  if (frame_sp) {
+SymbolContext frame_sc(
+frame_sp->GetSymbolContext(eSymbolContextLineEntry));
+if (frame_sc.line_entry.line != 0 && frame_sc.line_entry.file) {
+  Host::OpenFileInExternalEditor(frame_sc.line_entry.file,
+ frame_sc.line_entry.line);
+}
   }
 }
-  }
 
-  DumpUsingSettingsFormat(strm, start_frame, stop_format);
+DumpUsingSettingsFormat(strm, start_frame, stop_format);
+  }
 
+  size_t num_frames_shown = 0;
   if (num_frames > 0) {
 strm.IndentMore();
 
 const bool show_frame_info = true;
-
+const bool show_frame_unique = only_stacks;
 const char *selected_frame_marker = nullptr;
-if (num_frames == 1 ||
+if (num_frames == 1 || only_stacks ||
 (GetID() != GetProcess()->GetThreadList().GetSelectedThread()->GetID()))
   strm.IndentMore();
 else
   selected_frame_marker = "* ";
 
 num_frames_shown = GetStackFrameList()->GetStatus(
 strm, start_frame, num_frames, show_frame_info, num_frames_with_source,
-selected_frame_marker);
+show_frame_unique, selected_frame_marker);
 if (num_frames == 1)
   strm.IndentLess();
 strm.IndentLess();
Index: source/Target/StackFrameList.cpp
===
--- source/Target/StackFrameList.cpp
+++ source/Target/StackFrameList.cpp
@@ -801,7 +801,7 @@
 
 size_t StackFrameList::GetStatus(Stream , uint32_t first_frame,
  uint32_t num_frames, bool show_frame_info,
- uint32_t num_frames_with_source,
+ uint32_t num_frames_with_source, bool show_unique,
  const char *selected_frame_marker) {
   size_t num_frames_displayed = 0;
 
@@ -842,7 +842,7 @@
 
 if (!frame_sp->GetStatus(strm, show_frame_info,
  num_frames_with_source > (first_frame - frame_idx),
- marker))
+ show_unique, marker))
   break;
 ++num_frames_displayed;
   }
Index: source/Target/StackFrame.cpp
===
--- source/Target/StackFrame.cpp
+++ source/Target/StackFrame.cpp
@@ -1744,6 +1744,7 @@
 }
 
 void StackFrame::DumpUsingSettingsFormat(Stream *strm,
+