[PATCH v1] [gdb/infcmd]: Add next-expression command

[PATCH v1] [gdb/infcmd]: Add next-expression command

[Simon Farre]

This commit adds the source-level "next-expression" command. It utilizes
column metadata emitted by the compiler (if any). Column metadata
has been defined in the DWARF spec since 2.0, I am currently unaware
of how other debug information types work with columns and as such
this patch only introduces support for binaries that has DWARF emitted.

Much of the code in `next_expression` (infcmd.c) was borrowed from the `until_command`
as I was thinking it really should behave like it; because we know the addresses
we want to run to, we don't want to do intermediate stepping until that point,
just execute to that position. However, it was pretty
difficult for me to understand what's really going on; so please review
this part extra carefully if you have the time (infcmd.c)

For the other debug info types, column metadata is currently set to 0, to signal that
"we don't have column metadata"; this seems to be default behavior for line information
in other places as well so I continued that tradition.

This command will also map to the DAP request "nextRequest"
with the parameter "stmt"; however, after discussion on IRC,
I was informed that statement is not the correct term here
so I named the command to "next-expression"; though it can
step across statements on the same line.

When printing frame (either when typing frame on the cli)
or "on stopped" event in MI, column information is printed,
with the format file:line:column; as I think that's the most
common way to represent file coordinates.

The column information is also exposed via the Python object
Symtab_and_line. Documentation has been added for this,
as well as the command in gdb.texinfo.

Examples

Example 1:
Say we have
foo().bar().baz() and we're currently at _foo();

next-expression 2 will land us on _baz()

NOTE ON DEFAULT BEHAVIOR:
If the user types next-expression 10 in the above example
the command will iterate over linetable_entries and find that
there is not 10 columns on this line and fall back to the "next command"
I figured this was a sane default because if a user wants to "next-expression"
they are attempting to reach at some point _on the same source line_; however
I am absolutely willing to change this if anyone has any objections to this default.

Example 2:
Say we have
for(int i = 10; i < 10; i++) and we're at
_int i = 10;

next-expression will land us at _i < 10;

So next-expression will "next to" the next statement on
a line as well.
---
 gdb/NEWS               |   6 ++
 gdb/buildsym-legacy.c  |   4 +-
 gdb/buildsym-legacy.h  |   2 +-
 gdb/buildsym.c         |   3 +-
 gdb/buildsym.h         |   4 +-
 gdb/coffread.c         |   4 +-
 gdb/dbxread.c          |   6 +-
 gdb/doc/gdb.texinfo    |  16 +++++-
 gdb/doc/python.texi    |   5 ++
 gdb/dwarf2/read.c      |  31 ++++++----
 gdb/infcmd.c           | 128 ++++++++++++++++++++++++++++++++++++++++-
 gdb/mdebugread.c       |   2 +-
 gdb/python/py-symtab.c |  25 +++++---
 gdb/stack.c            |   7 +++
 gdb/symtab.c           |   1 +
 gdb/symtab.h           |  27 +++++++++
 16 files changed, 238 insertions(+), 33 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 6aa0d5171f2..bbd5c1d2bb9 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,12 @@
 
 *** Changes since GDB 13
 
+* Added the "next-expression" ("ne") command that source-level steps to
+  columns if the required debug info metadata was emitted by the compiler.
+
+* Added column attribute to Python type Symtab_and_line to reflect the added
+  column field on "linetable_entry" which is used in "next-expression" command.
+
 * The AArch64 'org.gnu.gdb.aarch64.pauth' Pointer Authentication feature string
   has been deprecated in favor of the 'org.gnu.gdb.aarch64.pauth_v2' feature
   string.
diff --git a/gdb/buildsym-legacy.c b/gdb/buildsym-legacy.c
index 131d24fe9b5..5036fd2b48d 100644
--- a/gdb/buildsym-legacy.c
+++ b/gdb/buildsym-legacy.c
@@ -205,12 +205,12 @@ finish_block (struct symbol *symbol, struct pending_block *old_blocks,
 }
 
 void
-record_line (struct subfile *subfile, int line, unrelocated_addr pc)
+record_line (struct subfile *subfile, int line, int col, unrelocated_addr pc)
 {
   gdb_assert (buildsym_compunit != nullptr);
   /* Assume every line entry is a statement start, that is a good place to
      put a breakpoint for that line number.  */
-  buildsym_compunit->record_line (subfile, line, pc, LEF_IS_STMT);
+  buildsym_compunit->record_line (subfile, line, col, pc, LEF_IS_STMT);
 }
 
 /* Start a new compunit_symtab for a new source file in OBJFILE.  Called, for
diff --git a/gdb/buildsym-legacy.h b/gdb/buildsym-legacy.h
index 664d6320e54..be44d0f5783 100644
--- a/gdb/buildsym-legacy.h
+++ b/gdb/buildsym-legacy.h
@@ -76,7 +76,7 @@ extern struct context_stack *push_context (int desc, CORE_ADDR valu);
 
 extern struct context_stack pop_context ();
 
-extern void record_line (struct subfile *subfile, int line,
+extern void record_line (struct subfile *subfile, int line, int col,
 			 unrelocated_addr pc);
 
 extern struct compunit_symtab *start_compunit_symtab (struct objfile *objfile,
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index d12ad2187ab..6b3859ea45e 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -626,7 +626,7 @@ buildsym_compunit::pop_subfile ()
    line vector for SUBFILE.  */
 
 void
