From 75f5963ff4dbd98d803d24300146c44ec82ffbdd Mon Sep 17 00:00:00 2001
From: paulo <paulo.fernando.silva@gmail.com>
Date: Sun, 11 Jun 2023 11:08:05 +0900
Subject: [PATCH] Add support for classdef breakpoints (bug #46451)

* libinterp/octave-value/cdef-class.cc: Add
cdef_class::cdef_class_rep::get_method (int line) const.

* libinterp/parse-tree/bp-table.cc: Add user_code_provider class to enable
setting breakpoints and using the dbstatus and dbclear commands within classdef
classes.

* libinterp/octave-value/cdef-class.h: Add OCTINTERP_API octave_value get_method
(int line) const.

* test/classdef-debug/breakpoints/classdef_breakpoints.m: New m-file containing
a simple classdef class for use in testing the new functionality.

* test/classdef-debug/breakpoints/test_classdef_breakpoints.m: New script for
testing breakpoints functionality.
---
 libinterp/octave-value/cdef-class.cc          |  33 +++
 libinterp/octave-value/cdef-class.h           |   7 +
 libinterp/parse-tree/bp-table.cc              | 255 ++++++++++++++----
 test/Makefile.am                              |   1 +
 test/classdef-debug/classdef_breakpoints.m    |  21 ++
 test/classdef-debug/module.mk                 |   5 +
 .../test_classdef_breakpoints.tst             |  31 +++
 7 files changed, 301 insertions(+), 52 deletions(-)
 create mode 100644 test/classdef-debug/classdef_breakpoints.m
 create mode 100644 test/classdef-debug/module.mk
 create mode 100644 test/classdef-debug/test_classdef_breakpoints.tst

diff --git a/libinterp/octave-value/cdef-class.cc b/libinterp/octave-value/cdef-class.cc
index 57609e8834..6cfd9b148a 100644
--- a/libinterp/octave-value/cdef-class.cc
+++ b/libinterp/octave-value/cdef-class.cc
@@ -708,6 +708,39 @@ cdef_class::cdef_class_rep::get_method (const std::string& name) const
   return p->second.get_function ();
 }
 
+octave_value
+cdef_class::cdef_class_rep::get_method (int line) const
+{
+  octave_value closest_match;
+  int closest_match_end_line = std::numeric_limits<int>::max ();
+  // Since we have a dynamic cast performance could be an issue
+  // If this is called from a critical path.
+  // If performance is an issue we can cache an ordered version of the method map
+  for (auto i = m_method_map.cbegin (); i != m_method_map.cend (); ++i)
+    {
+      const octave_value& fcn = i->second.get_function ();
+      octave_user_code *user_code = fcn.user_code_value ();
+
+      if (user_code == nullptr)
+        continue;
+
+      octave_user_function* pfcn
+        = dynamic_cast<octave_user_function*> (user_code);
+
+      if (pfcn == nullptr)
+        continue;
+
+      const int e = pfcn->ending_line ();
+      if (line <= e && e <= closest_match_end_line && pfcn->is_defined ()
+          && pfcn->is_user_code ())
+        {
+          closest_match = fcn;
+          closest_match_end_line = e;
+        }
+    }
+
+  return closest_match;
+}
 
 octave_value
 cdef_class::cdef_class_rep::construct (const octave_value_list& args)
diff --git a/libinterp/octave-value/cdef-class.h b/libinterp/octave-value/cdef-class.h
index bec780021f..a64fa8c8eb 100644
--- a/libinterp/octave-value/cdef-class.h
+++ b/libinterp/octave-value/cdef-class.h
@@ -124,6 +124,8 @@ private:
 
     OCTINTERP_API octave_value get_method (const std::string& name) const;
 
+    OCTINTERP_API octave_value get_method (int line) const;
+
     OCTINTERP_API octave_value construct (const octave_value_list& args);
 
     OCTINTERP_API cdef_object
@@ -357,6 +359,11 @@ public:
     return get_rep ()->get_method (nm);
   }
 
+  octave_value get_method (int ln) const
+  {
+    return get_rep ()->get_method (ln);
+  }
+
   OCTINTERP_API octave_value get_method_function (const std::string& nm);
 
   octave_value get_constructor_function ()
