Add a --all command option to "target delete"

[zturner]

Summary

Fairly self explanatory. The immediate use case for this is to add code to lldbtest teardown that deletes all targets before running make clean, to deal with file locking issues on Windows. But the option is generally useful independently of the test suite anyway.

I made the --all option not require an argument (so you just use --all instead of --all true). For consistency, I changed the --clean flag to work the same way by changing the last argument being passed to the constructor of m_clean_option from false to true. I don't think this is a big issue since we don't guarantee stability of the CLI and this is not a heavily used option, but worth calling out anyway.

[jingham]

I think it is worth mentioning in the help that if you do "clean" then you will need to reparse all shared modules the next time you debug. This can be a significant hiccup when you next debug... Can't think of a succinct way to say this off the top of my head, though...

I try to use boolean values only when it isn't clear what the default value is (for instance if it is also controlled by a setting, or maybe situationally determined.) That's not true for --all or --clean so it is fine to have them be argument-less flags.

And while I would be concerned about changing commonly used CLI options since that will make aliases to fail, these are not common options, so it is okay to clean up the clean option.

[zturner]

Seems reasonable, I can add something to the help. What do you think about also changing the semantics of --clean to clear the module list regardless of if the modules are orphaned? I've spent the past 2 days trying to track down a refcounting bug related to the shared module list that only happens when running from the test suite, even if I run the exact same sequence of commands from inside LLDB. But the gist of it is that when running from the test suite, when RemoveOrphanSharedModules() is called from the CommandObject, the ModuleSP for the executable has a ref count of 2, so it isn't unique to the shared module list, and it doesn't get removed.

I tested to see what happens if I change this logic to remove from the module list regardless of whether it's unique, and I can't find any unintended side effects other than lots of tests start passing :)

shared_ptrs are shared, so in theory there shouldn't be any negative consequences to just removing it. Whoever still has a reference can still continue to use it. Alternatively I can add a --force command line option, so that the semantics of specifying just --clean are unchanged.

Thoughts?

[zturner]

Ironically even though I've been staring at this problem for 2 days I think
I actually figured this out shortly after my last email.
TestBase.tearDown() already deletes all the targets but it was calling
Base.tearDown before doing TestBase.tearDown(). So the setup looked like
this:

def setup(self):

Base.setup(self)

# Do TestBase specific stuff

def tearDown(self):

Base.tearDown(self)

# Do TestBase specific stuff

the orders here need to be reversed to ensure that teardown happens in
reverse order from setup. In particular Base.tearDown() was calling the
tear down hooks, one of which in this case was to call make clean. So make
clean was being called before the targets were deleted. My clean --all
helped, but as you said, the SB stuff was still holding a reference
somewhere.

I fixed this by reversing the orders and also fixed about 20 other tests in
the process. Yay for progress.

source/Commands/CommandObjectTarget.cpp

                              "Delete one or more targets by target index.",
                              NULL,
                              0),
