[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370571: [lldb] Unify target checking in CommandObject 
(authored by teemperor, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66863?vs=218117=218207#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66863

Files:
  lldb/trunk/include/lldb/Interpreter/CommandObject.h
  lldb/trunk/source/Commands/CommandObjectBreakpointCommand.cpp
  lldb/trunk/source/Commands/CommandObjectDisassemble.cpp
  lldb/trunk/source/Commands/CommandObjectProcess.cpp
  lldb/trunk/source/Commands/CommandObjectTarget.cpp
  lldb/trunk/source/Commands/CommandObjectThread.cpp
  lldb/trunk/source/Commands/CommandObjectWatchpoint.cpp
  lldb/trunk/source/Commands/CommandObjectWatchpointCommand.cpp
  lldb/trunk/source/Interpreter/CommandObject.cpp

Index: lldb/trunk/include/lldb/Interpreter/CommandObject.h
===
--- lldb/trunk/include/lldb/Interpreter/CommandObject.h
+++ lldb/trunk/include/lldb/Interpreter/CommandObject.h
@@ -331,6 +331,7 @@
   // selected target, or if no target is present you want to prime the dummy
   // target with entities that will be copied over to new targets.
   Target (bool prefer_dummy = false);
+  Target ();
   Target ();
 
   // If a command needs to use the "current" thread, use this call. Command
Index: lldb/trunk/source/Interpreter/CommandObject.cpp
===
--- lldb/trunk/source/Interpreter/CommandObject.cpp
+++ lldb/trunk/source/Interpreter/CommandObject.cpp
@@ -925,6 +925,15 @@
   return *m_interpreter.GetDebugger().GetSelectedOrDummyTarget(prefer_dummy);
 }
 
+Target ::GetSelectedTarget() {
+  assert(m_flags.AnySet(eCommandRequiresTarget | eCommandProcessMustBePaused |
+eCommandProcessMustBeLaunched | eCommandRequiresFrame |
+eCommandRequiresThread | eCommandRequiresProcess |
+eCommandRequiresRegContext) &&
+ "GetSelectedTarget called from object that may have no target");
+  return *m_interpreter.GetDebugger().GetSelectedTarget();
+}
+
 Thread *CommandObject::GetDefaultThread() {
   Thread *thread_to_use = m_exe_ctx.GetThreadPtr();
   if (thread_to_use)
Index: lldb/trunk/source/Commands/CommandObjectThread.cpp
===
--- lldb/trunk/source/Commands/CommandObjectThread.cpp
+++ lldb/trunk/source/Commands/CommandObjectThread.cpp
@@ -819,13 +819,6 @@
   bool DoExecute(Args , CommandReturnObject ) override {
 bool synchronous_execution = m_interpreter.GetSynchronous();
 
-if (!GetDebugger().GetSelectedTarget()) {
-  result.AppendError("invalid target, create a debug target using the "
- "'target create' command");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
-
 Process *process = m_exe_ctx.GetProcessPtr();
 if (process == nullptr) {
   result.AppendError("no process exists. Cannot continue");
@@ -1091,13 +1084,7 @@
   bool DoExecute(Args , CommandReturnObject ) override {
 bool synchronous_execution = m_interpreter.GetSynchronous();
 
-Target *target = GetDebugger().GetSelectedTarget().get();
-if (target == nullptr) {
-  result.AppendError("invalid target, create a debug target using the "
- "'target create' command");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
+Target *target = ();
 
 Process *process = m_exe_ctx.GetProcessPtr();
 if (process == nullptr) {
Index: lldb/trunk/source/Commands/CommandObjectBreakpointCommand.cpp
===
--- lldb/trunk/source/Commands/CommandObjectBreakpointCommand.cpp
+++ lldb/trunk/source/Commands/CommandObjectBreakpointCommand.cpp
@@ -589,10 +589,10 @@
 class CommandObjectBreakpointCommandList : public CommandObjectParsed {
 public:
   CommandObjectBreakpointCommandList(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "list", "List the script or set of "
- "commands to be executed when "
- "the breakpoint is hit.",
-nullptr) {
+  : CommandObjectParsed(interpreter, "list",
+"List the script or set of commands to be "
+"executed when the breakpoint is hit.",
+nullptr, eCommandRequiresTarget) {
 CommandArgumentEntry arg;
 CommandArgumentData bp_id_arg;
 
@@ -612,14 +612,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == 

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 218117.
teemperor marked 3 inline comments as done.
teemperor added a comment.

- Fix typo in code and comment.
- Reflow some changed strings.


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

https://reviews.llvm.org/D66863

Files:
  lldb/include/lldb/Interpreter/CommandObject.h
  lldb/source/Commands/CommandObjectBreakpointCommand.cpp
  lldb/source/Commands/CommandObjectDisassemble.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/CommandObjectWatchpoint.cpp
  lldb/source/Commands/CommandObjectWatchpointCommand.cpp
  lldb/source/Interpreter/CommandObject.cpp

Index: lldb/source/Interpreter/CommandObject.cpp
===
--- lldb/source/Interpreter/CommandObject.cpp
+++ lldb/source/Interpreter/CommandObject.cpp
@@ -925,6 +925,15 @@
   return *m_interpreter.GetDebugger().GetSelectedOrDummyTarget(prefer_dummy);
 }
 
+Target ::GetSelectedTarget() {
+  assert(m_flags.AnySet(eCommandRequiresTarget | eCommandProcessMustBePaused |
+eCommandProcessMustBeLaunched | eCommandRequiresFrame |
+eCommandRequiresThread | eCommandRequiresProcess |
+eCommandRequiresRegContext) &&
+ "GetSelectedTarget called from object that may have no target");
+  return *m_interpreter.GetDebugger().GetSelectedTarget();
+}
+
 Thread *CommandObject::GetDefaultThread() {
   Thread *thread_to_use = m_exe_ctx.GetThreadPtr();
   if (thread_to_use)
Index: lldb/source/Commands/CommandObjectWatchpointCommand.cpp
===
--- lldb/source/Commands/CommandObjectWatchpointCommand.cpp
+++ lldb/source/Commands/CommandObjectWatchpointCommand.cpp
@@ -59,7 +59,7 @@
   : CommandObjectParsed(interpreter, "add",
 "Add a set of LLDB commands to a watchpoint, to be "
 "executed whenever the watchpoint is hit.",
-nullptr),
+nullptr, eCommandRequiresTarget),
 IOHandlerDelegateMultiline("DONE",
IOHandlerDelegate::Completion::LLDBCommand),
 m_options() {
@@ -389,14 +389,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == nullptr) {
-  result.AppendError("There is not a current executable; there are no "
- "watchpoints to which to add commands");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
+Target *target = ();
 
 const WatchpointList  = target->GetWatchpointList();
 size_t num_watchpoints = watchpoints.GetSize();
@@ -486,7 +479,7 @@
   CommandObjectWatchpointCommandDelete(CommandInterpreter )
   : CommandObjectParsed(interpreter, "delete",
 "Delete the set of commands from a watchpoint.",
-nullptr) {
+nullptr, eCommandRequiresTarget) {
 CommandArgumentEntry arg;
 CommandArgumentData wp_id_arg;
 
@@ -506,14 +499,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == nullptr) {
-  result.AppendError("There is not a current executable; there are no "
- "watchpoints from which to delete commands");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
+Target *target = ();
 
 const WatchpointList  = target->GetWatchpointList();
 size_t num_watchpoints = watchpoints.GetSize();
@@ -562,10 +548,10 @@
 class CommandObjectWatchpointCommandList : public CommandObjectParsed {
 public:
   CommandObjectWatchpointCommandList(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "list", "List the script or set of "
- "commands to be executed when "
- "the watchpoint is hit.",
-nullptr) {
+  : CommandObjectParsed(interpreter, "list",
+"List the script or set of commands to be executed "
+"when the watchpoint is hit.",
+nullptr, eCommandRequiresTarget) {
 CommandArgumentEntry arg;
 CommandArgumentData wp_id_arg;
 
@@ -585,14 +571,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == nullptr) {
-  result.AppendError("There is not a current executable; there are no "
- "watchpoints for which to list commands");
-  result.SetStatus(eReturnStatusFailed);
-  

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Greg Clayton via Phabricator via lldb-commits]

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

Just a few string reflows and fix a typo




Comment at: lldb/source/Commands/CommandObjectBreakpointCommand.cpp:593
+  : CommandObjectParsed(interpreter, "list",
+"List the script or set of  commands to be "
+"executed when the breakpoint is hit.",

replace multiple spaces with just one



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:2615-2617
+"Set the load addresses for "
+"one or more sections in a "
+"target module.",

Fix string



Comment at: lldb/source/Commands/CommandObjectWatchpointCommand.cpp:552-554
+"List the script or set of "
+"commands to be executed when "
+"the watchpoint is hit.",

reflow the string



Comment at: lldb/source/Interpreter/CommandObject.cpp:929
+Target ::GetSelectedTarget() {
+  assert(m_flags.AnySe(eCommandRequiresTarget | eCommandProcessMustBePaused |
+   eCommandProcessMustBeLaunched | eCommandRequiresFrame |

s/AnySe/AnySet/


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

https://reviews.llvm.org/D66863



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

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 218084.
teemperor added a comment.

- Removed target requirement where we already have another flag that's implying 
that we need a target.
- Properly formatted some strings.
- Extended check in GetSelectedTarget to also check for flags that imply 
eCommandRequiresTarget.


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

https://reviews.llvm.org/D66863

Files:
  lldb/include/lldb/Interpreter/CommandObject.h
  lldb/source/Commands/CommandObjectBreakpointCommand.cpp
  lldb/source/Commands/CommandObjectDisassemble.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/CommandObjectWatchpoint.cpp
  lldb/source/Commands/CommandObjectWatchpointCommand.cpp
  lldb/source/Interpreter/CommandObject.cpp

Index: lldb/source/Interpreter/CommandObject.cpp
===
--- lldb/source/Interpreter/CommandObject.cpp
+++ lldb/source/Interpreter/CommandObject.cpp
@@ -925,6 +925,15 @@
   return *m_interpreter.GetDebugger().GetSelectedOrDummyTarget(prefer_dummy);
 }
 
+Target ::GetSelectedTarget() {
+  assert(m_flags.AnySe(eCommandRequiresTarget | eCommandProcessMustBePaused |
+   eCommandProcessMustBeLaunched | eCommandRequiresFrame |
+   eCommandRequiresThread | eCommandRequiresProcess |
+   eCommandRequiresRegContext) &&
+ "GetSelectedTarget called from object that may have no target");
+  return *m_interpreter.GetDebugger().GetSelectedTarget();
+}
+
 Thread *CommandObject::GetDefaultThread() {
   Thread *thread_to_use = m_exe_ctx.GetThreadPtr();
   if (thread_to_use)
Index: lldb/source/Commands/CommandObjectWatchpointCommand.cpp
===
--- lldb/source/Commands/CommandObjectWatchpointCommand.cpp
+++ lldb/source/Commands/CommandObjectWatchpointCommand.cpp
@@ -59,7 +59,7 @@
   : CommandObjectParsed(interpreter, "add",
 "Add a set of LLDB commands to a watchpoint, to be "
 "executed whenever the watchpoint is hit.",
-nullptr),
+nullptr, eCommandRequiresTarget),
 IOHandlerDelegateMultiline("DONE",
IOHandlerDelegate::Completion::LLDBCommand),
 m_options() {
@@ -389,14 +389,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == nullptr) {
-  result.AppendError("There is not a current executable; there are no "
- "watchpoints to which to add commands");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
+Target *target = ();
 
 const WatchpointList  = target->GetWatchpointList();
 size_t num_watchpoints = watchpoints.GetSize();
@@ -486,7 +479,7 @@
   CommandObjectWatchpointCommandDelete(CommandInterpreter )
   : CommandObjectParsed(interpreter, "delete",
 "Delete the set of commands from a watchpoint.",
-nullptr) {
+nullptr, eCommandRequiresTarget) {
 CommandArgumentEntry arg;
 CommandArgumentData wp_id_arg;
 
@@ -506,14 +499,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == nullptr) {
-  result.AppendError("There is not a current executable; there are no "
- "watchpoints from which to delete commands");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
+Target *target = ();
 
 const WatchpointList  = target->GetWatchpointList();
 size_t num_watchpoints = watchpoints.GetSize();
@@ -562,10 +548,11 @@
 class CommandObjectWatchpointCommandList : public CommandObjectParsed {
 public:
   CommandObjectWatchpointCommandList(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "list", "List the script or set of "
- "commands to be executed when "
- "the watchpoint is hit.",
-nullptr) {
+  : CommandObjectParsed(interpreter, "list",
+"List the script or set of "
+"commands to be executed when "
+"the watchpoint is hit.",
+nullptr, eCommandRequiresTarget) {
 CommandArgumentEntry arg;
 CommandArgumentData wp_id_arg;
 
@@ -585,14 +572,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == nullptr) {
-  result.AppendError("There is not a 

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Awesome.

In D66863#1649436 , @jingham wrote:

> As a minor style thing, prior to this change CommandObjectThreadStepUntil 
> required a thread, but didn't require a process explicitly, because you can't 
> have a thread without a process.  You also can't have a thread without a 
> target since you can't have a process without a target.  But your change 
> means that if I specify that I require a thread I now ALSO have to require a 
> target explicitly or I will get an assert calling GetSelectedTarget.  That 
> seems a little awkward to me (especially since you still don't have to 
> require a process...)  It would be better if eCommandRequiresThread implied 
> eCommandRequiresProcess & eCommandRequiresTarget.  That would keep these 
> definitions less noisy.


This sounds like a good idea to me.




Comment at: lldb/source/Commands/CommandObjectTarget.cpp:2615-2617
+"Set the load addresses for "
+"one or more sections in a "
+"target module.",

Another quirk of clang-format is that it will not re-merge strings that it has 
split previously. If you manually join these three strings into one, and then 
re-run clang-format, you'll probably end up with something slightly nicer...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66863



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

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.
Herald added a subscriber: JDevlieghere.

The whole command flags was a late addition, but we were using it for new 
commands and "when you touch it" kind of changes.  That seems to have stalled 
the conversion, so thanks for completing this!

As a minor style thing, prior to this change CommandObjectThreadStepUntil 
required a thread, but didn't require a process explicitly, because you can't 
have a thread without a process.  You also can't have a thread without a target 
since you can't have a process without a target.  But your change means that if 
I specify that I require a thread I now ALSO have to require a target 
explicitly or I will get an assert calling GetSelectedTarget.  That seems a 
little awkward to me (especially since you still don't have to require a 
process...)  It would be better if eCommandRequiresThread implied 
eCommandRequiresProcess & eCommandRequiresTarget.  That would keep these 
definitions less noisy.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66863



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

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I support anything that reduces the code path differences between user-entered 
commands and their SBAPI counterparts.  Thanks for doing this!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D66863



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

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added a reviewer: labath.
Herald added subscribers: lldb-commits, abidh.
Herald added a project: LLDB.

We currently have several CommandObjects that manually reimplement the checking 
for a selected target
or a target in the execution context (which is the selected target when they 
are invoked). This patch removes
all these checks and replaces them by setting the eCommandRequiresTarget flag 
that Pavel suggested. With
this flag we are doing the same check but without having to duplicate this code 
in all these CommandObjects.

I also added a `GetSelectedTarget()` variant of the 
`GetSelectedOrDummyTarget()` function to the
CommandObject that checks that the flag is set and then returns a reference to 
the target. I didn't rewrite
all the `target` variables from `Target *` to `Target &` in this patch as last 
time this change caused a lot of merge
conflicts in Swift and I would prefer having that in a separate NFC commit.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D66863

Files:
  lldb/include/lldb/Interpreter/CommandObject.h
  lldb/source/Commands/CommandObjectBreakpointCommand.cpp
  lldb/source/Commands/CommandObjectDisassemble.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/CommandObjectWatchpoint.cpp
  lldb/source/Commands/CommandObjectWatchpointCommand.cpp
  lldb/source/Interpreter/CommandObject.cpp

Index: lldb/source/Interpreter/CommandObject.cpp
===
--- lldb/source/Interpreter/CommandObject.cpp
+++ lldb/source/Interpreter/CommandObject.cpp
@@ -925,6 +925,12 @@
   return *m_interpreter.GetDebugger().GetSelectedOrDummyTarget(prefer_dummy);
 }
 
+Target ::GetSelectedTarget() {
+  assert(m_flags.AllSet(eCommandRequiresTarget) &&
+ "GetSelectedTarget called from object that may have no target");
+  return *m_interpreter.GetDebugger().GetSelectedTarget();
+}
+
 Thread *CommandObject::GetDefaultThread() {
   Thread *thread_to_use = m_exe_ctx.GetThreadPtr();
   if (thread_to_use)
Index: lldb/source/Commands/CommandObjectWatchpointCommand.cpp
===
--- lldb/source/Commands/CommandObjectWatchpointCommand.cpp
+++ lldb/source/Commands/CommandObjectWatchpointCommand.cpp
@@ -59,7 +59,7 @@
   : CommandObjectParsed(interpreter, "add",
 "Add a set of LLDB commands to a watchpoint, to be "
 "executed whenever the watchpoint is hit.",
-nullptr),
+nullptr, eCommandRequiresTarget),
 IOHandlerDelegateMultiline("DONE",
IOHandlerDelegate::Completion::LLDBCommand),
 m_options() {
@@ -389,14 +389,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == nullptr) {
-  result.AppendError("There is not a current executable; there are no "
- "watchpoints to which to add commands");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
+Target *target = ();
 
 const WatchpointList  = target->GetWatchpointList();
 size_t num_watchpoints = watchpoints.GetSize();
@@ -486,7 +479,7 @@
   CommandObjectWatchpointCommandDelete(CommandInterpreter )
   : CommandObjectParsed(interpreter, "delete",
 "Delete the set of commands from a watchpoint.",
-nullptr) {
+nullptr, eCommandRequiresTarget) {
 CommandArgumentEntry arg;
 CommandArgumentData wp_id_arg;
 
@@ -506,14 +499,7 @@
 
 protected:
   bool DoExecute(Args , CommandReturnObject ) override {
-Target *target = GetDebugger().GetSelectedTarget().get();
-
-if (target == nullptr) {
-  result.AppendError("There is not a current executable; there are no "
- "watchpoints from which to delete commands");
-  result.SetStatus(eReturnStatusFailed);
-  return false;
-}
+Target *target = ();
 
 const WatchpointList  = target->GetWatchpointList();
 size_t num_watchpoints = watchpoints.GetSize();
@@ -562,10 +548,11 @@
 class CommandObjectWatchpointCommandList : public CommandObjectParsed {
 public:
   CommandObjectWatchpointCommandList(CommandInterpreter )
-  : CommandObjectParsed(interpreter, "list", "List the script or set of "
- "commands to be executed when "
- "the watchpoint is hit.",
-nullptr) {
+  : CommandObjectParsed(interpreter, "list",
+"List the script or set of "
+"commands to be executed when "
+