Re: Proposed patch for external debugger access

[John Swensen]

John W. Eaton wrote:
> On 23-Oct-2007, John Swensen wrote:
>
> | I have attached a patch to address some of the issues with IDE's that 
> | are going to want access to debugger information.
>
> I'll try to evaluate this patch before 2.9.16/3.0, but it would help
> if you could address the following comments and submit the patch
> again.
>
> | P.S. JWE, I still have the hardest time with your desired paren spacing 
> | ;)  I do see your argument about it being more like the English 
> | language, it is just that I am so used to doing it the other way that it 
> | still seems awkward in terms of typing where I don't have that muscle 
> | memory yet and I actually have to pause and force myself to do it.  
> | However, I do think I caught all of them in this patch.
>
> Please search for "( ", " )", and " ;" and fix those.
>   
Done.
> | +/**
> | + * A class to provide a standard interface to breakpoints, even if the 
> associated backend
> | + * changes, we can make sure this higher level interface stays the same.
> | + */
>
> For consistency with most of the rest of Octave, please use C++ style
> comments ("// comment").
>   
Done.
> | +class bp_table
> | +{
> | +public:
> | +  /**
> | +   * Default constructor
> | +   */
> | +  bp_table ();
>
> To match the style of the rest of Octave, please use (void) instead of
> just () for functions that don't take arguments.
>
>   
Done.
> | +  /**
> | +   * Adds a breakpoint at the nearest executable line.
> | +   *
> | +   * @param fname The name of the m-file in which to set the breakpoint
> | +   * @param line The line number on which to set the breakpoint.
> | +   *
> | +   * @return Returns the line number at which the breakpoint was
> | +   * actually placed. This is only different than the input if the
> | +   * input was not an executable line.
> | +   */
>
> What is the @param/@return for?  Are those doxygen markers?  I'm not
> really convinced we need that for Octave, and it doesn't match the
> style of the rest of the code.
>
>   
Done.
> | +  octave_value_list add_breakpoint (std::string fname = "", 
> octave_value_list lines = octave_value_list());
>
> Please try to avoid long lines like this.  Wrap them between
> arguments if possible.
>
>   
Done.
> | +  /**
> | +   * Returns a list of breakpoints in the system.  In the absence of the 
> parameter, all
> | +   * breakpoints are returned.  Using the parameter, a subset of the 
> breakpoints can be
> | +   * queried.
> | +   *
> | +   * @param fname_list A list of function names for which to check for 
> breakpoints
> | +   *
> | +   * @return A map of the pairwise <file, linenumber>
> | +   */
> | +  std::map< std::string, octave_value_list > get_breakpoint_list ( 
> octave_value_list fname_list );
>
> Please write
>
>   std::map <std::string, octave_value_list>
>   get_breakpoint_list (octave_value_list fname_list);
>
>   
Done.
> | -Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007 Ben Sapp
> | +Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007 Ben Sapp,
> | +              2007 John Swensen
>
> I think it would be best to just put
>
>   Copyright (C) 2007 John Swensen
>
> on a separate line.
>
>   
Just removed my copyright altogether.
> | @@ -19,39 +20,14 @@
> |  <http://www.gnu.org/licenses/>.
> |  
> |  */
> | +#include "debug.h"
>
> This should be in the list of other local includes, and definitely
> after config.h, which should always be the first include file.
>
> Oh, I see you moved all the includes to debug.h.  That's not really a
> good thing to do.  A header like debug.h should only include the
> minimum necessary for the header file itself, not everything for the
> implementation of the class that is in the .cc file.
>
>   
Done.  The debug.h file only needed to include <map> and "ov.h" with a 
couple of other forward declarations.
> | 
> +//-----------------------------------------------------------------------------------
>
> I don't think other files in Octave have these kinds of markers, and
> I'd prefer not to introduce them.  Why do you want them>
>
>   
Removed.
> | +bp_table::bp_table ()
> | +{
> | + 
> | +}
>
> If a function is empty, it's definition can certainly go in the
> declaration in the header file.
>
>   
Done.
> | +       cmds->delete_breakpoint ( static_cast<int> (line(i).matrix_value 
> ()(0)));
>
> Always repeating line(i).matrix_value()(0) looks clumsy, so I think it
> would be good to encapsulate it in another function.  Also, is it
> possible for line(i) to be something other than a matrix object?  Is
> it possible for it to be empty?  If so, then I think this could cause
> trouble.
>
>   
Changing to RowVectors to store the breakpoints, rather than 
octave_value_list objects, solved this almost everywhere.  As to whether 
this can be empty and causing problems, the for loop in which this is 
called will take care of ensuring that this will not be called improperly.
> Sorry if some of these things seem silly, but it helps make the whole
> of Octave easier to read if it follows a consistent style, so I will
> change the style when I check in the changes, and if I'm doing that
> it just slows things down.
>
> jwe
>
>   
I also added the filename associated with the function and breakpoints 
in the struct returned by 's=dbstatus' (like Matlab).  I still don't 
have the ability to pass the structure resulting from dbstatus into 
dbstop, but I think I have ironed out the problems everywhere else.

John Swensen

Index: debug.cc
===================================================================
RCS file: /cvs/octave/src/debug.cc,v
retrieving revision 1.26
diff -u -r1.26 debug.cc
--- debug.cc    12 Oct 2007 21:27:29 -0000      1.26
+++ debug.cc    24 Oct 2007 23:20:17 -0000
@@ -19,7 +19,6 @@
 <http://www.gnu.org/licenses/>.
 
 */
-
 #ifdef HAVE_CONFIG_H
 #include <config.h>
 #endif
@@ -28,6 +27,7 @@
 #include <fstream>
 #include <string>
 
+#include "debug.h"
 #include "defun.h"
 #include "error.h"
 #include "input.h"
@@ -37,9 +37,9 @@
 #include "parse.h"
 #include "symtab.h"
 #include "gripes.h"
-#include "ov.h"
 #include "ov-usr-fcn.h"
 #include "ov-fcn.h"
+#include "ov-struct.h"
 #include "pt-pr-code.h"
 #include "pt.h"
 #include "pt-bp.h"
@@ -48,10 +48,13 @@
 #include "unwind-prot.h"
 #include "variables.h"
 
+
+// Global variable containing the breakpoint information
+bp_table breakpoints;
+
 // Return a pointer to the user-defined function FNAME.  If FNAME is
 // empty, search backward for the first user-defined function in the
 // current call stack.
-
 static octave_user_function *
 get_user_function (std::string fname = "")
 {
@@ -83,98 +86,241 @@
   return dbg_fcn;
 }
 
-
-DEFCMD (dbstop, args, ,
-  "-*- texinfo -*-\n\
address@hidden {Loadable Function} {rline =} dbstop (func, line, @dots{})\n\
-Set a breakpoint in a function\n\
address@hidden @code\n\
address@hidden func\n\
-String representing the function name.  When already in debug\n\
-mode this should be left out and only the line should be given.\n\
address@hidden line\n\
-Line you would like the breakpoint to be set on. Multiple\n\
-lines might be given as separate arguments or as a vector.\n\
address@hidden table\n\
-\n\
-The rline returned is the real line that the breakpoint was set at.\n\
address@hidden, dbstatus, dbnext}\n\
address@hidden deftypefn")
+static void
+parse_dbfunction_params (octave_value_list args, 
+                        std::string& symbol_name, 
+                        RowVector& lines)
 {
-  octave_value retval;
+  octave_idx_type len = 0;
   int nargin = args.length ();
   int idx = 0;
-  std::string symbol_name = "";
+  int list_idx = 0;
+  symbol_name = std::string ();
 
-  if (nargin != 1 && args(0).is_string())
+  // If we are already in a debugging function
+  if (octave_call_stack::caller_user_function () != NULL)
+    {
+      idx = 0;
+    }
+  else
     {
-      symbol_name = args(0).string_value ();
+      symbol_name = args (0).string_value ();
       idx = 1;
     }
 
-  octave_user_function *dbg_fcn = get_user_function (symbol_name);
-
-  if (dbg_fcn)
+  for (int i = idx; i < nargin; i++ )
     {
-      octave_idx_type nsize = 10;
-      RowVector results (nsize);
-      octave_idx_type nr = 0;
-
-      tree_statement_list *cmds = dbg_fcn->body ();
+      if (args (i).is_string ())
+       len += 1;
+      else
+       len += args (i).array_value ().length (); 
+    }
 
-      for (int i = idx; i < nargin; i++)
+  lines = RowVector (len);
+  for (int i = idx; i < nargin; i++ )
+    {
+      if (args (i).is_string ())
+       {
+         int line = atoi (args (i).string_value ().c_str ());
+         if (error_state)
+             break;
+         lines.xelem (list_idx++) = line;
+       }
+      else
        {
-         if (args(i).is_string ())
+         const NDArray arg = args (i).array_value ();
+         
+         if (error_state)
+           break;
+         
+         for (octave_idx_type j = 0; j < arg.nelem(); j++)
            {
-             int line = atoi (args(i).string_value ().c_str ());
-
+             int line = static_cast<int> (arg.elem (j));
              if (error_state)
                break;
+             lines.xelem (list_idx++) = line;
+           }
+         
+         if (error_state)
+           break;
+       }
+    } 
+}
 
-             if (nr == nsize)
-               {
-                 nsize *= 2;
-                 results.resize (nsize);
-               }
+// copied from help.cc; it is static there so "unavailable" elsewhere
+static std::string
+do_which (const std::string& name)
+{
+  std::string retval;
 
-             results(nr++) = cmds->set_breakpoint (line);
-           }
-         else
-           {
-             const NDArray arg = args(i).array_value ();
+  symbol_record *sym_rec = lookup_by_name (name, 0);
 
-             if (error_state)
-               break;
+  if (sym_rec && sym_rec->is_defined ())
+    retval = sym_rec->which ();
+  else
+    retval = fcn_file_in_path (name);
 
-             for (octave_idx_type j = 0; j < arg.nelem(); j++)
-               {
-                 int line = static_cast<int> (arg.elem (j));
+  return retval;
+}
 
-                 if (error_state)
-                   break;
 
-                 if (nr == nsize)
-                   {
-                     nsize *= 2;
-                     results.resize (nsize);
-                   }
+RowVector bp_table::add_breakpoint (std::string fname, RowVector line)
+{
+  octave_idx_type len = line.length ();
+  RowVector retval (len);
+  octave_user_function *dbg_fcn = get_user_function (fname);
 
-                 results(nr++) = cmds->set_breakpoint (line);
-               }
+  if (dbg_fcn)
+    {
+      tree_statement_list *cmds = dbg_fcn->body ();
+      for (int i = 0; i < len; i++)
+       {
+         int lineno = static_cast<int> (line.xelem (i));
+         retval.xelem (i) = cmds->set_breakpoint (lineno);
+         if (static_cast<int> (retval.xelem (i)) != 0)
+           bp_map[fname] = dbg_fcn;
+       }
+    }
+  else
+    error ("add_breakpoint: unable to find the function requested\n");
 
-             if (error_state)
-               break;
+  return retval;
+}
+
+
+int bp_table::remove_breakpoint (std::string fname, RowVector line)
+{
+  octave_idx_type len = line.length ();
+  int retval = 0;
+
+  if (len == 0)
+    {
+      RowVector results = remove_all_breakpoints_in_file (fname);
+      retval = results.length ();
+    }
+  else
+    {
+      octave_user_function *dbg_fcn = get_user_function (fname);
+      if (dbg_fcn)
+       {
+         tree_statement_list *cmds = dbg_fcn->body ();
+         for (int i = 0; i < len; i++)
+           {
+             int lineno = static_cast<int> (line.xelem (i));
+             cmds->delete_breakpoint (lineno);
            }
+         octave_value_list results = cmds->list_breakpoints ();
+         if (results.length () == 0)
+           bp_map.erase (bp_map.find (fname));
+         retval = results.length ();
        }
+      else
+       error ("remove_breakpoint: unable to find the function requested\n");
+    }
+  return retval;
+}
 
-      if (! error_state)
+
+RowVector bp_table::remove_all_breakpoints_in_file (std::string fname)
+{
+  octave_value_list bkpts;
+  RowVector retval;
+  octave_user_function *dbg_fcn = get_user_function (fname);
+  
+  if (dbg_fcn)
+    {
+      tree_statement_list *cmds = dbg_fcn->body ();
+      bkpts = cmds->list_breakpoints ();
+      for (int i = 0; i < bkpts.length (); i++)
        {
-         results.resize (nr);
-         retval = results;
+         int lineno = static_cast<int> (bkpts (i).matrix_value ()(0));
+         cmds->delete_breakpoint (lineno);
+         retval(i) = lineno;
        }
+      bp_map.erase (bp_map.find (fname));
     }
   else
-    error ("dbstop: unable to find the function requested\n");
+    error ("remove_all_breakpoint_in_file: "
+          "unable to find the function requested\n");
+
+  return retval;
+}
+
+
+void bp_table::remove_all_breakpoints (void)
+{
+  std::map< std::string, octave_user_function* >::iterator it;
+  for (it = bp_map.begin (); it != bp_map.end (); it++)
+    {
+      remove_all_breakpoints_in_file ((*it).first);
+    }
+}
+
+std::string 
+do_find_bkpt_list (octave_value_list slist, 
+                  std::string match)
+{
+  std::string retval;
+  for (int i = 0; i < slist.length (); i++)
+    {
+      if (slist (i).string_value () == match)
+       {
+         retval = slist (i).string_value ();
+         break;
+       }
+    }
+  return retval;
+}
+
+
+std::map< std::string, RowVector > 
+bp_table::get_breakpoint_list (octave_value_list fname_list)
+{
+  std::map<std::string, RowVector> retval;
+
+  // Iterate through each of the files in the map and get the 
+  // name and list of breakpoints
+  std::map< std::string, octave_user_function* >::iterator it;
+  for (it = bp_map.begin (); it != bp_map.end (); it++)
+    {
+      if (fname_list.length () == 0 || 
+         do_find_bkpt_list (fname_list, (*it).first) != "")
+       {
+         octave_value_list bkpts = (*it).second->body ()->list_breakpoints ();
+         octave_idx_type len = bkpts.length (); 
+         RowVector bkpts_vec (len);
+         for (int i = 0; i < len; i++)
+           bkpts_vec.xelem (i) = bkpts (i).double_value ();
+         retval[ (*it).first ] = bkpts_vec;
+       }
+    }
+  return retval;
+}
+
+DEFCMD (dbstop, args, ,
+  "-*- texinfo -*-\n\
address@hidden {Loadable Function} {rline =} dbstop (func, line, @dots{})\n\
+Set a breakpoint in a function\n\
address@hidden @code\n\
address@hidden func\n\
+String representing the function name.  When already in debug\n\
+mode this should be left out and only the line should be given.\n\
address@hidden line\n\
+Line you would like the breakpoint to be set on. Multiple\n\
+lines might be given as separate arguments or as a vector.\n\
address@hidden table\n\
+\n\
+The rline returned is the real line that the breakpoint was set at.\n\
address@hidden, dbstatus, dbnext}\n\
address@hidden deftypefn")
+{
+  octave_value retval;
+  std::string symbol_name = "";
+  RowVector lines;
+  parse_dbfunction_params (args, symbol_name, lines);
+
+  if (!error_state)
+    retval = breakpoints.add_breakpoint (symbol_name, lines);
 
   return retval;
 }
@@ -197,62 +343,17 @@
 @end deftypefn")
 {
   octave_value retval;
-  int nargin = args.length ();
-  int idx = 0;
   std::string symbol_name = "";
-
-  if (nargin != 1 && args(0).is_string())
-    {
-      symbol_name = args(0).string_value ();
-      idx = 1;
-    }
-
-  octave_user_function *dbg_fcn = get_user_function (symbol_name);
-
-  if (dbg_fcn)
-    {
-      tree_statement_list *cmds = dbg_fcn->body ();
-
-      for (int i = idx; i < nargin; i++)
-       {
-         if (args(i).is_string ())
-           {
-             int line = atoi (args(i).string_value ().c_str ());
-
-             if (error_state)
-               break;
-
-             cmds->delete_breakpoint (line);
-           }
-         else
-           {
-             const NDArray arg = args(i).array_value ();
-
-             if (error_state)
-               break;
-
-             for (octave_idx_type j = 0; j < arg.nelem (); j++)
-               {
-                 int line = static_cast<int> (arg.elem (j));
-
-                 if (error_state)
-                   break;
-
-                 cmds->delete_breakpoint (line);
-               }
-
-             if (error_state)
-               break;
-           }
-       }
-    }
-  else
-    error ("dbclear: unable to find the function requested\n");
-
+  RowVector lines;
+  parse_dbfunction_params (args, symbol_name, lines);
+      
+  if (!error_state)
+    breakpoints.remove_breakpoint (symbol_name, lines);
+      
   return retval;
 }
 
-DEFCMD (dbstatus, args, ,
+DEFCMD (dbstatus, args, nargout,
   "-*- texinfo -*-\n\
 @deftypefn {Loadable Function} {lst =} dbstatus ([func])\n\
 Return a vector containing the lines on which a function has \n\
@@ -265,50 +366,74 @@
 @seealso{dbclear, dbwhere}\n\
 @end deftypefn")
 {
-  octave_value retval;
-
+  Octave_map retval;
   int nargin = args.length ();
+  octave_value_list fcn_list;
+  std::map< std::string, RowVector > bp_list;
+  std::string symbol_name = "";
 
   if (nargin != 0 && nargin != 1)
     {
       error ("dbstatus: only zero or one arguements accepted\n");
-      return retval;
+      return octave_value ();
     }
 
-  std::string symbol_name = "";
-
   if (nargin == 1)
     {
       if (args(0).is_string ())
-       symbol_name = args(0).string_value ();
+       {
+         symbol_name = args (0).string_value ();
+         fcn_list (0) = symbol_name;
+         bp_list = breakpoints.get_breakpoint_list (fcn_list);
+       }
       else
-       gripe_wrong_type_arg ("dbstatus", args(0));
+       gripe_wrong_type_arg ("dbstatus", args (0));
     }
-
-  octave_user_function *dbg_fcn = get_user_function (symbol_name);
-
-  if (dbg_fcn)
+  else
     {
-      tree_statement_list *cmds = dbg_fcn->body ();
-
-      octave_value_list lst = cmds->list_breakpoints ();
-
-      RowVector vec (lst.length (), 0.0);
-
-      for (int i = 0; i < lst.length (); i++)
+       octave_user_function *dbg_fcn = get_user_function ();
+       if (dbg_fcn)
+        {
+          symbol_name = dbg_fcn->name ();
+          fcn_list (0) = symbol_name;
+        }
+       bp_list = breakpoints.get_breakpoint_list (fcn_list);
+    }
+
+  std::map< std::string, RowVector >::iterator it;
+  if (nargout == 1)
+    {
+      // Fill in an array for return
+      int i = 0;
+      Cell namesCell (dim_vector (bp_list.size (), 1));
+      Cell fileCell (dim_vector (bp_list.size (), 1));
+      Cell lineCell (dim_vector (bp_list.size (), 1));
+      for (it = bp_list.begin (); it != bp_list.end (); it++)
        {
-         vec(i) = lst(i).double_value ();
-
-         if (error_state)
-           panic_impossible ();
+         namesCell (i) = (*it).first;
+         lineCell (i)  = (*it).second;
+         fileCell (i)  = do_which ((*it).first);
+         i++;
        }
-
-      retval = octave_value (vec);
+      retval.assign ("name", namesCell);
+      retval.assign ("file", fileCell);
+      retval.assign ("line", lineCell);
+      return octave_value (retval);
     }
   else
-    error ("dbstatus: unable to find the function you requested\n");
-
-  return retval;
+    {
+      // Print out the breakpoint information
+      for (it = bp_list.begin(); it != bp_list.end(); it++)
+       {         
+         octave_stdout << "Breakpoint in " << (*it).first << " at line(s) ";
+         for (int j = 0; j < (*it).second.length (); j++)
+           if (j < (*it).second.length()-1)
+             octave_stdout << (*it).second.xelem (j) << ", ";
+           else
+             octave_stdout << (*it).second.xelem (j) << "." << std::endl;
+       }
+      return octave_value ();
+    }
 }
 
 DEFCMD (dbwhere, , ,

--- /dev/null   2007-10-24 19:20:16.000000000 -0400
+++ debug.h     2007-10-24 19:08:03.000000000 -0400
@@ -0,0 +1,75 @@
+/*
+
+Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007 Ben Sapp
+
+This file is part of Octave.
+
+Octave is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by the
+Free Software Foundation; either version 3 of the License, or (at your
+option) any later version.
+
+Octave is distributed in the hope that it will be useful, but WITHOUT
+ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with Octave; see the file COPYING.  If not, see
+<http://www.gnu.org/licenses/>.
+
+*/
+
+#include <map>
+#include "ov.h"
+
+class RowVector;
+class octave_user_function;
+
+// A class to provide a standard interface to breakpoints, even if the 
+// associated backend changes, we can make sure this higher level 
+// interface stays the same.
+class bp_table
+{
+public:
+
+  // Default constructor
+  bp_table (void) {};
+
+  // Destructor
+  ~bp_table (void) {};
+
+  // Adds a breakpoint at the nearest executable line.
+  RowVector add_breakpoint (std::string fname = "", 
+                           RowVector lines = RowVector ());
+
+  // Remove a breakpoint from a line in file.
+  int remove_breakpoint (std::string fname = "", 
+                        RowVector lines = RowVector ());
+
+  //Removes all the breakpoints in a specified file.
+  RowVector remove_all_breakpoints_in_file (std::string fname);
+  
+  // Removes all the breakpoints registered with octave.
+  void remove_all_breakpoints (void);
+  
+  // Returns a list of breakpoints in the system.  In the absence of 
+  // the parameter, all breakpoints in the system are returned.
+  std::map <std::string, RowVector> 
+  get_breakpoint_list (octave_value_list fname_list);
+
+private:
+
+  // The STL map containing a list of the pairwise 
+  // <filename, octave_user_function*> for every function that 
+  // contains at least one breakpoint.
+  std::map< std::string, octave_user_function* > bp_map;
+};
+
+extern bp_table breakpoints;
+
+/*
+;;; Local Variables: ***
+;;; mode: C++ ***
+;;; End: ***
+*/