[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In https://reviews.llvm.org/D45547#1066044, @xiaobai wrote:

> I really like this idea! It will be very helpful for @sas and I. I'd like to 
> +1 creating a separate `stats dump` subcommand instead of dumping stats on 
> `stats disable`.


Thanks! If you has ideas or want to add stuffs, don't hesitate to start a 
discussion and/or submit a review!


Repository:
  rL LLVM

https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: lldb/source/Commands/CommandObjectExpression.cpp:626
 
+  Target *target = m_interpreter.GetExecutionContext().GetTargetPtr();
+  if (!target)

jingham wrote:
> This is what CommandObject::GetSelectedOrDummy target is for.
I just moved existing code, but I agree it's better using this API.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I made a few comments all to the same effect, you can use 
CommandObject::GetSelectedOrDummyTarget to get the target that the command is 
operating on, or you can get is straight from m_exe_ctx if you want to make 
sure that you are getting a real target.  In the case of stats, I think running 
with the dummy target is fine, since for instance you can run expressions in 
the dummy target...

Other than that I think this is a good place to start from.




Comment at: lldb/source/Commands/CommandObjectExpression.cpp:626
 
+  Target *target = m_interpreter.GetExecutionContext().GetTargetPtr();
+  if (!target)

This is what CommandObject::GetSelectedOrDummy target is for.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:30-31
+  bool DoExecute(Args , CommandReturnObject ) override {
+auto exe_ctx = m_interpreter.GetExecutionContext();
+Target *target = exe_ctx.GetTargetPtr();
+

By the time the DoExecute is run, m_exe_ctx has the execution context, so you 
can get the target from there, or use GetSelectedOrDummyTarget.  You can run 
expressions in the dummy target so there's no reason not to collect stats from 
it.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:37
+
+if (target->GetCollectingStats() == true) {
+  result.AppendError("stats already enabled");

xiaobai wrote:
> nit: You can drop the `== true`
Thanks, I'll fix these!


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In https://reviews.llvm.org/D45547#1066348, @jingham wrote:

> Timers seemed like they would be really useful for collection of data about 
> operations in lldb, but for most things I think they end up being hard to use 
> because actual wall-clock time is so variable from run to run, and especially 
> for disk and inter-process heavy operations, which lldb tends to be.  I'm not 
> sure we should give up on timers, sometimes you want to know "how many times 
> did I do X" and other times "how long did X take" and the Timers are more 
> useful for this than just a sample or wall-clock times because you can find 
> out how long it took "in the Dwarf parser", etc.
>
> But in many cases our performance is more driven by unnecessary lookups, and 
> that sort of error.  For that sort of error it will be much more useful to 
> say "given program A and expression B, how many DWARF DIE lookups did I do" 
> than "how long did I spent wall-clock in the DWARF parser..."  When the 
> former goes from 20 to 2000, that will be a much clearer symbol that we 
> probably introduced a performance regression.


Yes, I agree. I addressed all the comments.
Jim, can you please take a look? Thanks!


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide updated this revision to Diff 142301.

https://reviews.llvm.org/D45547

Files:
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/lldb-private-enumerations.h
  lldb/packages/Python/lldbsuite/test/functionalities/stats/Makefile
  lldb/packages/Python/lldbsuite/test/functionalities/stats/TestStats.py
  lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectStats.cpp
  lldb/source/Commands/CommandObjectStats.h
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -87,13 +87,14 @@
   Broadcaster(debugger.GetBroadcasterManager(),
   Target::GetStaticBroadcasterClass().AsCString()),
   ExecutionContextScope(), m_debugger(debugger), m_platform_sp(platform_sp),
-  m_mutex(), m_arch(target_arch),
-  m_images(this), m_section_load_history(), m_breakpoint_list(false),
-  m_internal_breakpoint_list(true), m_watchpoint_list(), m_process_sp(),
-  m_search_filter_sp(), m_image_search_paths(ImageSearchPathsChanged, this),
-  m_ast_importer_sp(), m_source_manager_ap(), m_stop_hooks(),
-  m_stop_hook_next_id(0), m_valid(true), m_suppress_stop_hooks(false),
-  m_is_dummy_target(is_dummy_target)
+  m_mutex(), m_arch(target_arch), m_images(this), m_section_load_history(),
+  m_breakpoint_list(false), m_internal_breakpoint_list(true),
+  m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
+  m_image_search_paths(ImageSearchPathsChanged, this), m_ast_importer_sp(),
+  m_source_manager_ap(), m_stop_hooks(), m_stop_hook_next_id(0),
+  m_valid(true), m_suppress_stop_hooks(false),
+  m_is_dummy_target(is_dummy_target),
+  m_stats_storage(static_cast(StatisticKind::StatisticMax))
 
 {
   SetEventName(eBroadcastBitBreakpointChanged, "breakpoint-changed");
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -425,7 +425,7 @@
   CommandObjectSP(new CommandObjectMultiwordSettings(*this));
   m_command_dict["source"] =
   CommandObjectSP(new CommandObjectMultiwordSource(*this));
-  m_command_dict["stats"] = CommandObjectSP(new CommandObjectStats(*this));
+  m_command_dict["statistics"] = CommandObjectSP(new CommandObjectStats(*this));
   m_command_dict["target"] =
   CommandObjectSP(new CommandObjectMultiwordTarget(*this));
   m_command_dict["thread"] =
Index: lldb/source/Commands/CommandObjectStats.h
===
--- lldb/source/Commands/CommandObjectStats.h
+++ lldb/source/Commands/CommandObjectStats.h
@@ -11,16 +11,14 @@
 #define liblldb_CommandObjectStats_h_
 
 #include "lldb/Interpreter/CommandObject.h"
+#include "lldb/Interpreter/CommandObjectMultiword.h"
 
 namespace lldb_private {
-class CommandObjectStats : public CommandObjectParsed {
+class CommandObjectStats : public CommandObjectMultiword {
 public:
   CommandObjectStats(CommandInterpreter );
 
   ~CommandObjectStats() override;
-
-protected:
-  bool DoExecute(Args , CommandReturnObject ) override;
 };
 } // namespace lldb_private
 
Index: lldb/source/Commands/CommandObjectStats.cpp
===
--- lldb/source/Commands/CommandObjectStats.cpp
+++ lldb/source/Commands/CommandObjectStats.cpp
@@ -11,18 +11,118 @@
 #include "lldb/Host/Host.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Target/Target.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-CommandObjectStats::CommandObjectStats(CommandInterpreter )
-: CommandObjectParsed(
-  interpreter, "stats", "Print statistics about a debugging session",
-  nullptr) {
-}
+class CommandObjectStatsEnable : public CommandObjectParsed {
+public:
+  CommandObjectStatsEnable(CommandInterpreter )
+  : CommandObjectParsed(
+interpreter, "enable", "Enable statistics collection", nullptr,
+eCommandRequiresTarget | eCommandProcessMustBePaused) {}
+
+  ~CommandObjectStatsEnable() override = default;
+
+protected:
+  bool DoExecute(Args , CommandReturnObject ) override {
+auto exe_ctx = m_interpreter.GetExecutionContext();
+Target *target = exe_ctx.GetTargetPtr();
+
+if (target->GetCollectingStats() == true) {
+  result.AppendError("statistics already enabled");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
+target->SetCollectingStats(true);
+result.SetStatus(eReturnStatusSuccessFinishResult);
+return true;
+  }
+};
+
+class 

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Timers seemed like they would be really useful for collection of data about 
operations in lldb, but for most things I think they end up being hard to use 
because actual wall-clock time is so variable from run to run, and especially 
for disk and inter-process heavy operations, which lldb tends to be.  I'm not 
sure we should give up on timers, sometimes you want to know "how many times 
did I do X" and other times "how long did X take" and the Timers are more 
useful for this than just a sample or wall-clock times because you can find out 
how long it took "in the Dwarf parser", etc.

But in many cases our performance is more driven by unnecessary lookups, and 
that sort of error.  For that sort of error it will be much more useful to say 
"given program A and expression B, how many DWARF DIE lookups did I do" than 
"how long did I spent wall-clock in the DWARF parser..."  When the former goes 
from 20 to 2000, that will be a much clearer symbol that we probably introduced 
a performance regression.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Alex Langford via Phabricator via lldb-commits]

xiaobai added subscribers: sas, xiaobai.
xiaobai added a comment.

I really like this idea! It will be very helpful for @sas and I. I'd like to +1 
creating a separate `stats dump` subcommand instead of dumping stats on `stats 
disable`.




Comment at: lldb/source/Commands/CommandObjectStats.cpp:37
+
+if (target->GetCollectingStats() == true) {
+  result.AppendError("stats already enabled");

nit: You can drop the `== true`



Comment at: lldb/source/Commands/CommandObjectStats.cpp:88
+
+if (target->GetCollectingStats() == false) {
+  result.AppendError("need to enable stats before disabling them");

nit: 
```
 if (!target->GetCollectingStats()) {
```


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

I'm under the impression that we should either merge `log timers` with `stats` 
or just remove log timers altogether and start from scratch.
From what I hear from Jim, it was really useful for a few people, so maybe a 
fresh start would be a better way of handling things. Thanks!


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added inline comments.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);

labath wrote:
> davide wrote:
> > labath wrote:
> > > Are you planning to have some kind of decentralized method of registering 
> > > statistics?
> > > If not, then I don't see a need for explicitly registering the statistic 
> > > kinds here, since the single source of truth about the available kinds 
> > > can be StatisticKind enum, and Target can just get the list from there 
> > > automatically when need (when stats are enabled?)
> > > 
> > > This way we could even simplify the code by avoiding the "statistic is 
> > > not registered but someone still called IncrementStats" state altogether) 
> > > and the stats map could be a simple array with NumStatistics elements.
> > No. I originally thought to make it that way but I have to say that maybe 
> > it's easier to have a centralized mechanism. I'm still unsure whether this 
> > should be a vector or a map, I'll think about it more.
> llvm manual recommends 
> 
>  vector as a first choice data structure for a map. :P
Allright, you convinced me :)


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D45547#1065661, @davide wrote:

> I prefer having it as a top level command rather than part of log. If you 
> think about it LLVM does the same distinction and it worked quite well in 
> practice.
>  We plan to collect more metrics to the command so I'd very much like to have 
> this living as a separate entity.


I agree that "logging" and "statistics" sound like two different entities. 
Though in that case, maybe it's the "log timers" command that is misplaced. 
"performance timers" sound more like "statistics" than "logging" to me. Maybe 
eventually these two things should be merged even? I don't know what's the 
right solution here, and I don't care much about it even, though I would be sad 
if the choice we make now prevents some future unification due to backwards 
compatibility issues.




Comment at: lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c:12
+  //%self.expect("stats enable")
+  //%self.expect("frame var", substrs=['27'])
+  //%self.expect("stats disable", substrs=['frame var successes : 1', 'frame 
var failures : 0'])

davide wrote:
> labath wrote:
> > Maybe test for failure as well (I guess trying to print an non-existing 
> > variable should do the trick)?
> Testing for failure here is a little tricky because the expression parser 
> reports a failure only when it fails internally (and not when succeeds but 
> returns an error because clangAST can't find the variable). Eventually we 
> could make it more fine grained but for now I just would like to get the 
> number of failures resolving variable due to expression parsing logic failing 
> (and not because typos).
Makes sense, thanks for the explanation.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);

davide wrote:
> labath wrote:
> > Are you planning to have some kind of decentralized method of registering 
> > statistics?
> > If not, then I don't see a need for explicitly registering the statistic 
> > kinds here, since the single source of truth about the available kinds can 
> > be StatisticKind enum, and Target can just get the list from there 
> > automatically when need (when stats are enabled?)
> > 
> > This way we could even simplify the code by avoiding the "statistic is not 
> > registered but someone still called IncrementStats" state altogether) and 
> > the stats map could be a simple array with NumStatistics elements.
> No. I originally thought to make it that way but I have to say that maybe 
> it's easier to have a centralized mechanism. I'm still unsure whether this 
> should be a vector or a map, I'll think about it more.
llvm manual recommends 

 vector as a first choice data structure for a map. :P


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

I prefer having it as a top level command rather than part of log. If you think 
about it LLVM does the same distinction and it worked quite well in practice.
We plan to collect more metrics to the command so I'd very much like to have 
this living as a separate entity.




Comment at: lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c:12
+  //%self.expect("stats enable")
+  //%self.expect("frame var", substrs=['27'])
+  //%self.expect("stats disable", substrs=['frame var successes : 1', 'frame 
var failures : 0'])

labath wrote:
> Maybe test for failure as well (I guess trying to print an non-existing 
> variable should do the trick)?
Testing for failure here is a little tricky because the expression parser 
reports a failure only when it fails internally (and not when succeeds but 
returns an error because clangAST can't find the variable). Eventually we could 
make it more fine grained but for now I just would like to get the number of 
failures resolving variable due to expression parsing logic failing (and not 
because typos).



Comment at: lldb/source/Commands/CommandObjectStats.cpp:31-35
+if (!target) {
+  result.AppendError("stats command needs a target");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}

labath wrote:
> I think we have some way (should involve `eCommandRequiresTarget`) to avoid 
> the need for checking this.
I'll check this out, thanks!



Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);

labath wrote:
> Are you planning to have some kind of decentralized method of registering 
> statistics?
> If not, then I don't see a need for explicitly registering the statistic 
> kinds here, since the single source of truth about the available kinds can be 
> StatisticKind enum, and Target can just get the list from there automatically 
> when need (when stats are enabled?)
> 
> This way we could even simplify the code by avoiding the "statistic is not 
> registered but someone still called IncrementStats" state altogether) and the 
> stats map could be a simple array with NumStatistics elements.
No. I originally thought to make it that way but I have to say that maybe it's 
easier to have a centralized mechanism. I'm still unsure whether this should be 
a vector or a map, I'll think about it more.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't see myself using this command personally, but I agree that splitting 
the "disable stats collection" and "dump accumulated stats" into two actions 
seems a better choice (maybe disable could do a final auto-dump though). I also 
think the header/footer is not needed for a primarily interactive tool. If we 
have a command that dumps so much data that the user cannot tell where the 
output starts and ends then we're doing something wrong (PS: I wish we had 
automatic pagination for the backtrace command).

I am also wondering whether statistics should be a top-level command, or should 
we put it under "log". We already have "log timers enable/disable/...", which 
are doing something similar to this, so it seems like a good idea to move these 
two things closer together.




Comment at: lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c:12
+  //%self.expect("stats enable")
+  //%self.expect("frame var", substrs=['27'])
+  //%self.expect("stats disable", substrs=['frame var successes : 1', 'frame 
var failures : 0'])

Maybe test for failure as well (I guess trying to print an non-existing 
variable should do the trick)?



Comment at: lldb/source/Commands/CommandObjectStats.cpp:31-35
+if (!target) {
+  result.AppendError("stats command needs a target");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}

I think we have some way (should involve `eCommandRequiresTarget`) to avoid the 
need for checking this.



Comment at: lldb/source/Commands/CommandObjectStats.cpp:43-46
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);

Are you planning to have some kind of decentralized method of registering 
statistics?
If not, then I don't see a need for explicitly registering the statistic kinds 
here, since the single source of truth about the available kinds can be 
StatisticKind enum, and Target can just get the list from there automatically 
when need (when stats are enabled?)

This way we could even simplify the code by avoiding the "statistic is not 
registered but someone still called IncrementStats" state altogether) and the 
stats map could be a simple array with NumStatistics elements.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In https://reviews.llvm.org/D45547#1065054, @jasonmolenda wrote:

> Ah, no you couldn't set up a command alias like that.  Still, if the full 
> name was statistics, you could type 'stat' and it would match.  'stats' 
> wouldn't, though.
>
> I do think decoupling the disabling action from the dumping action would be 
> an improvement.  You may want to dump the current statistics multiple times 
> during a sequence of commands, without disabling / re-enabling every time.  
> And dropping the header/footer from the results.


OK! I don't mind adding another `dump` subcommand. I think it makes sense. 
I can also disable the footer/header, I largely got that format from `opt 
-time-passes`. I don't feel strongly about it, so I'll let everybody else 
decide :)


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Ah, no you couldn't set up a command alias like that.  Still, if the full name 
was statistics, you could type 'stat' and it would match.  'stats' wouldn't, 
though.

I do think decoupling the disabling action from the dumping action would be an 
improvement.  You may want to dump the current statistics multiple times during 
a sequence of commands, without disabling / re-enabling every time.  And 
dropping the header/footer from the results.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

My two cents,

Why print the '### Start STATISTICS dump ###' header/footer when printing the 
results?  I don't think we demarcate result output like that anywhere else in 
lldb.  I don't think it adds anything for the user.

I would probably name the command statistics and have a built in "stats" alias.

Should statistics disable print the results, or should there be a 'statistics 
status' or 'statistics show' or 'statistics dump' command to show the 
statistics results accumulated so far?


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Davide Italiano via Phabricator via lldb-commits]

davide created this revision.
davide added reviewers: jingham, friss, JDevlieghere, aprantl, labath, clayborg.

This allows us to collect useful metrics about lldb debugging sessions.

I thought that an example would be better than a thousand words:

  Process 19705 stopped
  * thread #1, queue = 'com.apple.main-thread', stop reason = step in
  frame #0: 0x00010fb4 blah`main at blah.c:3
 1int main(void) {
 2  int a = 6;
  -> 3  return 0;
 4}
  (lldb) stats enable
  (lldb) frame var a
  (int) a = 6
  (lldb) expr a
  (int) $1 = 6
  (lldb) stats disable
  ### Start STATISTICS dump ###
  Number of expr evaluation successes : 1
  Number of expr evaluation failures : 0
  Number of frame var successes : 1
  Number of frame var failures : 0
  ### End STATISTICS dump ###

Future improvements might include:

1. Passing a file, or implementing categories. The way this patch has been 
implements is generic enough to allow this to be extended easily without 
breaking the grammar
2. Adding an SBAPI and Python API for use in scripts.

Thanks to Jim Ingham for discussing the design with me.


https://reviews.llvm.org/D45547

Files:
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/lldb-private-enumerations.h
  lldb/packages/Python/lldbsuite/test/functionalities/stats/Makefile
  lldb/packages/Python/lldbsuite/test/functionalities/stats/TestStats.py
  lldb/packages/Python/lldbsuite/test/functionalities/stats/main.c
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectStats.cpp
  lldb/source/Commands/CommandObjectStats.h

Index: lldb/source/Commands/CommandObjectStats.h
===
--- lldb/source/Commands/CommandObjectStats.h
+++ lldb/source/Commands/CommandObjectStats.h
@@ -11,16 +11,14 @@
 #define liblldb_CommandObjectStats_h_
 
 #include "lldb/Interpreter/CommandObject.h"
+#include "lldb/Interpreter/CommandObjectMultiword.h"
 
 namespace lldb_private {
-class CommandObjectStats : public CommandObjectParsed {
+class CommandObjectStats : public CommandObjectMultiword {
 public:
   CommandObjectStats(CommandInterpreter );
 
   ~CommandObjectStats() override;
-
-protected:
-  bool DoExecute(Args , CommandReturnObject ) override;
 };
 } // namespace lldb_private
 
Index: lldb/source/Commands/CommandObjectStats.cpp
===
--- lldb/source/Commands/CommandObjectStats.cpp
+++ lldb/source/Commands/CommandObjectStats.cpp
@@ -11,18 +11,107 @@
 #include "lldb/Host/Host.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Target/Target.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-CommandObjectStats::CommandObjectStats(CommandInterpreter )
-: CommandObjectParsed(
-  interpreter, "stats", "Print statistics about a debugging session",
-  nullptr) {
-}
+class CommandObjectStatsEnable : public CommandObjectParsed {
+public:
+  CommandObjectStatsEnable(CommandInterpreter )
+  : CommandObjectParsed(interpreter, "enable",
+"Enable statistics collection", nullptr) {}
+
+  ~CommandObjectStatsEnable() override = default;
+
+  bool DoExecute(Args , CommandReturnObject ) override {
+auto exe_ctx = m_interpreter.GetExecutionContext();
+Target *target = exe_ctx.GetTargetPtr();
+
+if (!target) {
+  result.AppendError("stats command needs a target");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
+if (target->GetCollectingStats() == true) {
+  result.AppendError("stats already enabled");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
+target->RegisterStats(StatisticKind::ExpressionSuccessful);
+target->RegisterStats(StatisticKind::ExpressionFailure);
+target->RegisterStats(StatisticKind::FrameVarSuccess);
+target->RegisterStats(StatisticKind::FrameVarFailure);
+
+target->SetCollectingStats(true);
+result.SetStatus(eReturnStatusSuccessFinishResult);
+return true;
+  }
+};
+
+class CommandObjectStatsDisable : public CommandObjectParsed {
+private:
+  std::string GetStatDescription(lldb_private::StatisticKind K) {
+switch (K) {
+case StatisticKind::ExpressionSuccessful:
+  return "Number of expr evaluation successes";
+case StatisticKind::ExpressionFailure:
+  return "Number of expr evaluation failures";
+case StatisticKind::FrameVarSuccess:
+  return "Number of frame var successes";
+case StatisticKind::FrameVarFailure:
+  return "Number of frame var failures";
+}
+llvm_unreachable("Statistic not registered!");
+  }
 
-bool CommandObjectStats::DoExecute(Args , CommandReturnObject ) {
-  return true;
+public:
+  CommandObjectStatsDisable(CommandInterpreter )
+  : CommandObjectParsed(interpreter, "disable",
+