Re: Proposed patch for external debugger access

[John Swensen]

John W. Eaton wrote:
> On 29-Oct-2007, John Swensen wrote:
>
> | I simply copied this function from the help.cc file.  Since it was 
> | declared "static" in the help.cc file, I could not access it from 
> | debug.cc.  Is it possible to make this do_which() function available 
> | from outside help.cc?  Then I wouldn't need to replicate it in debug.cc 
> | and any changes to do_which() for the object branch would follow 
> | directly in debug.cc.
>
> If it is the same function, then I don't think there is any reason to
> duplicate it.  We should remove the static qualifier from the function
> in help.cc instead.
>   
Included in the patch
> | >   // Return all breakpoints.  Each element of the map is a vector
> | >   // containing the breakpoints corresponding to a given function name.
> | >
> | > ?  What will happen when we have function overloading and there may be
> | > more than one function with the same name?  Or is this name an
> | > absolute file name?
> | >
> | >   
> | I'm not 100% sure about how to handle this one.  Currently in Matlab, if 
> | you set a breakpoint using dbstop, it will place the breakpoint in 
> | whichever file occurs first in the search path.  In fact, if you try to 
> | add a breakpoint via their editor, it asks whether you want to add the 
> | directory to the top of the search path or change the execution path to 
> | the directory containing the file.  I'm not exactly sure how the 
> | function overloading will work, so I guess I can't comment yet.  When a 
> | user calls 's=dbstatus()' it will return the full path of the file in 
> | which the breakpoint was placed in one of the structure fields.
>
> OK.
>
> | +intmap
> | +bp_table::add_breakpoint (std::string fname, 
> | +			  const intmap& line)
> | +{
> | +  intmap& linenc = const_cast<intmap&>(line);
>
> Why are you casting away const here?  It seems the only use of linenc
> is on the RHS of an assignment:
>
> | +       int lineno = linenc[i];
>
> so it seems that it should be OK to declare it const here.  Oh, intmap
> is std::map<int,int> and there is no const operator [] method (because
> the std::map operator [] always inserts an entry for a missing key) so
> I think you need to use something like
>
>   std::map<int, int>::const_iterator p = line.seek (i);
>
>   if (p != line.end ())
>     {
>       int lineno = p->second;
>       ...
>     }
>   else
>     // Handle case of missing index...
>
> BTW, I think the behavior would be unpredictible if you cast away
> const and the operator [] method actually did insert something.
>
>   
Fixed this.
> | +intmap
> | +bp_table::remove_all_breakpoints_in_file (std::string fname)
>
> This should be "const std::string& fname".
>
>   
Fixed several of these.
> | +      Cell namesCell (dim_vector (bp_list.size (), 1));
> | +      Cell fileCell (dim_vector (bp_list.size (), 1));
> | +      Cell lineCell (dim_vector (bp_list.size (), 1));
>
> I usually find that it is better to avoid including the data type in
> the variable name.
>
>   
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.
> | +class bp_table
> | +{
> | +public:
> |
> | [...]
> |
> | +};
> | +
> | +// Table of breakpoints
> | +extern bp_table breakpoints;
>
> Is there only one table of breakpoints?  If so, this should probably
> be a singleton class.
>
>   
Made it a singleton class.
> jwe
>
>   
I fixed up these suggestions also.  The patches are attached.


John Swensen

Index: debug.cc
===================================================================
RCS file: /cvs/octave/src/debug.cc,v
retrieving revision 1.26
diff -r1.26 debug.cc
22d21
< 
29a29,30
> #include <set>
> 
32a34
> #include "help.h"
42a45,46
> #include "ov-list.h"
> #include "ov-struct.h"
50a55,59
> #include "debug.h"
> 
> // Initialize the singleton object
> bp_table *bp_table::instance = NULL;
> 
54d62
< 
56c64
< get_user_function (std::string fname = "")
---
> get_user_function (const std::string& fname = "")
74d81
<       
86,102c93,96
< 
< DEFCMD (dbstop, args, ,
<   "-*- texinfo -*-\n\
< @deftypefn {Loadable Function} {rline =} dbstop (func, line, @dots{})\n\
< Set a breakpoint in a function\n\
< @table @code\n\
< @item 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\
< @item 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\
< @end table\n\
< \n\
< The rline returned is the real line that the breakpoint was set at.\n\
< @seealso{dbclear, dbstatus, dbnext}\n\
< @end deftypefn")
---
> static void
> parse_dbfunction_params (const octave_value_list& args, 
>                        std::string& symbol_name, 
>                        intmap& lines)
104c98
<   octave_value retval;
---
>   octave_idx_type len = 0;
107c101,102
<   std::string symbol_name = "";
---
>   int list_idx = 0;
>   symbol_name = std::string ();
109c104,105
<   if (nargin != 1 && args(0).is_string())
---
>   // If we are already in a debugging function
>   if (octave_call_stack::caller_user_function () != NULL)
111c107,113
<       symbol_name = args(0).string_value ();
---
>       idx = 0;
>     }
>   else
>     {
>       symbol_name = args (0).string_value ();
>       if (error_state)
>       return;
115,117c117
<   octave_user_function *dbg_fcn = get_user_function (symbol_name);
< 
<   if (dbg_fcn)
---
>   for (int i = idx; i < nargin; i++ )
119,123c119,123
<       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).numel ();
>     }
125c125,135
<       for (int i = idx; i < nargin; i++)
---
>   lines = intmap();
>   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[list_idx++] = line;
>       }
>       else
127c137,142
<         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++)
129,130c144
<             int line = atoi (args(i).string_value ().c_str ());
< 
---
>             int line = static_cast<int> (arg.elem (j));
132a147,154
>             lines[list_idx++] = line;
>           }
>         
>         if (error_state)
>           break;
>       }
>     } 
> }
134,138c156,161
<             if (nr == nsize)
<               {
<                 nsize *= 2;
<                 results.resize (nsize);
<               }
---
> intmap
> bp_table::add_breakpoint (const std::string& fname, 
>                         const intmap& line)
> {
>   if (!instance_ok ())
>     return intmap();
140,142c163,173
<             results(nr++) = cmds->set_breakpoint (line);
<           }
<         else
---
>   octave_idx_type len = line.size ();
>   intmap retval;
>   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++)
>       {
>         intmap::const_iterator p = line.find (i);
>         if (p != line.end ())
144c175,183
<             const NDArray arg = args(i).array_value ();
---
>             int lineno = p->second;
>             retval[i] = cmds->set_breakpoint (lineno);
>             if (retval[i] != 0)
>               instance->bp_map[fname] = dbg_fcn;
>           }
>       }
>     }
>   else
>     error ("add_breakpoint: unable to find the function requested\n");
146,147c185,186
<             if (error_state)
<               break;
---
>   return retval;
> }
149,151d187
<             for (octave_idx_type j = 0; j < arg.nelem(); j++)
<               {
<                 int line = static_cast<int> (arg.elem (j));
153,154c189,194
<                 if (error_state)
<                   break;
---
> int 
> bp_table::remove_breakpoint (const std::string& fname, 
>                            const intmap& line)
> {
>   if (!instance_ok ())
>     return 0;
156,160c196,197
<                 if (nr == nsize)
<                   {
<                     nsize *= 2;
<                     results.resize (nsize);
<                   }
---
>   octave_idx_type len = line.size ();
>   int retval = 0;
162c199,216
<                 results(nr++) = cmds->set_breakpoint (line);
---
>   if (len == 0)
>     {
>       intmap results = remove_all_breakpoints_in_file (fname);
>       retval = results.size ();
>     }
>   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++)
>           {
>             intmap::const_iterator p = line.find (i);
>             if (p != line.end ())
>               {
>                 int lineno = p->second;
>                 cmds->delete_breakpoint (lineno);
164,166d217
< 
<             if (error_state)
<               break;
167a219,222
>         octave_value_list results = cmds->list_breakpoints ();
>         if (results.length () == 0)
>           instance->bp_map.erase (instance->bp_map.find (fname));
>         retval = results.length ();
168a224,235
>       else
>       error ("remove_breakpoint: unable to find the function requested\n");
>     }
>   return retval;
> }
> 
> 
> intmap
> bp_table::remove_all_breakpoints_in_file (const std::string& fname)
> {
>   if (!instance_ok ())
>     return intmap();
170c237,245
<       if (! error_state)
---
>   octave_value_list bkpts;
>   intmap 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++)
172,173c247,249
<         results.resize (nr);
<         retval = results;
---
>         int lineno = static_cast<int> (bkpts (i).int_value ());
>         cmds->delete_breakpoint (lineno);
>         retval[i] = lineno;
174a251
>       instance->bp_map.erase (instance->bp_map.find (fname));
177c254,316
<     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)
> {
>   if (!instance_ok ())
>     return;
> 
>   std::map< std::string, octave_user_function* >::iterator it;
>   for (it = instance->bp_map.begin (); it != instance->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, intmap> 
> bp_table::get_breakpoint_list (const octave_value_list& fname_list)
> {
>   std::map<std::string, intmap> retval;
> 
>   if (!instance_ok ())
>     return 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 = instance->bp_map.begin (); it != instance->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 (); 
>         intmap bkpts_vec;
>         for (int i = 0; i < len; i++)
>           bkpts_vec[i] = bkpts (i).double_value ();
>         retval[ it->first ] = bkpts_vec;
>       }
>     }
>   return retval;
> }
178a318,332
> static octave_value
> intmap_to_ov (const intmap& line) 
> {
>   int idx = 0;
>   NDArray retval (dim_vector (1, line.size()));
>   for (int i = 0; i < line.size(); i++ )
>     {
>       intmap::const_iterator p = line.find (i);
>       if (p != line.end ())
>       {
>         int lineno = p->second;
>         retval (idx++) = lineno;
>       }
>     }
>   retval.resize (dim_vector (1, idx));
181a336,363
> DEFCMD (dbstop, args, ,
>   "-*- texinfo -*-\n\
> @deftypefn {Loadable Function} {rline =} dbstop (func, line, @dots{})\n\
> Set a breakpoint in a function\n\
> @table @code\n\
> @item 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\
> @item 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\
> @end table\n\
> \n\
> The rline returned is the real line that the breakpoint was set at.\n\
> @seealso{dbclear, dbstatus, dbnext}\n\
> @end deftypefn")
> {
>   intmap retval;
>   std::string symbol_name = "";
>   intmap lines;
>   parse_dbfunction_params (args, symbol_name, lines);
> 
>   if (!error_state)
>     retval = bp_table::add_breakpoint (symbol_name, lines);
> 
>   return intmap_to_ov(retval);
> }
> 
200,201d381
<   int nargin = args.length ();
<   int idx = 0;
203,250c383,387
< 
<   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");
---
>   intmap lines;
>   parse_dbfunction_params (args, symbol_name, lines);
>       
>   if (!error_state)
>     bp_table::remove_breakpoint (symbol_name, lines);
255c392
< DEFCMD (dbstatus, args, ,
---
> DEFCMD (dbstatus, args, nargout,
268,269c405
<   octave_value retval;
< 
---
>   Octave_map retval;
270a407,409
>   octave_value_list fcn_list;
>   std::map< std::string, intmap> bp_list;
>   std::string symbol_name = "";
275c414
<       return retval;
---
>       return octave_value ();
278,279d416
<   std::string symbol_name = "";
< 
283c420,424
<       symbol_name = args(0).string_value ();
---
>       {
>         symbol_name = args (0).string_value ();
>         fcn_list (0) = symbol_name;
>         bp_list = bp_table::get_breakpoint_list (fcn_list);
>       }
285c426
<       gripe_wrong_type_arg ("dbstatus", args(0));
---
>       gripe_wrong_type_arg ("dbstatus", args (0));
287,290c428
< 
<   octave_user_function *dbg_fcn = get_user_function (symbol_name);
< 
<   if (dbg_fcn)
---
>   else
292,298c430,447
<       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 = bp_table::get_breakpoint_list (fcn_list);
>     }
> 
>   std::map< std::string, intmap>::iterator it;
>   if (nargout == 1)
>     {
>       // Fill in an array for return
>       int i = 0;
>       Cell names (dim_vector (bp_list.size (), 1));
>       Cell file (dim_vector (bp_list.size (), 1));
>       Cell line (dim_vector (bp_list.size (), 1));
>       for (it = bp_list.begin (); it != bp_list.end (); it++)
300,303c449,452
<         vec(i) = lst(i).double_value ();
< 
<         if (error_state)
<           panic_impossible ();
---
>         names (i) = it->first;
>         line (i) = intmap_to_ov(it->second);
>         file (i)  = do_which (it->first);
>         i++;
305,306c454,457
< 
<       retval = octave_value (vec);
---
>       retval.assign ("name", names);
>       retval.assign ("file", file);
>       retval.assign ("line", line);
>       return octave_value (retval);
309,311c460,472
<     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.size (); j++)
>           if (j < it->second.size()-1)
>             octave_stdout << it->second [j] << ", ";
>           else
>             octave_stdout << it->second [j] << "." << std::endl;
>       }
>       return octave_value ();
>     }
Index: help.h
===================================================================
RCS file: /cvs/octave/src/help.h,v
retrieving revision 1.27
diff -r1.27 help.h
51a52,53
> extern std::string do_which (const std::string& name);
> 
Index: help.cc
===================================================================
RCS file: /cvs/octave/src/help.cc,v
retrieving revision 1.175
diff -r1.175 help.cc
1298c1298
< static std::string
---
> std::string