-        m_option_group (interpreter),
-        m_cleanup_option (LLDB_OPT_SET_1, false, "clean", 'c', "Perform extra cleanup to minimize memory consumption after deleting the target.", false, false)
+        m_option_group(interpreter),
+        m_all_option(LLDB_OPT_SET_1, false, "all", 'a', "Delete all targets.", false, true),
+        m_cleanup_option(LLDB_OPT_SET_1, false, "clean", 'c', "Perform extra cleanup to minimize memory consumption after deleting the target.", false, true)
     {
-        m_option_group.Append (&m_cleanup_option, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
+        m_option_group.Append(&m_all_option, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
+        m_option_group.Append(&m_cleanup_option, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
         m_option_group.Finalize();
     }
 

         const size_t argc = args.GetArgumentCount();
         std::vector<TargetSP> delete_target_list;
         TargetList &target_list = m_interpreter.GetDebugger().GetTargetList();
-        bool success = true;
         TargetSP target_sp;
-        if (argc > 0)
+
+        if (m_all_option.GetOptionValue())
+        {
+            for (uint32_t i = 0; i < target_list.GetNumTargets(); ++i)
+                delete_target_list.push_back(target_list.GetTargetAtIndex(i));
+        }
+        else if (argc > 0)
         {
             const uint32_t num_targets = target_list.GetNumTargets();
             // Bail out if don't have any targets.
             if (num_targets == 0) {
                 result.AppendError("no targets to delete");
                 result.SetStatus(eReturnStatusFailed);
-                success = false;
+                return false;
             }
 
-            for (uint32_t arg_idx = 0; success && arg_idx < argc; ++arg_idx)
+            for (uint32_t arg_idx = 0; arg_idx < argc; ++arg_idx)
             {
                 const char *target_idx_arg = args.GetArgumentAtIndex(arg_idx);
+                bool success = false;
                 uint32_t target_idx = StringConvert::ToUInt32 (target_idx_arg, UINT32_MAX, 0, &success);
-                if (success)
+                if (!success)
                 {
-                    if (target_idx < num_targets)
-                    {
-                        target_sp = target_list.GetTargetAtIndex (target_idx);
-                        if (target_sp)
-                        {
-                            delete_target_list.push_back (target_sp);
-                            continue;
-                        }
-                    }
-                    if (num_targets > 1)
-                        result.AppendErrorWithFormat ("target index %u is out of range, valid target indexes are 0 - %u\n",
-                                                      target_idx,
-                                                      num_targets - 1);
-                    else
-                        result.AppendErrorWithFormat("target index %u is out of range, the only valid index is 0\n",
-                                                    target_idx);
-
+                    result.AppendErrorWithFormat("invalid target index '%s'\n", target_idx_arg);
                     result.SetStatus (eReturnStatusFailed);
-                    success = false;
+                    return false;
                 }
-                else
+                if (target_idx < num_targets)
                 {
-                    result.AppendErrorWithFormat("invalid target index '%s'\n", target_idx_arg);
-                    result.SetStatus (eReturnStatusFailed);
-                    success = false;
+                    target_sp = target_list.GetTargetAtIndex (target_idx);
+                    if (target_sp)
+                    {
+                        delete_target_list.push_back (target_sp);
+                        continue;
+                    }
                 }
+                if (num_targets > 1)
+                    result.AppendErrorWithFormat ("target index %u is out of range, valid target indexes are 0 - %u\n",
+                                                    target_idx,
+                                                    num_targets - 1);
+                else
+                    result.AppendErrorWithFormat("target index %u is out of range, the only valid index is 0\n",
+                                                target_idx);
+
+                result.SetStatus (eReturnStatusFailed);
+                return false;
             }
         }
         else
         {
             target_sp = target_list.GetSelectedTarget();
-            if (target_sp)
-            {
-                delete_target_list.push_back (target_sp);
-            }
-            else
+            if (!target_sp)
             {
                 result.AppendErrorWithFormat("no target is currently selected\n");
                 result.SetStatus (eReturnStatusFailed);
-                success = false;
+                return false;
             }
+            delete_target_list.push_back (target_sp);
         }
-        if (success)
+
+        const size_t num_targets_to_delete = delete_target_list.size();
+        for (size_t idx = 0; idx < num_targets_to_delete; ++idx)
         {
-            const size_t num_targets_to_delete = delete_target_list.size();
-            for (size_t idx = 0; idx < num_targets_to_delete; ++idx)
-            {
-                target_sp = delete_target_list[idx];
-                target_list.DeleteTarget(target_sp);
-                target_sp->Destroy();
-            }
-            // If "--clean" was specified, prune any orphaned shared modules from
-            // the global shared module list
-            if (m_cleanup_option.GetOptionValue ())
-            {
-                const bool mandatory = true;
-                ModuleList::RemoveOrphanSharedModules(mandatory);
-            }
-            result.GetOutputStream().Printf("%u targets deleted.\n", (uint32_t)num_targets_to_delete);
-            result.SetStatus(eReturnStatusSuccessFinishResult);
+            target_sp = delete_target_list[idx];
+            target_list.DeleteTarget(target_sp);
+            target_sp->Destroy();
+        }
+        // If "--clean" was specified, prune any orphaned shared modules from
+        // the global shared module list
+        if (m_cleanup_option.GetOptionValue ())
+        {
+            const bool mandatory = true;
+            ModuleList::RemoveOrphanSharedModules(mandatory);
         }
+        result.GetOutputStream().Printf("%u targets deleted.\n", (uint32_t)num_targets_to_delete);
+        result.SetStatus(eReturnStatusSuccessFinishResult);
 
-        return result.Succeeded();
+        return true;
     }
 
     OptionGroupOptions m_option_group;
+    OptionGroupBoolean m_all_option;
     OptionGroupBoolean m_cleanup_option;
 };