diff --git a/libinterp/parse-tree/bp-table.cc b/libinterp/parse-tree/bp-table.cc
index 9af6b91a23..bcda1e4383 100644
--- a/libinterp/parse-tree/bp-table.cc
+++ b/libinterp/parse-tree/bp-table.cc
@@ -33,11 +33,13 @@
 #include <map>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "file-ops.h"
 #include "oct-env.h"
 
 #include "bp-table.h"
+#include "cdef-utils.h"
 #include "defun-int.h"
 #include "error.h"
 #include "event-manager.h"
@@ -607,8 +609,8 @@ void bp_table::process_id_list (const char *who,
 // If END_LINE != 0, *END_LINE is set to last line of the returned function.
 
 static octave_user_code * find_fcn_by_line (octave_user_code *main_fcn,
-    int lineno,
-    int *end_line = nullptr)
+                                            int lineno,
+                                            int *end_line = nullptr)
 {
   octave_user_code *retval = nullptr;
   octave_user_code *next_fcn = nullptr;  // 1st function starting after lineno
@@ -682,6 +684,101 @@ int bp_table::add_breakpoint_in_function (const std::string& fname,
   return result.empty () ? 0 : *(result.begin ());
 }
 
+// A class that encapsulates the "get_user_code" operation.
+// This operation changes depending on the symbol being a funciton or classdef.
+// Octave used to assume everything was a function, one per file.
+// Classdefs can have several functions in a single file.
+class user_code_provider
+{
+public:
+  user_code_provider (const std::string& fname, octave_user_code* pfcn)
+    : m_fcn (nullptr), m_is_valid (false)
+  {
+    if (pfcn)
+      {
+        // Already have the usercode, no need to do anything.
+        m_fcn = pfcn;
+        m_is_valid = true;
+      }
+    else
+      {
+        // Independently of cname being empty, if we get here try to get a classdef:
+        // Is it a classdef file plus line numbers?
+        m_cls = lookup_class (fname, false, false);
+        m_is_valid = m_cls.ok () && (m_cls.get_name () == fname);
+        if (m_is_valid)
+          populate_function_cache ();
+        else
+          error ("add_breakpoints_in_function: unable to find function '%s'\n",
+                 fname.c_str ());
+      }
+  }
+
+  octave_user_code * operator() (int line = 1)
+  {
+    if (! is_valid ())
+      return nullptr;
+
+    if (is_function ())
+      return find_fcn_by_line (m_fcn, line);
+
+    return m_cls.get_method (line).user_code_value (true);
+  }
+
+  bool is_function () const { return m_fcn != nullptr; }
+
+  bool is_valid () const { return m_is_valid; }
+
+  size_t number_of_functions () const
+  {
+    if (! is_valid ())
+      return 0;
+
+    if (is_function ())
+      return 1;
+
+    return m_methods_cache.size ();
+  }
+
+  octave_user_code * get_function (const size_t i) const
+  {
+    if (! is_valid ())
+      return nullptr;
+
+    if (is_function ())
+      return m_fcn;
+
+    if (i < m_methods_cache.size ())
+      return m_methods_cache[i];
+
+    return nullptr;
+  }
+
+private:
+  void populate_function_cache ()
+  {
+      if (m_methods_cache.empty ())
+        {
+          // Not the most effecient, but reuses code:
+          const std::map<std::string, cdef_method>& map
+            = m_cls.get_method_map (false, true);
+          for(auto iter = map.cbegin (); iter != map.cend (); ++iter)
+            {
+              octave_user_code *fcn
+                = m_cls.get_method (iter->first).user_code_value (true);
+              if (fcn != nullptr)
+                m_methods_cache.push_back (fcn);
+            }
+        }
+  }
+
+private:
+  std::vector<octave_user_code *> m_methods_cache;
+  cdef_class m_cls;
+  octave_user_code *m_fcn;
+  bool m_is_valid;
+};
+
 // Given file name fname, find the subfunction at line and create
 // a breakpoint there.  Put the system into debug_mode.
 bp_table::bp_lines
@@ -690,11 +787,8 @@ bp_table::add_breakpoints_in_function (const std::string& fname,
                                        const bp_table::bp_lines& lines,
                                        const std::string& condition)
 {
-  octave_user_code *main_fcn = m_evaluator.get_user_code (fname, class_name);
-
-  if (! main_fcn)
-    error ("add_breakpoints_in_function: unable to find function '%s'\n",
-           fname.c_str ());
+  user_code_provider
+    user_code (fname, m_evaluator.get_user_code (fname, class_name));
 
   condition_valid (condition);  // Throw error if condition not valid.
 
@@ -702,7 +796,7 @@ bp_table::add_breakpoints_in_function (const std::string& fname,
 
   for (const auto& lineno : lines)
     {
-      octave_user_code *dbg_fcn = find_fcn_by_line (main_fcn, lineno);
+      octave_user_code *dbg_fcn = user_code (lineno);
 
       // We've found the right (sub)function.  Now insert the breakpoint.
       bp_lines line_info;
@@ -814,7 +908,7 @@ bp_table::remove_breakpoint_from_function (const std::string& fname, int line)
 
 int
 bp_table::remove_breakpoints_from_function (const std::string& fname,
-    const bp_table::bp_lines& lines)
+                                            const bp_table::bp_lines& lines)
 {
   int retval = 0;
 
@@ -826,32 +920,68 @@ bp_table::remove_breakpoints_from_function (const std::string& fname,
   else
     {
       octave_user_code *dbg_fcn = m_evaluator.get_user_code (fname);
+      user_code_provider user_code (fname, dbg_fcn);
 
-      if (! dbg_fcn)
-        error ("remove_breakpoints_from_function: unable to find function %s\n",
-               fname.c_str ());
-
-      retval = remove_breakpoint_1 (dbg_fcn, fname, lines);
+      if (user_code.is_valid ())
+        {
+          // Remove all breakpoints from all functions
+          for (const auto line : lines)
+            {
+              octave_user_code *fcn = user_code (line);
+              std::string file = fcn->fcn_file_name ();
 
-      // Search subfunctions in the order they appear in the file.
+              tree_statement_list *cmds = fcn->body ();
+              if (cmds)
+                {
+                  octave_value_list results = cmds->list_breakpoints ();
 
-      const std::list<std::string> subfcn_names
-        = dbg_fcn->subfunction_names ();
+                  if (results.length () > 0)
+                    {
+                      interpreter& interp = m_evaluator.get_interpreter ();
+                      event_manager& evmgr = interp.get_event_manager ();
 
-      std::map<std::string, octave_value> subfcns
-        = dbg_fcn->subfunctions ();
+                      cmds->delete_breakpoint (line);
 
-      for (const auto& subf_nm : subfcn_names)
-        {
-          const auto q = subfcns.find (subf_nm);
+                      if (! file.empty ())
+                        evmgr.update_breakpoint (false, file, line);
+                    }
+                }
+            }
 
-          if (q != subfcns.end ())
+          // Remove all breakpoints from all subfunctions
+          if (dbg_fcn != nullptr)
             {
-              octave_user_code *dbg_subfcn = q->second.user_code_value ();
+              // Search subfunctions in the order they appear in the file.
+              const std::list<std::string> subfcn_names
+                = dbg_fcn->subfunction_names ();
+
+              std::map<std::string, octave_value> subfcns
+                = dbg_fcn->subfunctions ();
 
-              retval += remove_breakpoint_1 (dbg_subfcn, fname, lines);
+              for (const auto& subf_nm : subfcn_names)
+                {
+                  const auto q = subfcns.find (subf_nm);
+
+                  if (q != subfcns.end ())
+                    {
+                      octave_user_code *dbg_subfcn
+                        = q->second.user_code_value ();
+
+                      retval += remove_breakpoint_1 (dbg_subfcn, fname, lines);
+                    }
+                }
             }
+          // Remove file from breakpoint set if no breakpoints remain
+          octave_value_list fname_list = {fname};
+          const bool no_breakpoints
+            = get_breakpoint_list (fname_list).empty ();
+          auto iter = m_bp_set.find (fname);
+          if (no_breakpoints && iter != m_bp_set.end ())
+            m_bp_set.erase (iter);
         }
+      else
+        error ("remove_breakpoints_from_function: unable to find function %s\n",
+               fname.c_str ());
     }
 
   m_evaluator.reset_debug_state ();
@@ -863,30 +993,34 @@ bp_table::remove_breakpoints_from_function (const std::string& fname,
 
 bp_table::bp_lines
 bp_table::remove_all_breakpoints_from_function (const std::string& fname,
-    bool silent)
+                                                bool silent)
 {
   bp_lines retval;
 
-  octave_user_code *dbg_fcn = m_evaluator.get_user_code (fname);
+  octave_user_code *fcn = m_evaluator.get_user_code (fname);
+  user_code_provider user_code (fname, fcn);
 
-  if (dbg_fcn)
+  if (user_code.is_valid ())
     {
-      std::string file = dbg_fcn->fcn_file_name ();
-
-      tree_statement_list *cmds = dbg_fcn->body ();
-
-      if (cmds)
+      for (size_t i = 0; i != user_code.number_of_functions (); ++i)
         {
-          interpreter& interp = m_evaluator.get_interpreter ();
+          octave_user_code *dbg_fcn = user_code.get_function (i);
+          std::string file = dbg_fcn->fcn_file_name ();
 
-          event_manager& evmgr = interp.get_event_manager ();
+          tree_statement_list *cmds = dbg_fcn->body ();
 
-          retval = cmds->remove_all_breakpoints (evmgr, file);
+          if (cmds)
+            {
+              interpreter& interp = m_evaluator.get_interpreter ();
 
-          auto it = m_bp_set.find (fname);
-          if (it != m_bp_set.end ())
-            m_bp_set.erase (it);
+              event_manager& evmgr = interp.get_event_manager ();
+
+              retval = cmds->remove_all_breakpoints (evmgr, file);
+            }
         }
+      auto it = m_bp_set.find (fname);
+      if (it != m_bp_set.end ())
+        m_bp_set.erase (it);
     }
   else if (! silent)
     error ("remove_all_breakpoints_from_function: "
@@ -928,7 +1062,7 @@ bp_table::remove_breakpoints_from_file (const std::string& file,
 
 bp_table::bp_lines
 bp_table::remove_all_breakpoints_from_file (const std::string& file,
-    bool silent)
+                                            bool silent)
 {
   // Duplicates what the GUI was doing previously, but this action
   // should not be specific to the GUI.
@@ -986,22 +1120,39 @@ bp_table::get_breakpoint_list (const octave_value_list& fname_list)
           || find_bkpt_list (fname_list, bp_fname) != "")
         {
           octave_user_code *dbg_fcn = m_evaluator.get_user_code (bp_fname);
-
-          if (dbg_fcn)
+          user_code_provider user_code (bp_fname, dbg_fcn);
+          // Gather breakpoints from all functions in the file
+          if (user_code.is_valid ())
             {
-              tree_statement_list *cmds = dbg_fcn->body ();
-
-              // FIXME: move the operation on cmds to the
-              //        tree_statement_list class?
-              if (cmds)
+              std::list<bp_type> all_bkpts;
+              std::list<bp_type>::iterator it (all_bkpts.begin ());
+              for (size_t i = 0; i != user_code.number_of_functions (); ++i)
                 {
-                  std::list<bp_type> bkpts = cmds->breakpoints_and_conds ();
+                  octave_user_code *fcn = user_code.get_function (i);
+                  if (fcn)
+                    {
+                      tree_statement_list *cmds = fcn->body ();
+                      // FIXME: move the operation on cmds to the
+                      //        tree_statement_list class?
+                      if (cmds)
+                        {
+                          const std::list<bp_type>& bp
+                            = cmds->breakpoints_and_conds ();
+                          if(!bp.empty())
+                            it = all_bkpts.insert (it, bp.cbegin (),
+                                                   bp.cend ());
+                        }
+                    }
 
-                  if (! bkpts.empty ())
-                    retval[bp_fname] = bkpts;
                 }
+              if (! all_bkpts.empty ())
+                retval[bp_fname] = all_bkpts;
+            }
 
-              // look for breakpoints in subfunctions
+          // look for breakpoints in subfunctions
+          // Assuming classdefs can't have subfunctions
+          if (dbg_fcn != nullptr)
+            {
               const std::list<std::string> subf_nm
                 = dbg_fcn->subfunction_names ();
 
@@ -1017,7 +1168,7 @@ bp_table::get_breakpoint_list (const octave_value_list& fname_list)
                       octave_user_code *dbg_subfcn
                         = q->second.user_code_value ();
 
-                      cmds = dbg_subfcn->body ();
+                      tree_statement_list *cmds = dbg_subfcn->body ();
                       if (cmds)
                         {
                           std::list<bp_type> bkpts
diff --git a/test/Makefile.am b/test/Makefile.am
index ff195614bb..908ffb7171 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -109,6 +109,7 @@ include bug-61105/module.mk
 include bug-61191/module.mk
 include class-concat/module.mk
 include classdef/module.mk
+include classdef-debug/module.mk
 include classdef-multiple-inheritance/module.mk
 include classes/module.mk
 include colon-op/module.mk
diff --git a/test/classdef-debug/classdef_breakpoints.m b/test/classdef-debug/classdef_breakpoints.m
new file mode 100644
index 0000000000..49c2906219
--- /dev/null
+++ b/test/classdef-debug/classdef_breakpoints.m
@@ -0,0 +1,21 @@
+classdef classdef_breakpoints
+  properties
+    m_prop = [];
+  endproperties
+  methods
+    function this = classdef_breakpoints (prop)
+      this.m_prop = prop;
+    endfunction
+    function this = foo (this, prop)
+      this.m_prop = prop;
+    endfunction
+    function this = bar (this)
+      printf ("prop: %f\n", this.m_prop);
+    endfunction
+  endmethods
+endclassdef
+
+function my_subfunction (n)
+  ## my_subfunction hello
+  printf ("subfunction: %f\n", n);
+endfunction
diff --git a/test/classdef-debug/module.mk b/test/classdef-debug/module.mk
new file mode 100644
index 0000000000..7beaf1440f
--- /dev/null
+++ b/test/classdef-debug/module.mk
@@ -0,0 +1,5 @@
+classdef_breakpoints_TEST_FILES = \
+  %reldir%/classdef_breakpoints.m \
+  %reldir%/test_classdef_breakpoints.tst
+
+TEST_FILES += $(classdef_breakpoints_TEST_FILES)
diff --git a/test/classdef-debug/test_classdef_breakpoints.tst b/test/classdef-debug/test_classdef_breakpoints.tst
new file mode 100644
index 0000000000..96465bb474
--- /dev/null
+++ b/test/classdef-debug/test_classdef_breakpoints.tst
@@ -0,0 +1,31 @@
+##  Test script for setting breakpoints in classdefs using line numbers.
+
+%!function assert_dbstatus (bp0)
+%!  dbs = dbstatus ();
+%!  bp = [dbs(:).line];
+%!  assert (length (bp) == length (bp0),
+%!          'Number of breakpoints set must match');
+%!  if (length (bp) != 0)
+%!    assert (sort (bp) == bp0, 'Breakpoint line numbers must be equal');
+%!  endif
+%!endfunction
+
+## Add breakpoints in different member functions using line numbers.
+%!test <*46451>
+%! dbstop classdef_breakpoints 13 7 10;
+%! assert_dbstatus ([7, 10, 13]);
+
+## Remove one breakpoint and confirm the others remain.
+%!test <*46451>
+%! dbclear classdef_breakpoints 10;
+%! assert_dbstatus ([7, 13]);
+
+## Add breakpoint in local function.
+%!test <46451>
+%! dbstop classdef_breakpoints 19;
+%! assert_dbstatus ([7, 13, 20]);
+
+## Clear all breakpoints, none should be left.
+%!test <*46451>
+%! dbclear classdef_breakpoints;
+%! assert_dbstatus ([]);
-- 
2.27.0