--- /dev/null   2007-10-29 22:54:34.000000000 -0400
+++ debug.h     2007-10-29 22:57:32.000000000 -0400
@@ -0,0 +1,101 @@
+/*
+
+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/>.
+
+*/
+
+#if !defined (octave_debug_h)
+#define octave_debug_h 1
+
+#include <map>
+#include "ov.h"
+#include "dRowVector.h"
+
+//class RowVector;
+class octave_value_list;
+class octave_user_function;
+
+typedef std::map<int, int> intmap;
+
+// A singleton 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
+{
+private:
+
+  bp_table (void) {};
+
+  ~bp_table (void) {};
+
+public:
+
+  static bool instance_ok (void)
+  {
+    bool retval = true;
+
+    if (! instance)
+      instance = new bp_table ();
+
+    if (! instance)
+      {
+        ::error ("unable to create breakpoint table!");
+        retval = false;
+      }
+    
+    return retval;
+  }
+
+  // Add a breakpoint at the nearest executable line.
+  static intmap add_breakpoint (const std::string& fname = "", 
+                        const intmap& lines = intmap());
+
+  // Remove a breakpoint from a line in file.
+  static int remove_breakpoint (const std::string& fname = "", 
+                        const intmap& lines = intmap());
+
+  // Remove all the breakpoints in a specified file.
+  static intmap remove_all_breakpoints_in_file (const std::string& fname);
+  
+  // Remove all the breakpoints registered with octave.
+  static void remove_all_breakpoints (void);
+  
+  // Return all breakpoints.  Each element of the map is a vector
+  // containing the breakpoints corresponding to a given function name.
+  static std::map <std::string, intmap> 
+  get_breakpoint_list (const octave_value_list& fname_list);
+
+private:
+
+  // Map from function names to function objects for functions
+  // containing at least one breakpoint.
+  std::map<std::string, octave_user_function*> bp_map;
+
+  // Singleton instance
+  static bp_table *instance;
+};
+
+
+#endif
+
+/*
+;;; Local Variables: ***
+;;; mode: C++ ***
+;;; End: ***
+*/