From 4ee14f0f770d1bd83f7ff3e246844715413537ec Mon Sep 17 00:00:00 2001
From: paulo
Date: Sat, 29 Apr 2023 23:20:51 +0900
Subject: [PATCH] add support for classdef breakpoints (bug #46451)

* libinterp/octave-value/cdef-class.cc: Add cdef_class::cdef_class_rep::get_method (size_t 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 (size_t 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.tst: New script for testing breakpoints functionality.

* test/classdef-debug/breakpoints/assert_dbstatus.m: New script to help writing tests.
---
 libinterp/octave-value/cdef-class.cc          |  31 +++
 libinterp/octave-value/cdef-class.h           |   7 +
 libinterp/parse-tree/bp-table.cc              | 261 ++++++++++++++----
 test/Makefile.am                              |   1 +
 .../breakpoints/assert_dbstatus.m             |   8 +
 .../breakpoints/classdef_breakpoints.m        |  21 ++
 .../breakpoints/test_classdef_breakpoints.tst |  47 ++++
 test/classdef-debug/module.mk                 |   6 +
 8 files changed, 328 insertions(+), 54 deletions(-)
 create mode 100644 test/classdef-debug/breakpoints/assert_dbstatus.m
 create mode 100644 test/classdef-debug/breakpoints/classdef_breakpoints.m
 create mode 100644 test/classdef-debug/breakpoints/test_classdef_breakpoints.tst
 create mode 100644 test/classdef-debug/module.mk

diff --git a/libinterp/octave-value/cdef-class.cc b/libinterp/octave-value/cdef-class.cc
index 57609e8834..3c689e2911 100644
--- a/libinterp/octave-value/cdef-class.cc
+++ b/libinterp/octave-value/cdef-class.cc
@@ -708,6 +708,37 @@ 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 (size_t 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(method_const_iterator 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..f7045fc0c6 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 (size_t 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..9c9c9cb011 100644
--- a/libinterp/parse-tree/bp-table.cc
+++ b/libinterp/parse-tree/bp-table.cc
@@ -33,6 +33,7 @@
 #include <map>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "file-ops.h"
 #include "oct-env.h"
@@ -53,9 +54,14 @@
 #include "pt-exp.h"
 #include "pt-stmt.h"
 #include "sighandlers.h"
+#include "cdef-utils.h"
 
 OCTAVE_BEGIN_NAMESPACE(octave)
 
+static octave_user_code * find_fcn_by_line (octave_user_code *main_fcn,
+    int lineno,
+    int *end_line = nullptr);
+
 class bp_file_info
 {
 public:
@@ -112,6 +118,105 @@ private:
   std::string m_class_name;
 };
 
+// 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)
+  {
+    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);
+  }
+
+  octave_user_code* operator()()
+  {
+    return (*this)(1);
+  }
+
+  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);
+          typedef std::map<std::string, cdef_method>::const_iterator method_const_iterator;
+          for(method_const_iterator i = map.cbegin(); i != map.cend(); ++i)
+            {
+              octave_user_code* fcn = m_cls.get_method(i->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;
+};
+
 // Clear all reasons to stop, other than breakpoints.
 
 void bp_table::dbclear_all_signals ()
@@ -608,7 +713,7 @@ void bp_table::process_id_list (const char *who,
 
 static octave_user_code * find_fcn_by_line (octave_user_code *main_fcn,
     int lineno,
-    int *end_line = nullptr)
+    int *end_line)
 {
   octave_user_code *retval = nullptr;
   octave_user_code *next_fcn = nullptr;  // 1st function starting after lineno
@@ -690,11 +795,7 @@ 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 get_user_code(fname, m_evaluator.get_user_code (fname, class_name));
 
   condition_valid (condition);  // Throw error if condition not valid.
 
@@ -702,7 +803,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 = get_user_code(lineno);
 
       // We've found the right (sub)function.  Now insert the breakpoint.
       bp_lines line_info;
@@ -814,7 +915,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 +927,65 @@ 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 get_user_code(fname, dbg_fcn);
 
-      if (! dbg_fcn)
-        error ("remove_breakpoints_from_function: unable to find function %s\n",
-               fname.c_str ());
+      if(get_user_code.is_valid())
+      {
+        // Remove all breakpoints from all functions
+        for(const auto line : lines)
+          {
+            octave_user_code *fcn = get_user_code(line);
+            std::string file = fcn->fcn_file_name ();
 
-      retval = remove_breakpoint_1 (dbg_fcn, fname, lines);
+            tree_statement_list *cmds = fcn->body ();
+            if (cmds)
+              {
+                octave_value_list results = cmds->list_breakpoints ();
 
-      // Search subfunctions in the order they appear in the file.
+                if (results.length () > 0)
+                  {
+                    interpreter& interp = m_evaluator.get_interpreter ();
+                    event_manager& evmgr = interp.get_event_manager ();
 
-      const std::list<std::string> subfcn_names
-        = dbg_fcn->subfunction_names ();
+                    cmds->delete_breakpoint (line);
 
-      std::map<std::string, octave_value> subfcns
-        = dbg_fcn->subfunctions ();
+                    if (! file.empty ())
+                      evmgr.update_breakpoint (false, file, line);
+                  }
+              }
+          }
+        // Remove all breakpoints from all subfunctions
+        if(dbg_fcn != nullptr)
+          {
+            // Search subfunctions in the order they appear in the file.
+            const std::list<std::string> subfcn_names
+              = dbg_fcn->subfunction_names ();
 
-      for (const auto& subf_nm : subfcn_names)
-        {
-          const auto q = subfcns.find (subf_nm);
+            std::map<std::string, octave_value> subfcns
+              = dbg_fcn->subfunctions ();
 
-          if (q != subfcns.end ())
-            {
-              octave_user_code *dbg_subfcn = q->second.user_code_value ();
+            for (const auto& subf_nm : subfcn_names)
+              {
+                const auto q = subfcns.find (subf_nm);
 
-              retval += remove_breakpoint_1 (dbg_subfcn, fname, lines);
-            }
-        }
+                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 it = m_bp_set.find (fname);
+        if (no_breakpoints && it != m_bp_set.end ())
+          m_bp_set.erase (it);
+      }
+      else
+        error ("remove_breakpoints_from_function: unable to find function %s\n",
+               fname.c_str ());
     }
 
   m_evaluator.reset_debug_state ();
@@ -863,30 +997,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 get_user_code(fname, fcn);
 
-  if (dbg_fcn)
+  if (get_user_code.is_valid())
     {
-      std::string file = dbg_fcn->fcn_file_name ();
-
-      tree_statement_list *cmds = dbg_fcn->body ();
+      for(size_t i = 0; i != get_user_code.number_of_functions(); ++i)
+      {
+        octave_user_code *dbg_fcn = get_user_code.get_function(i);
+        std::string file = dbg_fcn->fcn_file_name ();
 
-      if (cmds)
-        {
-          interpreter& interp = m_evaluator.get_interpreter ();
+        tree_statement_list *cmds = dbg_fcn->body ();
 
-          event_manager& evmgr = interp.get_event_manager ();
+        if (cmds)
+          {
+            interpreter& interp = m_evaluator.get_interpreter ();
 
-          retval = cmds->remove_all_breakpoints (evmgr, file);
+            event_manager& evmgr = interp.get_event_manager ();
 
-          auto it = m_bp_set.find (fname);
-          if (it != m_bp_set.end ())
-            m_bp_set.erase (it);
-        }
+            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: "
@@ -979,29 +1117,44 @@ bp_table::get_breakpoint_list (const octave_value_list& fname_list)
 
   // make copy since changes may invalidate iters of m_bp_set.
   std::set<std::string> tmp_bp_set = m_bp_set;
-
+  
   for (auto& bp_fname : tmp_bp_set)
     {
       if (fname_list.empty ()
           || 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 get_user_code(bp_fname, dbg_fcn);
+          // Gather breakpoints from all functions in the file
+          if(get_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 != get_user_code.number_of_functions(); ++i)
                 {
-                  std::list<bp_type> bkpts = cmds->breakpoints_and_conds ();
+                  octave_user_code *fcn = get_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 +1170,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/breakpoints/assert_dbstatus.m b/test/classdef-debug/breakpoints/assert_dbstatus.m
new file mode 100644
index 0000000000..fe038bc839
--- /dev/null
+++ b/test/classdef-debug/breakpoints/assert_dbstatus.m
@@ -0,0 +1,8 @@
+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.')
+	end
+end
diff --git a/test/classdef-debug/breakpoints/classdef_breakpoints.m b/test/classdef-debug/breakpoints/classdef_breakpoints.m
new file mode 100644
index 0000000000..321012e72f
--- /dev/null
+++ b/test/classdef-debug/breakpoints/classdef_breakpoints.m
@@ -0,0 +1,21 @@
+classdef classdef_breakpoints
+	properties
+		m_prop = [];
+	end
+	methods
+		function this = classdef_breakpoints(prop)
+			this.m_prop = prop;
+		end
+		function this = foo(this, prop)
+			this.m_prop = prop;
+		end
+		function this = bar(this)
+			printf("prop: %f\n", this.m_prop);
+		end
+	end
+end
+
+function MySubfunction(n)
+	% MySubfunction helo
+	printf("subfunction: %f\n", n);
+end
diff --git a/test/classdef-debug/breakpoints/test_classdef_breakpoints.tst b/test/classdef-debug/breakpoints/test_classdef_breakpoints.tst
new file mode 100644
index 0000000000..14acf14b4c
--- /dev/null
+++ b/test/classdef-debug/breakpoints/test_classdef_breakpoints.tst
@@ -0,0 +1,47 @@
+########################################################################
+##
+## Copyright (C) 2017-2023 The Octave Project Developers
+##
+## See the file COPYRIGHT.md in the top-level directory of this
+## distribution or <https://octave.org/copyright/>.
+##
+## 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
+## <https://www.gnu.org/licenses/>.
+##
+########################################################################
+
+%%  Test script for setting breakpoints in classdefs using line numbers.
+%%
+%%  Note: This script and all classes are also intended to run
+%%        in MATLAB to test compatibility.  Don't break that!
+%%
+
+%% Add support for cli and gui breakpoints (bug #46451)
+%% 1) Add classdef to path, and now we should be able to set breakpoints.
+%!test
+%! addpath ../test/classdef-debug/breakpoints/
+%! dbstop classdef_breakpoints 13 7 10;
+%! assert_dbstatus([7 10 13])
+
+%% 2) Remove one breakpont and confirm the other remain.
+%!test
+%! dbclear classdef_breakpoints 10;
+%! assert_dbstatus([7 13])
+
+%% 3) Clear all breakpoints, none should be left.
+%!test
+%! dbclear classdef_breakpoints;
+%! assert_dbstatus([])
diff --git a/test/classdef-debug/module.mk b/test/classdef-debug/module.mk
new file mode 100644
index 0000000000..4443d6936b
--- /dev/null
+++ b/test/classdef-debug/module.mk
@@ -0,0 +1,6 @@
+classdef_breakpoints_TEST_FILES = \
+  %reldir%/breakpoints/assert_dbstatus.m \
+  %reldir%/breakpoints/classdef_breakpoints.m \
+  %reldir%/breakpoints/test_classdef_breakpoints.tst
+
+TEST_FILES += $(classdef_breakpoints_TEST_FILES)
-- 
2.27.0