-buildsym_compunit::record_line (struct subfile *subfile, int line,
+buildsym_compunit::record_line (struct subfile *subfile, int line, int col,
 				unrelocated_addr pc, linetable_entry_flags flags)
 {
   m_have_line_numbers = true;
@@ -666,6 +666,7 @@ buildsym_compunit::record_line (struct subfile *subfile, int line,
 
   subfile->line_vector_entries.emplace_back ();
   linetable_entry &e = subfile->line_vector_entries.back ();
+  e.column = col;
   e.line = line;
   e.is_stmt = (flags & LEF_IS_STMT) != 0;
   e.set_raw_pc (pc);
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 98dc8f02874..d3328c630d8 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -239,8 +239,8 @@ struct buildsym_compunit
 
   const char *pop_subfile ();
 
-  void record_line (struct subfile *subfile, int line, unrelocated_addr pc,
-		    linetable_entry_flags flags);
+  void record_line (struct subfile *subfile, int line, int col,
+  		    unrelocated_addr pc, linetable_entry_flags flags);
 
   struct compunit_symtab *get_compunit_symtab ()
   {
diff --git a/gdb/coffread.c b/gdb/coffread.c
index ff4d4ae5313..dfb75b12799 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -1133,7 +1133,7 @@ coff_symtab_read (minimal_symbol_reader &reader,
 		 other statement-line-number.  */
 	      if (fcn_last_line == 1)
 		record_line
-		  (get_current_subfile (), fcn_first_line,
+		  (get_current_subfile (), fcn_first_line, 0,
 		   unrelocated_addr (gdbarch_addr_bits_remove (gdbarch,
 							       fcn_first_line_addr)));
 	      else
@@ -1462,7 +1462,7 @@ enter_linenos (file_ptr file_offset, int first_line,
 	{
 	  CORE_ADDR addr = lptr.l_addr.l_paddr;
 	  record_line (get_current_subfile (),
-		       first_line + L_LNNO32 (&lptr),
+		       first_line + L_LNNO32 (&lptr), 0,
 		       unrelocated_addr (gdbarch_addr_bits_remove (gdbarch,
 								   addr)));
 	}
diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index 73371edd841..310ee0eef4d 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2476,7 +2476,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 	      CORE_ADDR addr = last_function_start + valu;
 
 	      record_line
-		(get_current_subfile (), 0,
+		(get_current_subfile (), 0, 0,
 		 unrelocated_addr (gdbarch_addr_bits_remove (gdbarch, addr)
 				   - objfile->text_section_offset ()));
 	    }
@@ -2686,14 +2686,14 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 			   last_function_start : valu;
 
 	  record_line
-	    (get_current_subfile (), desc,
+	    (get_current_subfile (), desc, 0,
 	     unrelocated_addr (gdbarch_addr_bits_remove (gdbarch, addr)
 			       - objfile->text_section_offset ()));
 	  sline_found_in_function = 1;
 	}
       else
 	record_line
-	  (get_current_subfile (), desc,
+	  (get_current_subfile (), desc, 0,
 	   unrelocated_addr (gdbarch_addr_bits_remove (gdbarch, valu)
 			     - objfile->text_section_offset ()));
       break;
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 531147f6e6b..c489408cd60 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -6359,6 +6359,20 @@ The @code{next} command only stops at the first instruction of a
 source line.  This prevents multiple stops that could otherwise occur in
 @code{switch} statements, @code{for} loops, etc.
 
+@kindex next-expression
+@kindex ne @r{(@code{next-expression})}
+@item next-expression @r{[}@var{count}@r{]}
+Continue to the next expression or statement on the current source line
+in the current (innermost) stack frame. This is a source-level command
+and as such requires that debug information was emitted by the compiler. If
+no such debug information could be found this command defaults to
+@code{next}.
+
+
+An argument @var{count} is a repeat count, as for @code{next}. If
+repeat count is larger than amount of expressions on the current source line
+the command will default to the @code{next} command.
+
 @kindex set step-mode
 @item set step-mode
 @cindex functions without line info, and stepping
@@ -31313,7 +31327,7 @@ An -exec-until or similar CLI command was accomplished.
 @item watchpoint-scope
 A watchpoint has gone out of scope.
 @item end-stepping-range
-An -exec-next, -exec-next-instruction, -exec-step, -exec-step-instruction or 
+An -exec-next, -exec-next-instruction, -exec-step, -exec-step-instruction or
 similar CLI command was accomplished.
 @item exited-signalled 
 The inferior exited because of a signal.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 7c3a3ccd379..d74566d999c 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5899,6 +5899,11 @@ Indicates the current line number for this object.  This
 attribute is not writable.
 @end defvar
 
+@defvar Symtab_and_line.column
+Indicates the current column number for this object. This
+attribute is not writeable.
+@end defvar
+
 A @code{gdb.Symtab_and_line} object has the following methods:
 
 @defun Symtab_and_line.is_valid ()
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index e91efe853b7..c7a170e94aa 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18100,6 +18100,11 @@ class lnp_state_machine
     m_flags |= LEF_PROLOGUE_END;
   }
 
+  void handle_set_column (unsigned int col)
+  {
+    m_column = col;
+  }
+
 private:
   /* Advance the line by LINE_DELTA.  */
   void advance_line (int line_delta)
@@ -18124,6 +18129,7 @@ class lnp_state_machine
   /* The line table index of the current file.  */
   file_name_index m_file = 1;
   unsigned int m_line = 1;
+  unsigned int m_column = 1;
 
   /* These are initialized in the constructor.  */
 
@@ -18274,9 +18280,8 @@ dwarf_record_line_p (struct dwarf2_cu *cu,
 
 static void
 dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
-		     unsigned int line, CORE_ADDR address,
-		     linetable_entry_flags flags,
-		     struct dwarf2_cu *cu)
+		     unsigned int line, unsigned int col, CORE_ADDR address,
+		     linetable_entry_flags flags, struct dwarf2_cu *cu)
 {
   unrelocated_addr addr
     = unrelocated_addr (gdbarch_addr_bits_remove (gdbarch, address));
@@ -18290,7 +18295,7 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
     }
 
   if (cu != nullptr)
-    cu->get_builder ()->record_line (subfile, line, addr, flags);
+    cu->get_builder ()->record_line (subfile, line, (int) col, addr, flags);
 }
 
 /* Subroutine of dwarf_decode_lines_1 to simplify it.
@@ -18313,7 +18318,7 @@ dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
 		  paddress (gdbarch, address));
     }
 
-  dwarf_record_line_1 (gdbarch, subfile, 0, address, LEF_IS_STMT, cu);
+  dwarf_record_line_1 (gdbarch, subfile, 0, 0, address, LEF_IS_STMT, cu);
 }
 
 void
@@ -18381,10 +18386,10 @@ lnp_state_machine::record_line (bool end_sequence)
 				   m_last_subfile))
 	    {
 	      buildsym_compunit *builder = m_cu->get_builder ();
-	      dwarf_record_line_1 (m_gdbarch,
-				   builder->get_current_subfile (),
-				   m_line, m_address, lte_flags,
-				   m_currently_recording_lines ? m_cu : nullptr);
+	      dwarf_record_line_1 (m_gdbarch, builder->get_current_subfile (),
+				   m_line, m_column, m_address, lte_flags,
+				   m_currently_recording_lines ? m_cu
+							       : nullptr);
 	    }
 	  m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
 	  m_last_line = m_line;
@@ -18605,8 +18610,12 @@ dwarf_decode_lines_1 (struct line_header *lh, struct dwarf2_cu *cu,
 	      }
 	      break;
 	    case DW_LNS_set_column:
-	      (void) read_unsigned_leb128 (abfd, line_ptr, &bytes_read);
-	      line_ptr += bytes_read;
+	      {
+		const auto col
+		  = read_unsigned_leb128 (abfd, line_ptr, &bytes_read);
+		state_machine.handle_set_column (col);
+		line_ptr += bytes_read;
+	      }
 	      break;
 	    case DW_LNS_negate_stmt:
 	      state_machine.handle_negate_stmt ();
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b12b58db9cb..0c57512c964 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -231,8 +231,7 @@ strip_bg_char (const char *args, int *bg_char_p)
 void
 post_create_inferior (int from_tty)
 {
-
-  /* Be sure we own the terminal in case write operations are performed.  */ 
+  /* Be sure we own the terminal in case write operations are performed.  */
   target_terminal::ours_for_output ();
 
   infrun_debug_show_threads ("threads in the newly created inferior",
@@ -833,6 +832,123 @@ step_command_fsm_prepare (struct step_command_fsm *sm,
   thread->control.stepping_command = 1;
 }
 
+// next-expression finite state machine
+struct next_expression_fsm : public thread_fsm
+{
+  int thread;
+  std::vector<breakpoint_up> bps;
+
+  next_expression_fsm (interp *cmd_interp, int thread,
+		       std::vector<breakpoint_up> &&breakpoints)
+      : thread_fsm (cmd_interp), thread (thread), bps (std::move (breakpoints))
+  {
+  }
+
+  void clean_up (thread_info *thread) override;
+  bool should_stop (thread_info *thread) override;
+  enum async_reply_reason do_async_reply_reason () override;
+};
+
+bool
+next_expression_fsm::should_stop (thread_info *tp)
+{
+  const auto bp = std::find_if (
+    bps.begin (), bps.end (), [bpstat = tp->control.stop_bpstat] (auto &bkpt) {
+      return bpstat_find_breakpoint (bpstat, bkpt.get ()) != nullptr;
+    });
+
+  if (bp != std::end (bps))
+    set_finished ();
+
+  return true;
+}
+
+void
+next_expression_fsm::clean_up (thread_info *)
+{
+  bps.clear ();
+  delete_longjmp_breakpoint (thread);
+}
+
+async_reply_reason
+next_expression_fsm::do_async_reply_reason ()
+{
+  return async_reply_reason::EXEC_ASYNC_LOCATION_REACHED;
+}
+
+static gdb::optional<CORE_ADDR>
+get_next_column_pc (frame_info_ptr frame, int column_count)
+{
+  const auto sal = find_frame_sal (frame);
+  const auto range = sal.symtab->linetable_entries_for (sal.line);
+
+  for (const auto &entry : range)
+    {
+      if (entry.column > sal.col)
+	column_count--;
+      if (column_count == 0)
+	return entry.pc (sal.pspace->symfile_object_file);
+    }
+  return {};
+}
+
+static void
+next_expression (const char *count_str, int from_tty)
+{
+  int doasync;
+  gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (count_str, &doasync);
+  auto processed_count_str = stripped.get ();
+
+  prepare_execution_command (current_inferior ()->top_target (), doasync);
+
+  const auto count
+    = processed_count_str ? parse_and_eval_long (processed_count_str) : 1;
+
+  auto frame = get_selected_frame (nullptr);
+  const auto next_col_addr = get_next_column_pc (frame, count);
+
+  if (!next_col_addr.has_value ())
+    return next_command (nullptr, from_tty);
+
+  clear_proceed_status (1);
+  auto tp = inferior_thread ();
+  auto thread = tp->global_num;
+
+  std::vector<breakpoint_up> breakpoints;
+  gdb::optional<delete_longjmp_breakpoint_cleanup> lj_deleter;
+
+  const auto caller_frame_id = frame_unwind_caller_id (frame);
+  if (frame_id_p (caller_frame_id))
+    {
+      struct symtab_and_line sal2;
+      struct gdbarch *caller_gdbarch;
+
+      sal2 = find_pc_line (frame_unwind_caller_pc (frame), 0);
+      sal2.pc = frame_unwind_caller_pc (frame);
+      caller_gdbarch = frame_unwind_caller_arch (frame);
+
+      breakpoint_up caller_breakpoint = set_momentary_breakpoint (
+	caller_gdbarch, sal2, caller_frame_id, bp_until);
+      breakpoints.emplace_back (std::move (caller_breakpoint));
+      const auto stack_frame_id = get_stack_frame_id (frame);
+      set_longjmp_breakpoint (tp, stack_frame_id);
+      lj_deleter.emplace (thread);
+    }
+
+  const auto frame_gdbarch = get_frame_arch (frame);
+  auto bp_loc = set_momentary_breakpoint_at_pc (frame_gdbarch, *next_col_addr,
+						bp_breakpoint);
+  breakpoints.emplace_back (std::move (bp_loc));
+  auto fsm = std::make_unique<next_expression_fsm> (
+    command_interp (), tp->global_num, std::move (breakpoints));
+  tp->set_thread_fsm (std::move (fsm));
+
+  if (lj_deleter)
+    lj_deleter->release ();
+
+  proceed (-1, GDB_SIGNAL_DEFAULT);
+}
+
 static int prepare_one_step (thread_info *, struct step_command_fsm *sm);
 
 static void
@@ -3311,6 +3427,14 @@ Argument N means step N times (or till program stops for another \
 reason)."));
   add_com_alias ("s", step_cmd, class_run, 1);
 
+  cmd_list_element *ne_cmd
+    = add_com ("next-expression", class_run, next_expression, _ ("\
+Step program by expressions info, proceeding through subroutine calls.\n\
+Usage: next-expression [N]\n\
+This is a source-level command and if no debug information was emitted by\n\
+the compiler this command will default to behaving like 'next'"));
+  add_com_alias ("ne", ne_cmd, class_run, 1);
+
   cmd_list_element *until_cmd
     = add_com ("until", class_run, until_command, _("\
 Execute until past the current line or past a LOCATION.\n\
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 697ce0b5b1a..aaf4878de25 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -4012,7 +4012,7 @@ mdebug_expand_psymtab (legacy_psymtab *pst, struct objfile *objfile)
 		{
 		  /* Handle encoded stab line number.  */
 		  record_line
-		    (get_current_subfile (), sh.index,
+		    (get_current_subfile (), sh.index, 0,
 		     unrelocated_addr (gdbarch_addr_bits_remove (gdbarch,
 								 valu)));
 		}
diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
index 26aa8b2fb04..f9dd112c67a 100644
--- a/gdb/python/py-symtab.c
+++ b/gdb/python/py-symtab.c
@@ -286,8 +286,8 @@ salpy_str (PyObject *self)
       filename = symtab_to_filename_for_display (symtab);
     }
 
-  return PyUnicode_FromFormat ("symbol and line for %s, line %d", filename,
-			       sal->line);
+  return PyUnicode_FromFormat ("symbol and line for %s, line %d:%d", filename,
+			       sal->line, sal->col);
 }
 
 static void
@@ -343,6 +343,16 @@ salpy_get_line (PyObject *self, void *closure)
   return gdb_py_object_from_longest (sal->line).release ();
 }
 
+static PyObject *
+salpy_get_col (PyObject *self, void *closure)
+{
+  struct symtab_and_line *sal = NULL;
+
+  SALPY_REQUIRE_VALID (self, sal);
+
+  return gdb_py_object_from_longest (sal->col).release ();
+}
+
 static PyObject *
 salpy_get_symtab (PyObject *self, void *closure)
 {
@@ -597,11 +607,12 @@ PyTypeObject symtab_object_type = {
 static gdb_PyGetSetDef sal_object_getset[] = {
   { "symtab", salpy_get_symtab, NULL, "Symtab object.", NULL },
   { "pc", salpy_get_pc, NULL, "Return the symtab_and_line's pc.", NULL },
-  { "last", salpy_get_last, NULL,
-    "Return the symtab_and_line's last address.", NULL },
-  { "line", salpy_get_line, NULL,
-    "Return the symtab_and_line's line.", NULL },
-  {NULL}  /* Sentinel */
+  { "last", salpy_get_last, NULL, "Return the symtab_and_line's last address.",
+    NULL },
+  { "line", salpy_get_line, NULL, "Return the symtab_and_line's line.", NULL },
+  { "column", salpy_get_col, NULL, "Return the symtab_and_line's column.",
+    NULL },
+  { NULL } /* Sentinel */
 };
 
 static PyMethodDef sal_object_methods[] = {
diff --git a/gdb/stack.c b/gdb/stack.c
index b1b25aa1c7e..590862236b7 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1418,6 +1418,13 @@ print_frame (const frame_print_options &fp_opts,
 	uiout->text (":");
 	annotate_frame_source_line ();
 	uiout->field_signed ("line", sal.line);
+
+	/* Only print column if we have column meta data. */
+	if (sal.col > 0)
+	  {
+	    uiout->text (":");
+	    uiout->field_signed ("column", sal.col);
+	  }
 	annotate_frame_source_end ();
       }
 
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 5e85c53d550..605267d60ba 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3247,6 +3247,7 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
       val.symtab = best_symtab;
       val.line = best->line;
       val.pc = best->pc (objfile);
+      val.col = best->column;
       if (best_end && (!alt || best_end < alt->pc (objfile)))
 	val.end = best_end;
       else if (alt)
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 404d0ab30a8..4f30b74f4a0 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1603,6 +1603,10 @@ struct linetable_entry
   /* The line number for this entry.  */
   int line;
 
+  /* The column number for this entry. 0 means it was set by code that
+     doesn't understand columns. */
+  int column;
+
   /* True if this PC is a good location to place a breakpoint for LINE.  */
   bool is_stmt : 1;
 
@@ -1669,6 +1673,26 @@ struct symtab
     return m_linetable;
   }
 
+  gdb::array_view<const linetable_entry>
+  linetable_entries_for (unsigned int line)
+  {
+    for (auto i = 0;
+	 i < m_linetable->nitems && m_linetable->item[i].line <= line; i++)
+      {
+	if (m_linetable->item[i].line == line)
+	  {
+	    auto k = i + 1;
+	    while (k < m_linetable->nitems
+		   && m_linetable->item[k].line == line)
+	      k++;
+	    const linetable_entry *begin = m_linetable->item + i;
+	    const linetable_entry *end = m_linetable->item + k;
+	    return gdb::array_view<const linetable_entry>{ begin, end };
+	  }
+      }
+    return {};
+  }
+
   void set_linetable (const struct linetable *linetable)
   {
     m_linetable = linetable;
@@ -2318,6 +2342,9 @@ struct symtab_and_line
      information is not available.  */
   int line = 0;
 
+  /* See comments on line number. */
+  int col = 0;
+
   CORE_ADDR pc = 0;
   CORE_ADDR end = 0;
   bool explicit_pc = false;
-- 
2.40.1

Re: [PATCH v1] [gdb/infcmd]: Add next-expression command

[Eli Zaretskii]

> Cc: Simon Farre <simon.farre.cx@gmail.com>
> Date: Tue, 16 May 2023 11:47:11 +0200
> From: Simon Farre via Gdb-patches <gdb-patches@sourceware.org>
> 
>  gdb/NEWS               |   6 ++
>  gdb/buildsym-legacy.c  |   4 +-
>  gdb/buildsym-legacy.h  |   2 +-
>  gdb/buildsym.c         |   3 +-
>  gdb/buildsym.h         |   4 +-
>  gdb/coffread.c         |   4 +-
>  gdb/dbxread.c          |   6 +-
>  gdb/doc/gdb.texinfo    |  16 +++++-
>  gdb/doc/python.texi    |   5 ++
>  gdb/dwarf2/read.c      |  31 ++++++----
>  gdb/infcmd.c           | 128 ++++++++++++++++++++++++++++++++++++++++-
>  gdb/mdebugread.c       |   2 +-
>  gdb/python/py-symtab.c |  25 +++++---
>  gdb/stack.c            |   7 +++
>  gdb/symtab.c           |   1 +
>  gdb/symtab.h           |  27 +++++++++
>  16 files changed, 238 insertions(+), 33 deletions(-)

Thanks.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,12 @@
>  
>  *** Changes since GDB 13
>  
> +* Added the "next-expression" ("ne") command that source-level steps to
> +  columns if the required debug info metadata was emitted by the compiler.
> +

This is not the usual style of NEWS when describing new commands.  We
usually say 'New command "next-expression" which ...".

Also, I don't think I understand this description.  What did you mean
by "source-level steps to columns"?

> +* Added column attribute to Python type Symtab_and_line to reflect the added
> +  column field on "linetable_entry" which is used in "next-expression" command.
> +

The usual style is something like "New 'column' attribute of Python
type Symtab_and_line...".

> +@kindex next-expression
> +@kindex ne @r{(@code{next-expression})}
> +@item next-expression @r{[}@var{count}@r{]}
> +Continue to the next expression or statement on the current source line

This begs the question what happens if we are already on the last
expression or statement.

> +in the current (innermost) stack frame. This is a source-level command
> +and as such requires that debug information was emitted by the compiler. If
> +no such debug information could be found this command defaults to
> +@code{next}.

It would be good to mention the version of DWARF2 that supports the
necessary information, and perhaps also the compilers known to emit
it.  At least for GCC, I'd certainly like to see this in the manual.

> +An argument @var{count} is a repeat count, as for @code{next}. If
   ^^^^^^^^^^^^^^^^^^^^^^^                                      ^^
"The argument @var{count}".  Also, please leave two spaces between
sentences, per our conventions.

> +repeat count is larger than amount of expressions on the current source line
> +the command will default to the @code{next} command.

I suggest to reword:

  If the current source line doesn't have enough expressions to
  satisfy @var{count}, this command will do the same as @code{next}.

> @@ -31313,7 +31327,7 @@ An -exec-until or similar CLI command was accomplished.
>  @item watchpoint-scope
>  A watchpoint has gone out of scope.
>  @item end-stepping-range
> -An -exec-next, -exec-next-instruction, -exec-step, -exec-step-instruction or 
> +An -exec-next, -exec-next-instruction, -exec-step, -exec-step-instruction or
>  similar CLI command was accomplished.
>  @item exited-signalled 
>  The inferior exited because of a signal.

This hunk is an unrelated whitespace change.

> +@defvar Symtab_and_line.column
> +Indicates the current column number for this object. This
> +attribute is not writeable.                        ^^

Two spaces there.  More importantly, what does current column measure
in this case?  Is that a character number from beginning of source
line, a byte number since the beginning of line, or the visual column
(which can be different from the character number if some characters
are double-width)?  How many columns is a TAB?

IOW, we must explain the manual what we mean by "column" here.  It
isn't trivial.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH v1] [gdb/infcmd]: Add next-expression command

[Simon Farre]

[-- Attachment #1: Type: text/plain, Size: 2743 bytes --]

Hi Eli, thanks for your review,

I'll change the NEWS message in in next patch as well as correct
any documentation formatting errors as well as reword phrases that you've
requested
to be changed.

> Also, I don't think I understand this description.  What did you mean by
"source-level steps to columns"?

I was attempting to signal that this is a "pure" source-level command, i.e.
that it's not
like "step", "stepi", or "nexti". After some discussion on IRC with Andrew
Burgess,
he pointed out to me that these commands are not supposed to work like this
new proposed command
because they were not "source level commands" but "instruction level
commands" so I figured I'd run
with that distinction/definition.

> This begs the question what happens if we are already on the last
expression or statement.

If we're on the last expression or statement on a source line, we'll step
to the first
expression on the next line (i.e. fall back to normal `next` behavior).
This first patch
explicitly only allows stepping to expressions on the same line and if it
can't find a column=N+1
in the linetable it will fall back on the `next` command; and as such won't
introduce new edge cases;
it will just call existing code that's battle tested.

> It would be good to mention the version of DWARF2 that supports the
> necessary information, and perhaps also the compilers known to emit
> it.  At least for GCC, I'd certainly like to see this in the manual.

I actually wrote this but decided to remove it which was something along
the lines of
"Column metadata has been supported since DWARF 2.0 (1992) and as such seem
to be
emitted by a lot of compilers I've tested, like g++, clang, rustc etc."

Where would you want me to put the information about compilers and
compatibility with emitting
column data?

> Two spaces there.  More importantly, what does current column measure
> in this case?  Is that a character number from beginning of source
> line, a byte number since the beginning of line, or the visual column
> (which can be different from the character number if some characters
> are double-width)?  How many columns is a TAB?

> IOW, we must explain the manual what we mean by "column" here.  It
> isn't trivial.

I have tested two different versions of GCC, clang and 1 version of the
rust compiler
and they all emit "columns" on a byte-level. So a smilie face with 4 bytes,
will be 4 columns.
I expect this to be the case for all DWARF-compliant compilers. Where would
you like
me to put this information? I could not find any information about this in
the DWARF standard
but seeing as how the two major C/C++ compilers (as well as the Rust
compiler)
emit it as such, then I think it's safe to assume this is how it's intended
to be?

Re: [PATCH v1] [gdb/infcmd]: Add next-expression command

[Eli Zaretskii]

> From: Simon Farre <simon.farre.cx@gmail.com>
> Date: Thu, 18 May 2023 13:12:23 +0200
> Cc: Simon Farre via Gdb-patches <gdb-patches@sourceware.org>
> 
> > Also, I don't think I understand this description.  What did you mean by "source-level steps to
> columns"?
> 
> I was attempting to signal that this is a "pure" source-level command, i.e. that it's not 
> like "step", "stepi", or "nexti". After some discussion on IRC with Andrew Burgess,
> he pointed out to me that these commands are not supposed to work like this new proposed
> command
> because they were not "source level commands" but "instruction level commands" so I figured I'd run
> with that distinction/definition.

The "source-level" part is clear, but what about "stepping to
columns"?  The command steps by expressions, not by columns.

> > It would be good to mention the version of DWARF2 that supports the 
> > necessary information, and perhaps also the compilers known to emit
> > it.  At least for GCC, I'd certainly like to see this in the manual.
> 
> I actually wrote this but decided to remove it which was something along the lines of
> "Column metadata has been supported since DWARF 2.0 (1992) and as such seem to be
> emitted by a lot of compilers I've tested, like g++, clang, rustc etc."
> 
> Where would you want me to put the information about compilers and compatibility with emitting
> column data?

There's no need for too much detail, if this is an old feature.  Just
say something like

  The support for expressions was added to DWARF2 30 years ago, so any
  compiler which emits DWARF2 debug info should nowadays produce the
  information required by this command.  E.g., GCC supports this since
  version <put here the version of GCC>.

(Assuming it is true that every compiler released reasonably recently
will produce this as part of DWARF debug info.)

> > Two spaces there.  More importantly, what does current column measure
> > in this case?  Is that a character number from beginning of source
> > line, a byte number since the beginning of line, or the visual column
> > (which can be different from the character number if some characters
> > are double-width)?  How many columns is a TAB?
> 
> > IOW, we must explain the manual what we mean by "column" here.  It
> > isn't trivial.
> 
> I have tested two different versions of GCC, clang and 1 version of the rust compiler
> and they all emit "columns" on a byte-level. So a smilie face with 4 bytes, will be 4 columns.
> I expect this to be the case for all DWARF-compliant compilers. Where would you like
> me to put this information? I could not find any information about this in the DWARF standard
> but seeing as how the two major C/C++ compilers (as well as the Rust compiler) 
> emit it as such, then I think it's safe to assume this is how it's intended to be?

We need to explain in the manual that the "columns" actually are (or
are likely to be) byte offsets from the beginning of the source line.

Re: [PATCH v1] [gdb/infcmd]: Add next-expression command

[Simon Farre]

[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]


>   The "source-level" part is clear, but what about "stepping to
> columns"?  The command steps by expressions, not by columns.

Well, the column metadata is essentially emitted for expressions and 
statements (or "logical breakpoint locations"), but the actual meta data 
that is recorded is by column (and a statement flag), so we can't really 
be _entirely_ sure that what we're stepping to is an expression or 
statement, even though that is what it's /intended/ to represent. Which 
is why I used column and expression interchangeably.

Perhaps further explanation is required here, or possibly even renaming 
the feature to `next-column` and `step-column` for the two variants that 
I've implemented; where the `step` variant behaves similarly to what the 
normal step/next counterparts do; i.e. if there's an inline frame on the 
same source file (like a lambda or callback) then step will step into 
it, whereas next will step over it. The 2 variants will be in the 
upcoming v3.

> There's no need for too much detail, if this is an old feature.  Just
> say something like
>
>    The support for expressions was added to DWARF2 30 years ago, so any
>    compiler which emits DWARF2 debug info should nowadays produce the
>    information required by this command.  E.g., GCC supports this since
>    version <put here the version of GCC>.
>
> (Assuming it is true that every compiler released reasonably recently
> will produce this as part of DWARF debug info.)
Right, I've only tested with the latest, but I'm pretty sure that the 
last 4-5 years of compilers at least emit column meta data. I've tested 
gcc-12, clang-14 and the latest rustc compiler (I know rustc has emitted 
it for at _least_ 5 years). I'm building gcc-8 (5+ years) as I write 
this and will test that. Where in the manual would you like me to put 
the above paragraph? Together with the documentation of the command or 
somewhere else? And the same question goes for
> We need to explain in the manual that the "columns" actually are (or
> are likely to be) byte offsets from the beginning of the source line.

Re: [PATCH v1] [gdb/infcmd]: Add next-expression command

[Eli Zaretskii]

> Date: Wed, 24 May 2023 10:18:33 +0200
> Cc: gdb-patches@sourceware.org
> From: Simon Farre <simon.farre.cx@gmail.com>
> 
>   The "source-level" part is clear, but what about "stepping to
> columns"?  The command steps by expressions, not by columns.
> 
> Well, the column metadata is essentially emitted for expressions and statements (or "logical
> breakpoint locations"), but the actual meta data that is recorded is by column (and a statement flag),
> so we can't really be _entirely_ sure that what we're stepping to is an expression or statement, even
> though that is what it's intended to represent. Which is why I used column and expression
> interchangeably. 

AFAIU, the fact that the metadata is defined in terms of columns is an
implementation detail of no particular interest to GDB users.  On the
user level, this is supposed to step by expressions, is that right?

Perhaps, to avoid too many miscommunications, you could point me to
the relevant description in the DWARF2 spec, so I could read it, and
then we could talk based on the same information?

> Perhaps further explanation is required here, or possibly even renaming the feature to `next-column`
> and `step-column` for the two variants that I've implemented; where the `step` variant behaves similarly
> to what the normal step/next counterparts do; i.e. if there's an inline frame on the same source file (like
> a lambda or callback) then step will step into it, whereas next will step over it. The 2 variants will be in
> the upcoming v3. 

OK, but I think we still need to understand better the purpose of the
feature, and specifically what kind of information the DWARF2 data is
supposed to provide here.

>  There's no need for too much detail, if this is an old feature.  Just
> say something like
> 
>   The support for expressions was added to DWARF2 30 years ago, so any
>   compiler which emits DWARF2 debug info should nowadays produce the
>   information required by this command.  E.g., GCC supports this since
>   version <put here the version of GCC>.
> 
> (Assuming it is true that every compiler released reasonably recently
> will produce this as part of DWARF debug info.)
> 
> Right, I've only tested with the latest, but I'm pretty sure that the last 4-5 years of compilers at least emit
> column meta data. I've tested gcc-12, clang-14 and the latest rustc compiler (I know rustc has emitted
> it for at _least_ 5 years). I'm building gcc-8 (5+ years) as I write this and will test that. Where in the
> manual would you like me to put the above paragraph? Together with the documentation of the
> command or somewhere else? And the same question goes for 
> 
>  We need to explain in the manual that the "columns" actually are (or
> are likely to be) byte offsets from the beginning of the source line.

Both should be explained where these new commands are described,
because that is needed to fully understand what the commands do and
how to use them.

Re: [PATCH v1] [gdb/infcmd]: Add next-expression command

[Simon Farre]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]


> AFAIU, the fact that the metadata is defined in terms of columns is an
> implementation detail of no particular interest to GDB users.  On the
> user level, this is supposed to step by expressions, is that right?
>
> Perhaps, to avoid too many miscommunications, you could point me to
> the relevant description in the DWARF2 spec, so I could read it, and
> then we could talk based on the same information?
Unfortunately the spec is not too descriptive, it isn't until recent 
years (probably in the last ten-ish) that people (compilers/languages) 
has gotten around to understanding the point of the "Line Number 
Program", that records the line & column meta data, particulary when it 
comes to the column meta data.

There's a new proposal for further extending the LNP that discusses it 
some what more in depth and /perhaps/ that will give you some insight, 
it can be found here: https://dwarfstd.org/issues/140906.1.html (it was 
provided to me by jemarch after discussion on the IRC channel)

Otherwise, the "Line Number Program" can be read about in the latest (as 
well as earlier) specs: https://dwarfstd.org/doc/DWARF5.pdf under the 
chapter "6. Other Debugging Information" (specifically 6.2, the Line 
Number Program). But it won't explain much to you on how this is used by 
compilers.

The DWARF2-5 specs are of few words when it comes to columns, so one has 
to resort to "3rd party" (the proposal link, above) to get any 
explanations of what compilers actually do (or should do) but as 
previously said, GCC, Clang, and Rust all does it, though GCC emits 
somewhat faulty column meta data, see the issue I've filed on bugzilla 
for GCC after discussions on the GDB irc channel: 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109902

The best description of what the column meta data is meant to do, is 
serve as "logical breakpoint locations" on a source code line; this 
includes statements and expressions - though it's never _explicitly_ 
worded like that in the spec (unfortunately, DWARF is not as meticulous 
with it's documentation and describing the point of certain aspects of 
the spec, as "we are", here in the GDB community).

Regards,

Simon