[PATCH 00/23] More breakpoints cleanups

[PATCH 00/23] More breakpoints cleanups

[Pedro Alves]

The main goal of this series was to eliminate init_raw_breakpoint.

It slowly tweaks a number of things to be able to do that.

Along the way, it converts the remaining init_foo functions to proper
ctors, and tweaks the class hierarchy a bit such that all code
breakpoints inherit from base_breakpoint.  In the end, it proposes
renaming base_breakpoint to code_breakpoint.

By inspection, I ran into a couple bugs around momentary breakpoints.
There are a couples patches in here to fix them.

I also noticed that nothing currently exercises the paths that make
create_breakpoints_sal actually create more than one breakpoint.  One
of the patches here is about adding a testcase for that.

See the individual patches for details.

Pedro Alves (23):
  add_location_to_breakpoint -> breakpoint::add_location
  Make structs breakpoint/base_breakpoint/catchpoint be abstract
  ranged_breakpoint: don't use init_raw_breakpoint
  ranged_breakpoint, use install_breakpoint
  ranged_breakpoint - move initialization to ctor
  Make a few functions work with base_breakpoint instead of breakpoint
  More breakpoint_ops parameter elimination
  Remove "internal" parameter from a couple functions
  init_breakpoint_sal -> base_breakpoint::base_breakpoint
  Make ada_catchpoint_location's owner ctor parameter be ada_catchpoint
  Convert init_ada_exception_catchpoint to a ctor
  Refactor set_internal_breakpoint / internal_breakpoint ctor
  Refactor momentary breakpoints, eliminate
    set_raw_breakpoint{,_without_location}
  Make exception_catchpoint inherit base_breakpoint instead of
    catchpoint
  Make breakpoint_address_bits look at the location kind
  Make catchpoint inherit breakpoint, eliminate init_raw_breakpoint
  Move common bits of catchpoint/exception_catchpoint to breakpoint's
    ctor
  Move add_location(sal) to base_breakpoint
  Add/tweak intro comments of struct breakpoint and several subclasses
  Momentary breakpoints should have no breakpoint number
  Make sure momentary breakpoints are always thread-specific
  Test "set multiple-symbols on" creating multiple breakpoints
  Rename base_breakpoint -> code_breakpoint

 gdb/ada-lang.c                     |  81 ++-
 gdb/break-catch-syscall.c          |   6 +-
 gdb/break-catch-throw.c            |  14 +-
 gdb/breakpoint.c                   | 805 ++++++++++++-----------------
 gdb/breakpoint.h                   |  93 ++--
 gdb/elfread.c                      |   6 +-
 gdb/mi/mi-cmd-break.c              |   4 +-
 gdb/minsyms.c                      |   4 +-
 gdb/python/py-finishbreakpoint.c   |   2 +-
 gdb/symtab.h                       |   5 +-
 gdb/testsuite/gdb.cp/ovldbreak.exp | 100 +++-
 11 files changed, 546 insertions(+), 574 deletions(-)


base-commit: e90601a4f1af82ff5350797ddc54e24efd731535
-- 
2.36.0

[PATCH 01/23] add_location_to_breakpoint -> breakpoint::add_location

[Pedro Alves]

Make add_location_to_breakpoint be a method of struct breakpoint.

A patch later in the series will move this to base_breakpoint, but for
now, it needs to be here.

Change-Id: I5bdc2ec1a7c2d66f26f51bf6f6adc8384a90b129
---
 gdb/breakpoint.c | 70 +++++++++++++++++++++++-------------------------
 gdb/breakpoint.h |  3 +++
 2 files changed, 36 insertions(+), 37 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 9abc1443d96..385c92a6e7b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -114,9 +114,6 @@ static void mention (const breakpoint *);
 
 static struct breakpoint *set_raw_breakpoint_without_location (struct gdbarch *,
 							       enum bptype);
-static struct bp_location *add_location_to_breakpoint (struct breakpoint *,
-						       const struct symtab_and_line *);
-
 /* This function is used in gdbtk sources and thus can not be made
    static.  */
 static struct breakpoint *set_raw_breakpoint (struct gdbarch *gdbarch,
@@ -7308,7 +7305,7 @@ static void
 init_raw_breakpoint (struct breakpoint *b, struct symtab_and_line sal,
 		     enum bptype bptype)
 {
-  add_location_to_breakpoint (b, &sal);
+  b->add_location (sal);
 
   if (bptype != bp_catchpoint)
     gdb_assert (sal.pspace != NULL);
@@ -8109,16 +8106,15 @@ handle_automatic_hardware_breakpoints (bp_location *bl)
     }
 }
 
-static struct bp_location *
-add_location_to_breakpoint (struct breakpoint *b,
-			    const struct symtab_and_line *sal)
+bp_location *
+breakpoint::add_location (const symtab_and_line &sal)
 {
-  struct bp_location *loc, **tmp;
+  struct bp_location *new_loc, **tmp;
   CORE_ADDR adjusted_address;
-  struct gdbarch *loc_gdbarch = get_sal_arch (*sal);
+  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
 
   if (loc_gdbarch == NULL)
-    loc_gdbarch = b->gdbarch;
+    loc_gdbarch = gdbarch;
 
   /* Adjust the breakpoint's address prior to allocating a location.
      Once we call allocate_location(), that mostly uninitialized
@@ -8127,31 +8123,31 @@ add_location_to_breakpoint (struct breakpoint *b,
      not want its scan of the location chain to find a breakpoint and
      location that's only been partially initialized.  */
   adjusted_address = adjust_breakpoint_address (loc_gdbarch,
-						sal->pc, b->type);
+						sal.pc, type);
 
   /* Sort the locations by their ADDRESS.  */
-  loc = b->allocate_location ();
-  for (tmp = &(b->loc); *tmp != NULL && (*tmp)->address <= adjusted_address;
+  new_loc = allocate_location ();
+  for (tmp = &(loc); *tmp != NULL && (*tmp)->address <= adjusted_address;
        tmp = &((*tmp)->next))
     ;
-  loc->next = *tmp;
-  *tmp = loc;
-
-  loc->requested_address = sal->pc;
-  loc->address = adjusted_address;
-  loc->pspace = sal->pspace;
-  loc->probe.prob = sal->prob;
-  loc->probe.objfile = sal->objfile;
-  gdb_assert (loc->pspace != NULL);
-  loc->section = sal->section;
-  loc->gdbarch = loc_gdbarch;
-  loc->line_number = sal->line;
-  loc->symtab = sal->symtab;
-  loc->symbol = sal->symbol;
-  loc->msymbol = sal->msymbol;
-  loc->objfile = sal->objfile;
-
-  set_breakpoint_location_function (loc);
+  new_loc->next = *tmp;
+  *tmp = new_loc;
+
+  new_loc->requested_address = sal.pc;
+  new_loc->address = adjusted_address;
+  new_loc->pspace = sal.pspace;
+  new_loc->probe.prob = sal.prob;
+  new_loc->probe.objfile = sal.objfile;
+  gdb_assert (new_loc->pspace != NULL);
+  new_loc->section = sal.section;
+  new_loc->gdbarch = loc_gdbarch;
+  new_loc->line_number = sal.line;
+  new_loc->symtab = sal.symtab;
+  new_loc->symbol = sal.symbol;
+  new_loc->msymbol = sal.msymbol;
+  new_loc->objfile = sal.objfile;
+
+  set_breakpoint_location_function (new_loc);
 
   /* While by definition, permanent breakpoints are already present in the
      code, we don't mark the location as inserted.  Normally one would expect
@@ -8166,10 +8162,10 @@ add_location_to_breakpoint (struct breakpoint *b,
      (If GDB later needs to continue execution past the permanent breakpoint,
      it manually increments the PC, thus avoiding executing the breakpoint
      instruction.)  */
-  if (bp_loc_is_permanent (loc))
-    loc->permanent = 1;
+  if (bp_loc_is_permanent (new_loc))
+    new_loc->permanent = 1;
 
-  return loc;
+  return new_loc;
 }
 \f
 
@@ -8375,7 +8371,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	}
       else
 	{
-	  loc = add_location_to_breakpoint (b, &sal);
+	  loc = b->add_location (sal);
 	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
 	    loc->inserted = 1;
 	}
@@ -12646,7 +12642,7 @@ update_breakpoint_locations (struct breakpoint *b,
 
       switch_to_program_space_and_thread (sal.pspace);
 
-      new_loc = add_location_to_breakpoint (b, &sal);
+      new_loc = b->add_location (sal);
 
       /* Reparse conditions, they might contain references to the
 	 old symtab.  */
@@ -13537,7 +13533,7 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
   sal.pc = pc;
   sal.section = find_pc_overlay (pc);
   sal.explicit_pc = 1;
-  add_location_to_breakpoint (tp->control.single_step_breakpoints, &sal);
+  tp->control.single_step_breakpoints->add_location (sal);
 
   update_global_location_list (UGLL_INSERT);
 }
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index ac738fd7c2d..fb8651c55c3 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -739,6 +739,9 @@ struct breakpoint
     /* Nothing to do.  */
   }
 
+  /* Add a location for SAL to this breakpoint.  */
+  bp_location *add_location (const symtab_and_line &sal);
+
   /* Return a range of this breakpoint's locations.  */
   bp_location_range locations () const;
 
-- 
2.36.0

[PATCH 02/23] Make structs breakpoint/base_breakpoint/catchpoint be abstract

[Pedro Alves]

You should never instanciate these types directly.

Change-Id: I8086c74c415eadbd44924bb0ef20f34b5b97ee6f
---
 gdb/breakpoint.c | 15 +++++++++++++++
 gdb/breakpoint.h |  6 +++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 385c92a6e7b..8f6794511e7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -252,6 +252,21 @@ static const struct breakpoint_ops tracepoint_probe_breakpoint_ops =
   create_breakpoints_sal,
 };
 
+/* Implementation of abstract dtors.  These must exist to satisfy the
+   linker.  */
+
+breakpoint::~breakpoint ()
+{
+}
+
+base_breakpoint::~base_breakpoint ()
+{
+}
+
+catchpoint::~catchpoint ()
+{
+}
+
 /* The structure to be used in regular breakpoints.  */
 struct ordinary_breakpoint : public base_breakpoint
 {
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fb8651c55c3..b7e3b4dc6a1 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -631,7 +631,7 @@ struct breakpoint
 
   DISABLE_COPY_AND_ASSIGN (breakpoint);
 
-  virtual ~breakpoint () = default;
+  virtual ~breakpoint () = 0;
 
   /* Allocate a location for this breakpoint.  */
   virtual struct bp_location *allocate_location ();
@@ -862,6 +862,8 @@ struct base_breakpoint : public breakpoint
 {
   using breakpoint::breakpoint;
 
+  ~base_breakpoint () override = 0;
+
   void re_set () override;
   int insert_location (struct bp_location *) override;
   int remove_location (struct bp_location *,
@@ -1023,6 +1025,8 @@ struct catchpoint : public base_breakpoint
      COND_STRING is not NULL, then store it in the breakpoint.  */
   catchpoint (struct gdbarch *gdbarch, bool temp, const char *cond_string);
 
+  ~catchpoint () override = 0;
+
   void re_set () override
   {
     /* For catchpoints, the default is to do nothing.  */
-- 
2.36.0

[PATCH 03/23] ranged_breakpoint: don't use init_raw_breakpoint

[Pedro Alves]

ranged_breakpoint's ctor already sets the breakpoint's type to
bp_hardware_breakpoint.

Since this is a "regular" breakpoint, b->pspace should remain NULL.

Thus, the only thing init_raw_breakpoint is needed for, is to add the
breakpoint's location.  Do that directly.

Change-Id: I1505de94c3919881c2b300437e2c0da9b05f76bd
---
 gdb/breakpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8f6794511e7..f9332e1f998 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9439,7 +9439,7 @@ break_range_command (const char *arg, int from_tty)
 
   /* Now set up the breakpoint.  */
   std::unique_ptr<breakpoint> br (new ranged_breakpoint (get_current_arch ()));
-  init_raw_breakpoint (br.get (), sal_start, bp_hardware_breakpoint);
+  br->add_location (sal_start);
   b = add_to_breakpoint_chain (std::move (br));
 
   set_breakpoint_count (breakpoint_count + 1);
-- 
2.36.0

[PATCH 04/23] ranged_breakpoint, use install_breakpoint

[Pedro Alves]

This commit replaces a chunk of code in break_range_command by an
equivalent call to install_breakpoint.

Change-Id: I31c06cabd36f5be91740aab029265f678aa78e35
---
 gdb/breakpoint.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f9332e1f998..ced976ca39d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9356,7 +9356,6 @@ break_range_command (const char *arg, int from_tty)
   struct linespec_result canonical_start, canonical_end;
   int bp_count, can_use_bp, length;
   CORE_ADDR end;
-  struct breakpoint *b;
 
   /* We don't support software ranged breakpoints.  */
   if (target_ranged_break_num_registers () < 0)
@@ -9440,18 +9439,12 @@ break_range_command (const char *arg, int from_tty)
   /* Now set up the breakpoint.  */
   std::unique_ptr<breakpoint> br (new ranged_breakpoint (get_current_arch ()));
   br->add_location (sal_start);
-  b = add_to_breakpoint_chain (std::move (br));
-
-  set_breakpoint_count (breakpoint_count + 1);
-  b->number = breakpoint_count;
-  b->disposition = disp_donttouch;
-  b->location = std::move (start_location);
-  b->location_range_end = std::move (end_location);
-  b->loc->length = length;
+  br->disposition = disp_donttouch;
+  br->location = std::move (start_location);
+  br->location_range_end = std::move (end_location);
+  br->loc->length = length;
 
-  mention (b);
-  gdb::observers::breakpoint_created.notify (b);
-  update_global_location_list (UGLL_MAY_INSERT);
+  install_breakpoint (false, std::move (br), true);
 }
 
 /*  Return non-zero if EXP is verified as constant.  Returned zero
-- 
2.36.0

[PATCH 05/23] ranged_breakpoint - move initialization to ctor

[Pedro Alves]

Move initialization of ranged_breakpoint's fields to its ctor.

Change-Id: If7b842861f3cc6a429ea329d45598b5852283ba3
---
 gdb/breakpoint.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ced976ca39d..1c1dbfb3ad7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -317,9 +317,20 @@ struct dprintf_breakpoint : public ordinary_breakpoint
 /* Ranged breakpoints.  */
 struct ranged_breakpoint : public ordinary_breakpoint
 {
-  explicit ranged_breakpoint (struct gdbarch *gdbarch)
+  explicit ranged_breakpoint (struct gdbarch *gdbarch,
+			      const symtab_and_line &sal_start,
+			      int length,
+			      event_location_up start_location,
+			      event_location_up end_location)
     : ordinary_breakpoint (gdbarch, bp_hardware_breakpoint)
   {
+    bp_location *bl = add_location (sal_start);
+    bl->length = length;
+
+    disposition = disp_donttouch;
+
+    location = std::move (start_location);
+    location_range_end = std::move (end_location);
   }
 
   int breakpoint_hit (const struct bp_location *bl,
@@ -9436,13 +9447,13 @@ break_range_command (const char *arg, int from_tty)
       return;
     }
 
-  /* Now set up the breakpoint.  */
-  std::unique_ptr<breakpoint> br (new ranged_breakpoint (get_current_arch ()));
-  br->add_location (sal_start);
-  br->disposition = disp_donttouch;
-  br->location = std::move (start_location);
-  br->location_range_end = std::move (end_location);
-  br->loc->length = length;
+  /* Now set up the breakpoint and install it.  */
+
+  std::unique_ptr<breakpoint> br
+    (new ranged_breakpoint (get_current_arch (),
+			    sal_start, length,
+			    std::move (start_location),
+			    std::move (end_location)));
 
   install_breakpoint (false, std::move (br), true);
 }
-- 
2.36.0

[PATCH 06/23] Make a few functions work with base_breakpoint instead of breakpoint

[Pedro Alves]

This makes tracepoints inherit from base_breakpoint, since their
locations are code locations.  If we do that, then we can eliminate
tracepoint::re_set and tracepoint::decode_location, as they are doing
the same as the base_breakpoint implementations.

With this, all breakpoint types created by new_breakpoint_from_type
are code breakpoints, i.e., base_breakpoint subclasses, and thus we
can make it return a base_breakpoint pointer.

Finally, init_breakpoint_sal can take a base_breakpoint pointer as
"self" pointer too.  This will let us convert this function to a
base_breakpoint ctor in a following patch.

Change-Id: I3a4073ff1a4c865f525588095c18dc42b744cb54
---
 gdb/breakpoint.c | 30 ++++++------------------------
 gdb/breakpoint.h |  9 ++-------
 2 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1c1dbfb3ad7..3df3eb8979d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1258,10 +1258,10 @@ is_tracepoint (const struct breakpoint *b)
 /* Factory function to create an appropriate instance of breakpoint given
    TYPE.  */
 
-static std::unique_ptr<breakpoint>
+static std::unique_ptr<base_breakpoint>
 new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type)
 {
-  breakpoint *b;
+  base_breakpoint *b;
 
   switch (type)
     {
@@ -1317,7 +1317,7 @@ new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type)
       gdb_assert_not_reached ("invalid type");
     }
 
-  return std::unique_ptr<breakpoint> (b);
+  return std::unique_ptr<base_breakpoint> (b);
 }
 
 /* A helper function that validates that COMMANDS are valid for a
@@ -8297,7 +8297,7 @@ update_dprintf_commands (const char *args, int from_tty,
    "address location" from the address in the SAL.  */
 
 static void
-init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
+init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
 		     gdb::array_view<const symtab_and_line> sals,
 		     event_location_up &&location,
 		     gdb::unique_xmalloc_ptr<char> filter,
@@ -8359,7 +8359,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	  if (type == bp_static_tracepoint
 	      || type == bp_static_marker_tracepoint)
 	    {
-	      struct tracepoint *t = (struct tracepoint *) b;
+	      auto *t = static_cast <struct tracepoint *> (b);
 	      struct static_tracepoint_marker marker;
 
 	      if (strace_marker_p (b))
@@ -8453,7 +8453,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       int enabled, int internal, unsigned flags,
 		       int display_canonical)
 {
-  std::unique_ptr<breakpoint> b = new_breakpoint_from_type (gdbarch, type);
+  std::unique_ptr<base_breakpoint> b = new_breakpoint_from_type (gdbarch, type);
 
   init_breakpoint_sal (b.get (), gdbarch,
 		       sals, std::move (location),
@@ -11952,14 +11952,6 @@ bkpt_probe_decode_location (struct breakpoint *b,
   return sals;
 }
 
-/* The breakpoint_ops structure to be used in tracepoints.  */
-
-void
-tracepoint::re_set ()
-{
-  breakpoint_re_set_default (this);
-}
-
 int
 tracepoint::breakpoint_hit (const struct bp_location *bl,
 			    const address_space *aspace, CORE_ADDR bp_addr,
@@ -12034,16 +12026,6 @@ tracepoint::print_recreate (struct ui_file *fp) const
     gdb_printf (fp, "  passcount %d\n", pass_count);
 }
 
-std::vector<symtab_and_line>
-tracepoint::decode_location (struct event_location *location,
-			     struct program_space *search_pspace)
-{
-  if (event_location_type (location) == PROBE_LOCATION)
-    return bkpt_probe_decode_location (this, location, search_pspace);
-
-  return decode_location_default (this, location, search_pspace);
-}
-
 /* Virtual table for tracepoints on static probes.  */
 
 static void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index b7e3b4dc6a1..48ceceabb3d 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -974,21 +974,16 @@ extern bool is_exception_catchpoint (breakpoint *bp);
 /* An instance of this type is used to represent all kinds of
    tracepoints.  */
 
-struct tracepoint : public breakpoint
+struct tracepoint : public base_breakpoint
 {
-  using breakpoint::breakpoint;
+  using base_breakpoint::base_breakpoint;
 
-  void re_set () override;
   int breakpoint_hit (const struct bp_location *bl,
 		      const address_space *aspace, CORE_ADDR bp_addr,
 		      const target_waitstatus &ws) override;
   void print_one_detail (struct ui_out *uiout) const override;
   void print_mention () const override;
   void print_recreate (struct ui_file *fp) const override;
-  std::vector<symtab_and_line> decode_location
-       (struct event_location *location,
-	struct program_space *search_pspace) override;
-
 
   /* Number of times this tracepoint should single-step and collect
      additional data.  */
-- 
2.36.0

[PATCH 07/23] More breakpoint_ops parameter elimination

[Pedro Alves]

Remove breakpoint_ops parameters from a few functions that don't need
it.

Change-Id: Ifcf5e1cc688184acbf5e19b8ea60138ebe63cf28
---
 gdb/breakpoint.c | 16 +++++++---------
 gdb/breakpoint.h |  3 +--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3df3eb8979d..c84614eeb02 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -100,7 +100,6 @@ static void create_breakpoints_sal (struct gdbarch *,
 				    enum bptype,
 				    enum bpdisp, int, int,
 				    int,
-				    const struct breakpoint_ops *,
 				    int, int, int, unsigned);
 
 static std::vector<symtab_and_line> decode_location_default
@@ -8305,7 +8304,7 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
 		     gdb::unique_xmalloc_ptr<char> extra_string,
 		     enum bptype type, enum bpdisp disposition,
 		     int thread, int task, int ignore_count,
-		     const struct breakpoint_ops *ops, int from_tty,
+		     int from_tty,
 		     int enabled, int internal, unsigned flags,
 		     int display_canonical)
 {
@@ -8449,7 +8448,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       gdb::unique_xmalloc_ptr<char> extra_string,
 		       enum bptype type, enum bpdisp disposition,
 		       int thread, int task, int ignore_count,
-		       const struct breakpoint_ops *ops, int from_tty,
+		       int from_tty,
 		       int enabled, int internal, unsigned flags,
 		       int display_canonical)
 {
@@ -8462,7 +8461,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       std::move (extra_string),
 		       type, disposition,
 		       thread, task, ignore_count,
-		       ops, from_tty,
+		       from_tty,
 		       enabled, internal, flags,
 		       display_canonical);
 
@@ -8491,7 +8490,7 @@ create_breakpoints_sal (struct gdbarch *gdbarch,
 			gdb::unique_xmalloc_ptr<char> extra_string,
 			enum bptype type, enum bpdisp disposition,
 			int thread, int task, int ignore_count,
-			const struct breakpoint_ops *ops, int from_tty,
+			int from_tty,
 			int enabled, int internal, unsigned flags)
 {
   if (canonical->pre_expanded)
@@ -8513,7 +8512,7 @@ create_breakpoints_sal (struct gdbarch *gdbarch,
 			     std::move (cond_string),
 			     std::move (extra_string),
 			     type, disposition,
-			     thread, task, ignore_count, ops,
+			     thread, task, ignore_count,
 			     from_tty, enabled, internal, flags,
 			     canonical->special_display);
     }
@@ -8997,7 +8996,7 @@ create_breakpoint (struct gdbarch *gdbarch,
 				   std::move (extra_string_copy),
 				   type_wanted,
 				   tempflag ? disp_del : disp_donttouch,
-				   thread, task, ignore_count, ops,
+				   thread, task, ignore_count,
 				   from_tty, enabled, internal, flags);
     }
   else
@@ -12128,7 +12127,6 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
 				      enum bpdisp disposition,
 				      int thread,
 				      int task, int ignore_count,
-				      const struct breakpoint_ops *ops,
 				      int from_tty, int enabled,
 				      int internal, unsigned flags)
 {
@@ -12153,7 +12151,7 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
 			   std::move (cond_string),
 			   std::move (extra_string),
 			   type_wanted, disposition,
-			   thread, task, ignore_count, ops,
+			   thread, task, ignore_count,
 			   from_tty, enabled, internal, flags,
 			   canonical->special_display);
       /* Given that its possible to have multiple markers with
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 48ceceabb3d..fb2bbd729c0 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -582,8 +582,7 @@ struct breakpoint_ops
 				  gdb::unique_xmalloc_ptr<char>,
 				  gdb::unique_xmalloc_ptr<char>,
 				  enum bptype, enum bpdisp, int, int,
-				  int, const struct breakpoint_ops *,
-				  int, int, int, unsigned);
+				  int, int, int, int, unsigned);
 };
 
 enum watchpoint_triggered
-- 
2.36.0

[PATCH 08/23] Remove "internal" parameter from a couple functions

[Pedro Alves]

None of init_breakpoint_sal, create_breakpoint_sal, and
strace_marker_create_breakpoints_sal make use of their "internal"
parameter, so remove it.

Change-Id: I943f3bb44717ade7a7b7547edf8f3ff3c37da435
---
 gdb/breakpoint.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c84614eeb02..1e21b337802 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8305,7 +8305,7 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
 		     enum bptype type, enum bpdisp disposition,
 		     int thread, int task, int ignore_count,
 		     int from_tty,
-		     int enabled, int internal, unsigned flags,
+		     int enabled, unsigned flags,
 		     int display_canonical)
 {
   int i;
@@ -8462,7 +8462,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       type, disposition,
 		       thread, task, ignore_count,
 		       from_tty,
-		       enabled, internal, flags,
+		       enabled, flags,
 		       display_canonical);
 
   install_breakpoint (internal, std::move (b), 0);
@@ -12152,7 +12152,7 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
 			   std::move (extra_string),
 			   type_wanted, disposition,
 			   thread, task, ignore_count,
-			   from_tty, enabled, internal, flags,
+			   from_tty, enabled, flags,
 			   canonical->special_display);
       /* Given that its possible to have multiple markers with
 	 the same string id, if the user is creating a static
-- 
2.36.0

[PATCH 09/23] init_breakpoint_sal -> base_breakpoint::base_breakpoint

[Pedro Alves]

This converts init_breakpoint_sal to a base_breakpoint constructor.

It removes a use of init_raw_breakpoint.

To avoid manually adding a bunch of parameters to
new_breakpoint_from_type, and manually passing them down to the
constructors of a number of different base_breakpoint subclasses, make
new_breakpoint_from_type a variable template function.

Change-Id: I4cc24133ac4c292f547289ec782fc78e5bbe2510
---
 gdb/breakpoint.c | 239 +++++++++++++++++++++++------------------------
 gdb/breakpoint.h |  16 ++++
 2 files changed, 133 insertions(+), 122 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1e21b337802..ce742b97fd5 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1257,8 +1257,10 @@ is_tracepoint (const struct breakpoint *b)
 /* Factory function to create an appropriate instance of breakpoint given
    TYPE.  */
 
+template<typename... Arg>
 static std::unique_ptr<base_breakpoint>
-new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type)
+new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type,
+			  Arg&&... args)
 {
   base_breakpoint *b;
 
@@ -1266,21 +1268,25 @@ new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type)
     {
     case bp_breakpoint:
     case bp_hardware_breakpoint:
-      b = new ordinary_breakpoint (gdbarch, type);
+      b = new ordinary_breakpoint (gdbarch, type,
+				   std::forward<Arg> (args)...);
       break;
 
     case bp_fast_tracepoint:
     case bp_static_tracepoint:
     case bp_tracepoint:
-      b = new tracepoint (gdbarch, type);
+      b = new tracepoint (gdbarch, type,
+			  std::forward<Arg> (args)...);
       break;
 
     case bp_static_marker_tracepoint:
-      b = new static_marker_tracepoint (gdbarch, type);
+      b = new static_marker_tracepoint (gdbarch, type,
+					std::forward<Arg> (args)...);
       break;
 
     case bp_dprintf:
-      b = new dprintf_breakpoint (gdbarch, type);
+      b = new dprintf_breakpoint (gdbarch, type,
+				  std::forward<Arg> (args)...);
       break;
 
     case bp_overlay_event:
@@ -1290,12 +1296,14 @@ new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type)
     case bp_thread_event:
     case bp_jit_event:
     case bp_shlib_event:
-      b = new internal_breakpoint (gdbarch, type);
+      b = new internal_breakpoint (gdbarch, type,
+				   std::forward<Arg> (args)...);
       break;
 
     case bp_longjmp:
     case bp_exception:
-      b = new longjmp_breakpoint (gdbarch, type);
+      b = new longjmp_breakpoint (gdbarch, type,
+				  std::forward<Arg> (args)...);
       break;
 
     case bp_watchpoint_scope:
@@ -1309,7 +1317,8 @@ new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type)
     case bp_call_dummy:
     case bp_until:
     case bp_std_terminate:
-      b = new momentary_breakpoint (gdbarch, type);
+      b = new momentary_breakpoint (gdbarch, type,
+				    std::forward<Arg> (args)...);
       break;
 
     default:
@@ -8290,23 +8299,19 @@ update_dprintf_commands (const char *args, int from_tty,
 	update_dprintf_command_list (b);
 }
 
-/* Create a breakpoint with SAL as location.  Use LOCATION
-   as a description of the location, and COND_STRING
-   as condition expression.  If LOCATION is NULL then create an
-   "address location" from the address in the SAL.  */
-
-static void
-init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
-		     gdb::array_view<const symtab_and_line> sals,
-		     event_location_up &&location,
-		     gdb::unique_xmalloc_ptr<char> filter,
-		     gdb::unique_xmalloc_ptr<char> cond_string,
-		     gdb::unique_xmalloc_ptr<char> extra_string,
-		     enum bptype type, enum bpdisp disposition,
-		     int thread, int task, int ignore_count,
-		     int from_tty,
-		     int enabled, unsigned flags,
-		     int display_canonical)
+base_breakpoint::base_breakpoint (struct gdbarch *gdbarch_,
+				  enum bptype type_,
+				  gdb::array_view<const symtab_and_line> sals,
+				  event_location_up &&location_,
+				  gdb::unique_xmalloc_ptr<char> filter_,
+				  gdb::unique_xmalloc_ptr<char> cond_string_,
+				  gdb::unique_xmalloc_ptr<char> extra_string_,
+				  enum bpdisp disposition_,
+				  int thread_, int task_, int ignore_count_,
+				  int from_tty,
+				  int enabled_, unsigned flags,
+				  int display_canonical_)
+  : breakpoint (gdbarch_, type_)
 {
   int i;
 
@@ -8326,81 +8331,68 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
 
   gdb_assert (!sals.empty ());
 
-  for (const auto &sal : sals)
-    {
-      struct bp_location *loc;
+  thread = thread_;
+  task = task_;
 
-      if (from_tty)
-	{
-	  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
-	  if (!loc_gdbarch)
-	    loc_gdbarch = gdbarch;
+  cond_string = std::move (cond_string_);
+  extra_string = std::move (extra_string_);
+  ignore_count = ignore_count_;
+  enable_state = enabled_ ? bp_enabled : bp_disabled;
+  disposition = disposition_;
 
-	  describe_other_breakpoints (loc_gdbarch,
-				      sal.pspace, sal.pc, sal.section, thread);
-	}
+  if (type == bp_static_tracepoint
+      || type == bp_static_marker_tracepoint)
+    {
+      auto *t = static_cast <struct tracepoint *> (this);
+      struct static_tracepoint_marker marker;
 
-      if (&sal == &sals[0])
+      if (strace_marker_p (this))
 	{
-	  init_raw_breakpoint (b, sal, type);
-	  b->thread = thread;
-	  b->task = task;
+	  /* We already know the marker exists, otherwise, we
+	     wouldn't see a sal for it.  */
+	  const char *p
+	    = &event_location_to_string (location_.get ())[3];
+	  const char *endp;
 
-	  b->cond_string = std::move (cond_string);
-	  b->extra_string = std::move (extra_string);
-	  b->ignore_count = ignore_count;
-	  b->enable_state = enabled ? bp_enabled : bp_disabled;
-	  b->disposition = disposition;
+	  p = skip_spaces (p);
 
-	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
-	    b->loc->inserted = 1;
+	  endp = skip_to_space (p);
 
-	  if (type == bp_static_tracepoint
-	      || type == bp_static_marker_tracepoint)
-	    {
-	      auto *t = static_cast <struct tracepoint *> (b);
-	      struct static_tracepoint_marker marker;
-
-	      if (strace_marker_p (b))
-		{
-		  /* We already know the marker exists, otherwise, we
-		     wouldn't see a sal for it.  */
-		  const char *p
-		    = &event_location_to_string (b->location.get ())[3];
-		  const char *endp;
-
-		  p = skip_spaces (p);
-
-		  endp = skip_to_space (p);
-
-		  t->static_trace_marker_id.assign (p, endp - p);
-
-		  gdb_printf (_("Probed static tracepoint "
-				"marker \"%s\"\n"),
-			      t->static_trace_marker_id.c_str ());
-		}
-	      else if (target_static_tracepoint_marker_at (sal.pc, &marker))
-		{
-		  t->static_trace_marker_id = std::move (marker.str_id);
+	  t->static_trace_marker_id.assign (p, endp - p);
 
-		  gdb_printf (_("Probed static tracepoint "
-				"marker \"%s\"\n"),
-			      t->static_trace_marker_id.c_str ());
-		}
-	      else
-		warning (_("Couldn't determine the static "
-			   "tracepoint marker to probe"));
-	    }
+	  gdb_printf (_("Probed static tracepoint "
+			"marker \"%s\"\n"),
+		      t->static_trace_marker_id.c_str ());
+	}
+      else if (target_static_tracepoint_marker_at (sals[0].pc, &marker))
+	{
+	  t->static_trace_marker_id = std::move (marker.str_id);
 
-	  loc = b->loc;
+	  gdb_printf (_("Probed static tracepoint "
+			"marker \"%s\"\n"),
+		      t->static_trace_marker_id.c_str ());
 	}
       else
+	warning (_("Couldn't determine the static "
+		   "tracepoint marker to probe"));
+    }
+
+  for (const auto &sal : sals)
+    {
+      if (from_tty)
 	{
-	  loc = b->add_location (sal);
-	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
-	    loc->inserted = 1;
+	  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
+	  if (!loc_gdbarch)
+	    loc_gdbarch = gdbarch;
+
+	  describe_other_breakpoints (loc_gdbarch,
+				      sal.pspace, sal.pc, sal.section, thread);
 	}
 
+      bp_location *new_loc = add_location (sal);
+      if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
+	new_loc->inserted = 1;
+
       /* Do not set breakpoint locations conditions yet.  As locations
 	 are inserted, they get sorted based on their addresses.  Let
 	 the list stabilize to have reliable location numbers.  */
@@ -8409,34 +8401,33 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
 	 command line, otherwise it's an error.  */
       if (type == bp_dprintf)
 	{
-	  if (b->extra_string)
-	    update_dprintf_command_list (b);
+	  if (extra_string)
+	    update_dprintf_command_list (this);
 	  else
 	    error (_("Format string required"));
 	}
-      else if (b->extra_string)
-	error (_("Garbage '%s' at end of command"), b->extra_string.get ());
+      else if (extra_string)
+	error (_("Garbage '%s' at end of command"), extra_string.get ());
     }
 
-
   /* The order of the locations is now stable.  Set the location
      condition using the location's number.  */
   int loc_num = 1;
-  for (bp_location *loc : b->locations ())
+  for (bp_location *bl : locations ())
     {
-      if (b->cond_string != nullptr)
-	set_breakpoint_location_condition (b->cond_string.get (), loc,
-					   b->number, loc_num);
+      if (cond_string != nullptr)
+	set_breakpoint_location_condition (cond_string.get (), bl,
+					   number, loc_num);
 
       ++loc_num;
     }
 
-  b->display_canonical = display_canonical;
-  if (location != NULL)
-    b->location = std::move (location);
+  display_canonical = display_canonical_;
+  if (location_ != NULL)
+    location = std::move (location_);
   else
-    b->location = new_address_location (b->loc->address, NULL, 0);
-  b->filter = std::move (filter);
+    location = new_address_location (this->loc->address, NULL, 0);
+  filter = std::move (filter_);
 }
 
 static void
@@ -8452,18 +8443,19 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       int enabled, int internal, unsigned flags,
 		       int display_canonical)
 {
-  std::unique_ptr<base_breakpoint> b = new_breakpoint_from_type (gdbarch, type);
-
-  init_breakpoint_sal (b.get (), gdbarch,
-		       sals, std::move (location),
-		       std::move (filter),
-		       std::move (cond_string),
-		       std::move (extra_string),
-		       type, disposition,
-		       thread, task, ignore_count,
-		       from_tty,
-		       enabled, flags,
-		       display_canonical);
+  std::unique_ptr<base_breakpoint> b
+    = new_breakpoint_from_type (gdbarch,
+				type,
+				sals,
+				std::move (location),
+				std::move (filter),
+				std::move (cond_string),
+				std::move (extra_string),
+				disposition,
+				thread, task, ignore_count,
+				from_tty,
+				enabled, flags,
+				display_canonical);
 
   install_breakpoint (internal, std::move (b), 0);
 }
@@ -12144,16 +12136,19 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
       event_location_up location
 	= copy_event_location (canonical->location.get ());
 
-      std::unique_ptr<tracepoint> tp (new tracepoint (gdbarch,
-						      type_wanted));
-      init_breakpoint_sal (tp.get (), gdbarch, lsal.sals[i],
-			   std::move (location), NULL,
-			   std::move (cond_string),
-			   std::move (extra_string),
-			   type_wanted, disposition,
-			   thread, task, ignore_count,
-			   from_tty, enabled, flags,
-			   canonical->special_display);
+      std::unique_ptr<tracepoint> tp
+	(new tracepoint (gdbarch,
+			 type_wanted,
+			 lsal.sals[i],
+			 std::move (location),
+			 NULL,
+			 std::move (cond_string),
+			 std::move (extra_string),
+			 disposition,
+			 thread, task, ignore_count,
+			 from_tty, enabled, flags,
+			 canonical->special_display));
+
       /* Given that its possible to have multiple markers with
 	 the same string id, if the user is creating a static
 	 tracepoint by marker id ("strace -m MARKER_ID"), then
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fb2bbd729c0..40ba98b7496 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -861,6 +861,22 @@ struct base_breakpoint : public breakpoint
 {
   using breakpoint::breakpoint;
 
+  /* Create a breakpoint with SALS as locations.  Use LOCATION as a
+     description of the location, and COND_STRING as condition
+     expression.  If LOCATION is NULL then create an "address
+     location" from the address in the SAL.  */
+  base_breakpoint (struct gdbarch *gdbarch, bptype type,
+		   gdb::array_view<const symtab_and_line> sals,
+		   event_location_up &&location,
+		   gdb::unique_xmalloc_ptr<char> filter,
+		   gdb::unique_xmalloc_ptr<char> cond_string,
+		   gdb::unique_xmalloc_ptr<char> extra_string,
+		   enum bpdisp disposition,
+		   int thread, int task, int ignore_count,
+		   int from_tty,
+		   int enabled, unsigned flags,
+		   int display_canonical);
+
   ~base_breakpoint () override = 0;
 
   void re_set () override;
-- 
2.36.0

[PATCH 10/23] Make ada_catchpoint_location's owner ctor parameter be ada_catchpoint

[Pedro Alves]

This commit changes ada_catchpoint_location's ctor from:

  ada_catchpoint_location (breakpoint *owner)

to:

  ada_catchpoint_location (ada_catchpoint *owner)

just to make the code better document intention.

To do this, we need to move the ada_catchpoint_location type's
definition to after ada_catchpoint is defined, otherwise the compiler
doesn't know that ada_catchpoint is convertible to struct breakpoint.

Change-Id: Id908b2e38bde30b262381e00c5637adb9bf0129d
---
 gdb/ada-lang.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 248b847cfbc..1c70f4178d0 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12102,22 +12102,6 @@ static std::string ada_exception_catchpoint_cond_string
    exception, in order to be able to re-set the condition expression
    when symbols change.  */
 
-/* An instance of this type is used to represent an Ada catchpoint
-   breakpoint location.  */
-
-class ada_catchpoint_location : public bp_location
-{
-public:
-  ada_catchpoint_location (breakpoint *owner)
-    : bp_location (owner, bp_loc_software_breakpoint)
-  {}
-
-  /* The condition that checks whether the exception that was raised
-     is the specific exception the user specified on catchpoint
-     creation.  */
-  expression_up excep_cond_expr;
-};
-
 /* An instance of this type is used to represent an Ada catchpoint.  */
 
 struct ada_catchpoint : public base_breakpoint
@@ -12144,6 +12128,22 @@ struct ada_catchpoint : public base_breakpoint
   enum ada_exception_catchpoint_kind m_kind;
 };
 
+/* An instance of this type is used to represent an Ada catchpoint
+   breakpoint location.  */
+
+class ada_catchpoint_location : public bp_location
+{
+public:
+  explicit ada_catchpoint_location (ada_catchpoint *owner)
+    : bp_location (owner, bp_loc_software_breakpoint)
+  {}
+
+  /* The condition that checks whether the exception that was raised
+     is the specific exception the user specified on catchpoint
+     creation.  */
+  expression_up excep_cond_expr;
+};
+
 /* Parse the exception condition string in the context of each of the
    catchpoint's locations, and store them for later evaluation.  */
 
-- 
2.36.0

[PATCH 11/23] Convert init_ada_exception_catchpoint to a ctor

[Pedro Alves]

Currently, init_ada_exception_catchpoint is defined in breakpoint.c, I
presume so it can call the static describe_other_breakpoints function.
I think this is a dependency inversion.
init_ada_exception_catchpoint, being code specific to Ada catchpoints,
should be in ada-lang.c, and describe_other_breakpoints, a core
function, should be exported.

And then, we can convert init_ada_exception_catchpoint to an
ada_catchpoint ctor.

Change-Id: I07695572dabc5a75d3d3740fd9b95db1529406a1
---
 gdb/ada-lang.c   | 43 +++++++++++++++++++++++++++++++++++++++----
 gdb/breakpoint.c | 46 ++--------------------------------------------
 gdb/breakpoint.h | 19 ++++++++-----------
 3 files changed, 49 insertions(+), 59 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 1c70f4178d0..5ddca104dc3 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12107,10 +12107,45 @@ static std::string ada_exception_catchpoint_cond_string
 struct ada_catchpoint : public base_breakpoint
 {
   ada_catchpoint (struct gdbarch *gdbarch_,
-		  enum ada_exception_catchpoint_kind kind)
+		  enum ada_exception_catchpoint_kind kind,
+		  struct symtab_and_line sal,
+		  const char *addr_string_,
+		  bool tempflag,
+		  bool enabled,
+		  bool from_tty)
     : base_breakpoint (gdbarch_, bp_catchpoint),
       m_kind (kind)
   {
+    add_location (sal);
+
+    /* Unlike most base_breakpoint types, Ada catchpoints are
+       pspace-specific.  */
+    gdb_assert (sal.pspace != nullptr);
+    this->pspace = sal.pspace;
+
+    if (from_tty)
+      {
+	struct gdbarch *loc_gdbarch = get_sal_arch (sal);
+	if (!loc_gdbarch)
+	  loc_gdbarch = gdbarch;
+
+	describe_other_breakpoints (loc_gdbarch,
+				    sal.pspace, sal.pc, sal.section, -1);
+	/* FIXME: brobecker/2006-12-28: Actually, re-implement a special
+	   version for exception catchpoints, because two catchpoints
+	   used for different exception names will use the same address.
+	   In this case, a "breakpoint ... also set at..." warning is
+	   unproductive.  Besides, the warning phrasing is also a bit
+	   inappropriate, we should use the word catchpoint, and tell
+	   the user what type of catchpoint it is.  The above is good
+	   enough for now, though.  */
+      }
+
+    enable_state = enabled ? bp_enabled : bp_disabled;
+    disposition = tempflag ? disp_del : disp_donttouch;
+    location = string_to_event_location (&addr_string_,
+					 language_def (language_ada));
+    language = language_ada;
   }
 
   struct bp_location *allocate_location () override;
@@ -12759,9 +12794,9 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch,
   std::string addr_string;
   struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string);
 
-  std::unique_ptr<ada_catchpoint> c (new ada_catchpoint (gdbarch, ex_kind));
-  init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string.c_str (),
-				 tempflag, disabled, from_tty);
+  std::unique_ptr<ada_catchpoint> c
+    (new ada_catchpoint (gdbarch, ex_kind, sal, addr_string.c_str (),
+			 tempflag, disabled, from_tty));
   c->excep_string = excep_string;
   create_excep_cond_exprs (c.get (), ex_kind);
   if (!cond_string.empty ())
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ce742b97fd5..3773f317bfe 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -130,10 +130,6 @@ static CORE_ADDR adjust_breakpoint_address (struct gdbarch *gdbarch,
 					    CORE_ADDR bpaddr,
 					    enum bptype bptype);
 
-static void describe_other_breakpoints (struct gdbarch *,
-					struct program_space *, CORE_ADDR,
-					struct obj_section *, int);
-
 static int watchpoint_locations_match (struct bp_location *loc1,
 				       struct bp_location *loc2);
 
@@ -6862,11 +6858,9 @@ breakpoint_has_pc (struct breakpoint *b,
   return 0;
 }
 
-/* Print a message describing any user-breakpoints set at PC.  This
-   concerns with logical breakpoints, so we match program spaces, not
-   address spaces.  */
+/* See breakpoint.h.  */
 
-static void
+void
 describe_other_breakpoints (struct gdbarch *gdbarch,
 			    struct program_space *pspace, CORE_ADDR pc,
 			    struct obj_section *section, int thread)
@@ -10594,42 +10588,6 @@ until_break_command (const char *arg, int from_tty, int anywhere)
   proceed (-1, GDB_SIGNAL_DEFAULT);
 }
 
-void
-init_ada_exception_breakpoint (struct breakpoint *b,
-			       struct gdbarch *gdbarch,
-			       struct symtab_and_line sal,
-			       const char *addr_string,
-			       int tempflag,
-			       int enabled,
-			       int from_tty)
-{
-  if (from_tty)
-    {
-      struct gdbarch *loc_gdbarch = get_sal_arch (sal);
-      if (!loc_gdbarch)
-	loc_gdbarch = gdbarch;
-
-      describe_other_breakpoints (loc_gdbarch,
-				  sal.pspace, sal.pc, sal.section, -1);
-      /* FIXME: brobecker/2006-12-28: Actually, re-implement a special
-	 version for exception catchpoints, because two catchpoints
-	 used for different exception names will use the same address.
-	 In this case, a "breakpoint ... also set at..." warning is
-	 unproductive.  Besides, the warning phrasing is also a bit
-	 inappropriate, we should use the word catchpoint, and tell
-	 the user what type of catchpoint it is.  The above is good
-	 enough for now, though.  */
-    }
-
-  init_raw_breakpoint (b, sal, bp_catchpoint);
-
-  b->enable_state = enabled ? bp_enabled : bp_disabled;
-  b->disposition = tempflag ? disp_del : disp_donttouch;
-  b->location = string_to_event_location (&addr_string,
-					  language_def (language_ada));
-  b->language = language_ada;
-}
-
 \f
 
 /* Compare two breakpoints and return a strcmp-like result.  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 40ba98b7496..a4ead8b4d4e 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1460,17 +1460,6 @@ extern void
 		     void *user_data_catch,
 		     void *user_data_tcatch);
 
-/* Initialize a breakpoint struct for Ada exception catchpoints.  */
-
-extern void
-  init_ada_exception_breakpoint (struct breakpoint *b,
-				 struct gdbarch *gdbarch,
-				 struct symtab_and_line sal,
-				 const char *addr_string,
-				 int tempflag,
-				 int enabled,
-				 int from_tty);
-
 /* Add breakpoint B on the breakpoint list, and notify the user, the
    target and breakpoint_created observers of its existence.  If
    INTERNAL is non-zero, the breakpoint number will be allocated from
@@ -1905,4 +1894,12 @@ extern void catch_exception_event (enum exception_event_kind ex_event,
 
 extern void print_solib_event (bool is_catchpoint);
 
+/* Print a message describing any user-breakpoints set at PC.  This
+   concerns with logical breakpoints, so we match program spaces, not
+   address spaces.  */
+
+extern void describe_other_breakpoints (struct gdbarch *,
+					struct program_space *, CORE_ADDR,
+					struct obj_section *, int);
+
 #endif /* !defined (BREAKPOINT_H) */
-- 
2.36.0

[PATCH 12/23] Refactor set_internal_breakpoint / internal_breakpoint ctor

[Pedro Alves]

This moves initialization of internal_breakpoint's breakpoint fields
to internal_breakpoint's ctor, and stops using
new_breakpoint_from_type for internal_breakpoint breakpoints.

Change-Id: I898ed0565f47cb00e4429f1c6446e6f9a385a78d
---
 gdb/breakpoint.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3773f317bfe..1659d2b9737 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -119,6 +119,8 @@ static struct breakpoint *set_raw_breakpoint (struct gdbarch *gdbarch,
 					      struct symtab_and_line,
 					      enum bptype);
 
+static breakpoint *add_to_breakpoint_chain (std::unique_ptr<breakpoint> &&b);
+
 static struct breakpoint *
   momentary_breakpoint_from_master (struct breakpoint *orig,
 				    enum bptype type,
@@ -276,7 +278,19 @@ struct ordinary_breakpoint : public base_breakpoint
 /* Internal breakpoints.  */
 struct internal_breakpoint : public base_breakpoint
 {
-  using base_breakpoint::base_breakpoint;
+  internal_breakpoint (struct gdbarch *gdbarch,
+		       enum bptype type, CORE_ADDR address)
+    : base_breakpoint (gdbarch, type)
+  {
+    symtab_and_line sal;
+    sal.pc = address;
+    sal.section = find_pc_overlay (sal.pc);
+    sal.pspace = current_program_space;
+    add_location (sal);
+
+    pspace = current_program_space;
+    disposition = disp_donttouch;
+  }
 
   void re_set () override;
   void check_status (struct bpstat *bs) override;
@@ -1285,17 +1299,6 @@ new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type,
 				  std::forward<Arg> (args)...);
       break;
 
-    case bp_overlay_event:
-    case bp_longjmp_master:
-    case bp_std_terminate_master:
-    case bp_exception_master:
-    case bp_thread_event:
-    case bp_jit_event:
-    case bp_shlib_event:
-      b = new internal_breakpoint (gdbarch, type,
-				   std::forward<Arg> (args)...);
-      break;
-
     case bp_longjmp:
     case bp_exception:
       b = new longjmp_breakpoint (gdbarch, type,
@@ -3294,16 +3297,12 @@ static struct breakpoint *
 create_internal_breakpoint (struct gdbarch *gdbarch,
 			    CORE_ADDR address, enum bptype type)
 {
-  symtab_and_line sal;
-  sal.pc = address;
-  sal.section = find_pc_overlay (sal.pc);
-  sal.pspace = current_program_space;
+  std::unique_ptr<internal_breakpoint> b
+    (new internal_breakpoint (gdbarch, type, address));
 
-  breakpoint *b = set_raw_breakpoint (gdbarch, sal, type);
   b->number = internal_breakpoint_number--;
-  b->disposition = disp_donttouch;
 
-  return b;
+  return add_to_breakpoint_chain (std::move (b));
 }
 
 static const char *const longjmp_names[] =
-- 
2.36.0

[PATCH 13/23] Refactor momentary breakpoints, eliminate set_raw_breakpoint{, _without_location}

[Pedro Alves]

This commit makes set_momentary_breakpoint allocate the breakpoint
type without relying on set_raw_breakpoint, and similarly,
momentary_breakpoint_from_master not rely on
set_raw_breakpoint_without_location.  This will let us convert
init_raw_breakpoint to a ctor in a following patch.

The comment about set_raw_breakpoint being used in gdbtk sources is
stale.  gdbtk no longer uses it.

Change-Id: Ibbf77731e4b22e18ccebc1b5799bbec0aff28c8a
---
 gdb/breakpoint.c | 97 ++++++++++++------------------------------------
 1 file changed, 24 insertions(+), 73 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1659d2b9737..6e24137ec4b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -111,13 +111,7 @@ static int can_use_hardware_watchpoint
 
 static void mention (const breakpoint *);
 
-static struct breakpoint *set_raw_breakpoint_without_location (struct gdbarch *,
-							       enum bptype);
-/* This function is used in gdbtk sources and thus can not be made
-   static.  */
-static struct breakpoint *set_raw_breakpoint (struct gdbarch *gdbarch,
-					      struct symtab_and_line,
-					      enum bptype);
+static breakpoint *add_to_breakpoint_chain (std::unique_ptr<breakpoint> &&b);
 
 static breakpoint *add_to_breakpoint_chain (std::unique_ptr<breakpoint> &&b);
 
@@ -1299,27 +1293,6 @@ new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type,
 				  std::forward<Arg> (args)...);
       break;
 
-    case bp_longjmp:
-    case bp_exception:
-      b = new longjmp_breakpoint (gdbarch, type,
-				  std::forward<Arg> (args)...);
-      break;
-
-    case bp_watchpoint_scope:
-    case bp_finish:
-    case bp_gnu_ifunc_resolver_return:
-    case bp_step_resume:
-    case bp_hp_step_resume:
-    case bp_longjmp_resume:
-    case bp_longjmp_call_dummy:
-    case bp_exception_resume:
-    case bp_call_dummy:
-    case bp_until:
-    case bp_std_terminate:
-      b = new momentary_breakpoint (gdbarch, type,
-				    std::forward<Arg> (args)...);
-      break;
-
     default:
       gdb_assert_not_reached ("invalid type");
     }
@@ -7256,18 +7229,6 @@ add_to_breakpoint_chain (std::unique_ptr<breakpoint> &&b)
   return result;
 }
 
-/* Helper to set_raw_breakpoint below.  Creates a breakpoint
-   that has type BPTYPE and has no locations as yet.  */
-
-static struct breakpoint *
-set_raw_breakpoint_without_location (struct gdbarch *gdbarch,
-				     enum bptype bptype)
-{
-  std::unique_ptr<breakpoint> b = new_breakpoint_from_type (gdbarch, bptype);
-
-  return add_to_breakpoint_chain (std::move (b));
-}
-
 /* Initialize loc->function_name.  */
 
 static void
@@ -7344,31 +7305,6 @@ init_raw_breakpoint (struct breakpoint *b, struct symtab_and_line sal,
     b->pspace = sal.pspace;
 }
 
-/* set_raw_breakpoint is a low level routine for allocating and
-   partially initializing a breakpoint of type BPTYPE.  The newly
-   created breakpoint's address, section, source file name, and line
-   number are provided by SAL.  The newly created and partially
-   initialized breakpoint is added to the breakpoint chain and
-   is also returned as the value of this function.
-
-   It is expected that the caller will complete the initialization of
-   the newly created breakpoint struct as well as output any status
-   information regarding the creation of a new breakpoint.  In
-   particular, set_raw_breakpoint does NOT set the breakpoint
-   number!  Care should be taken to not allow an error to occur
-   prior to completing the initialization of the breakpoint.  If this
-   should happen, a bogus breakpoint will be left on the chain.  */
-
-static struct breakpoint *
-set_raw_breakpoint (struct gdbarch *gdbarch,
-		    struct symtab_and_line sal, enum bptype bptype)
-{
-  std::unique_ptr<breakpoint> b = new_breakpoint_from_type (gdbarch, bptype);
-
-  init_raw_breakpoint (b.get (), sal, bptype);
-  return add_to_breakpoint_chain (std::move (b));
-}
-
 /* Call this routine when stepping and nexting to enable a breakpoint
    if we do a longjmp() or 'throw' in TP.  FRAME is the frame which
    initiated the operation.  */
@@ -7977,6 +7913,17 @@ new_single_step_breakpoint (int thread, struct gdbarch *gdbarch)
   return add_to_breakpoint_chain (std::move (b));
 }
 
+/* Allocate a new momentary breakpoint.  */
+
+static momentary_breakpoint *
+new_momentary_breakpoint (struct gdbarch *gdbarch, enum bptype type)
+{
+  if (type == bp_longjmp || type == bp_exception)
+    return new longjmp_breakpoint (gdbarch, type);
+  else
+    return new momentary_breakpoint (gdbarch, type);
+}
+
 /* Set a momentary breakpoint of type TYPE at address specified by
    SAL.  If FRAME_ID is valid, the breakpoint is restricted to that
    frame.  */
@@ -7985,22 +7932,26 @@ breakpoint_up
 set_momentary_breakpoint (struct gdbarch *gdbarch, struct symtab_and_line sal,
 			  struct frame_id frame_id, enum bptype type)
 {
-  struct breakpoint *b;
-
   /* If FRAME_ID is valid, it should be a real frame, not an inlined or
      tail-called one.  */
   gdb_assert (!frame_id_artificial_p (frame_id));
 
-  b = set_raw_breakpoint (gdbarch, sal, type);
+  std::unique_ptr<momentary_breakpoint> b
+    (new_momentary_breakpoint (gdbarch, type));
+
+  b->add_location (sal);
+  b->pspace = sal.pspace;
   b->enable_state = bp_enabled;
   b->disposition = disp_donttouch;
   b->frame_id = frame_id;
 
   b->thread = inferior_thread ()->global_num;
 
+  breakpoint_up bp (add_to_breakpoint_chain (std::move (b)));
+
   update_global_location_list_nothrow (UGLL_MAY_INSERT);
 
-  return breakpoint_up (b);
+  return bp;
 }
 
 /* Make a momentary breakpoint based on the master breakpoint ORIG.
@@ -8012,9 +7963,8 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
 				  enum bptype type,
 				  int loc_enabled)
 {
-  struct breakpoint *copy;
-
-  copy = set_raw_breakpoint_without_location (orig->gdbarch, type);
+  std::unique_ptr<breakpoint> copy
+    (new_momentary_breakpoint (orig->gdbarch, type));
   copy->loc = copy->allocate_location ();
   set_breakpoint_location_function (copy->loc);
 
@@ -8035,8 +7985,9 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
   copy->disposition = disp_donttouch;
   copy->number = internal_breakpoint_number--;
 
+  breakpoint *b = add_to_breakpoint_chain (std::move (copy));
   update_global_location_list_nothrow (UGLL_DONT_INSERT);
-  return copy;
+  return b;
 }
 
 /* Make a deep copy of momentary breakpoint ORIG.  Returns NULL if
-- 
2.36.0

[PATCH 14/23] Make exception_catchpoint inherit base_breakpoint instead of catchpoint

[Pedro Alves]

exception_catchpoint is really a code breakpoint, with locations set
by sals, re-set like other code breakpoints, etc., so make it inherit
base_breakpoint.

This adds a bit of duplicated code to exception_catchpoint's ctor
(copied from struct catchpoint's ctor), but it will be eliminated in a
following patch.

Change-Id: I9fbb2927491120e9744a4f5e5cb5e6870ca07009
---
 gdb/break-catch-throw.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index f15fa531519..90fc3e6d325 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -63,15 +63,17 @@ static const struct exception_names exception_functions[] =
   { "-probe-stap libstdcxx:catch", "__cxa_begin_catch" }
 };
 
-/* The type of an exception catchpoint.  */
+/* The type of an exception catchpoint.  Unlike most catchpoints, this
+   one is implemented with code breakpoints, so it inherits struct
+   base_breakpoint, not struct catchpoint.  */
 
-struct exception_catchpoint : public catchpoint
+struct exception_catchpoint : public base_breakpoint
 {
   exception_catchpoint (struct gdbarch *gdbarch,
-			bool temp, const char *cond_string,
+			bool temp, const char *cond_string_,
 			enum exception_event_kind kind_,
 			std::string &&except_rx)
-    : catchpoint (gdbarch, temp, cond_string),
+    : base_breakpoint (gdbarch, bp_catchpoint),
       kind (kind_),
       exception_rx (std::move (except_rx)),
       pattern (exception_rx.empty ()
@@ -79,6 +81,12 @@ struct exception_catchpoint : public catchpoint
 	       : new compiled_regex (exception_rx.c_str (), REG_NOSUB,
 				     _("invalid type-matching regexp")))
   {
+    if (cond_string_ != nullptr)
+      cond_string = make_unique_xstrdup (cond_string_);
+    disposition = temp ? disp_del : disp_donttouch;
+
+    pspace = current_program_space;
+    re_set ();
   }
 
   void re_set () override;
@@ -375,8 +383,6 @@ handle_gnu_v3_exceptions (int tempflag, std::string &&except_rx,
     (new exception_catchpoint (gdbarch, tempflag, cond_string,
 			       ex_event, std::move (except_rx)));
 
-  cp->re_set ();
-
   install_breakpoint (0, std::move (cp), 1);
 }
 
-- 
2.36.0

[PATCH 15/23] Make breakpoint_address_bits look at the location kind

[Pedro Alves]

Software watchpoints allocate a special dummy location using
software_watchpoint_add_no_memory_location, and then
breakpoint_address_bits checks whether the location is that special
location to decide whether the location has a meaninful address to
print.

Introduce a new bp_loc_software_watchpoint location kind, and make
breakpoint_address_bits use bl_address_is_meaningful instead, which
returns false for bp_loc_other, which is in accordance with we
document for bp_location::address:

  /* (... snip ...)  Valid for all types except
     bp_loc_other.  */
  CORE_ADDR address = 0;

Rename software_watchpoint_add_no_memory_location to
add_dummy_location, and simplify it.  This will be used by catchpoints
too in a following patch.

Note that neither "info breakpoints" nor "maint info breakpoints"
actually prints the addresses of watchpoints, but I think it would be
useful to do so in "maint info breakpoints".  This approach let's us
implement that in the future.

Change-Id: I50e398f66ef618c31ffa662da755eaba6295aed7
---
 gdb/breakpoint.c | 59 ++++++++++++++++++------------------------------
 gdb/breakpoint.h |  1 +
 2 files changed, 23 insertions(+), 37 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6e24137ec4b..6dfb89f51bc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -164,6 +164,8 @@ static std::vector<symtab_and_line> bkpt_probe_decode_location
       struct event_location *location,
       struct program_space *search_pspace);
 
+static bool bl_address_is_meaningful (bp_location *loc);
+
 /* update_global_location_list's modes of operation wrt to whether to
    insert locations now.  */
 enum ugll_insert_mode
@@ -1830,34 +1832,18 @@ extract_bitfield_from_watchpoint_value (struct watchpoint *w, struct value *val)
   return bit_val;
 }
 
-/* Allocate a dummy location and add it to B, which must be a software
-   watchpoint.  This is required because even if a software watchpoint
-   is not watching any memory, bpstat_stop_status requires a location
-   to be able to report stops.  */
+/* Allocate a dummy location and add it to B.  This is required
+   because bpstat_stop_status requires a location to be able to report
+   stops.  */
 
 static void
-software_watchpoint_add_no_memory_location (struct breakpoint *b,
-					    struct program_space *pspace)
+add_dummy_location (struct breakpoint *b,
+		    struct program_space *pspace)
 {
-  gdb_assert (b->type == bp_watchpoint && b->loc == NULL);
+  gdb_assert (b->loc == NULL);
 
-  b->loc = b->allocate_location ();
+  b->loc = new bp_location (b, bp_loc_other);
   b->loc->pspace = pspace;
-  b->loc->address = -1;
-  b->loc->length = -1;
-}
-
-/* Returns true if B is a software watchpoint that is not watching any
-   memory (e.g., "watch $pc").  */
-
-static bool
-is_no_memory_software_watchpoint (struct breakpoint *b)
-{
-  return (b->type == bp_watchpoint
-	  && b->loc != NULL
-	  && b->loc->next == NULL
-	  && b->loc->address == -1
-	  && b->loc->length == -1);
 }
 
 /* Assuming that B is a watchpoint:
@@ -2197,7 +2183,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	  else
 	    b->type = bp_watchpoint;
 
-	  loc_type = (b->type == bp_watchpoint? bp_loc_other
+	  loc_type = (b->type == bp_watchpoint? bp_loc_software_watchpoint
 		      : bp_loc_hardware_watchpoint);
 	  for (bp_location *bl : b->locations ())
 	    bl->loc_type = loc_type;
@@ -2208,7 +2194,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	 bpstat_stop_status requires a location to be able to report
 	 stops, so make sure there's at least a dummy one.  */
       if (b->type == bp_watchpoint && b->loc == NULL)
-	software_watchpoint_add_no_memory_location (b, frame_pspace);
+	add_dummy_location (b, frame_pspace);
     }
   else if (!within_current_scope)
     {
@@ -3840,11 +3826,12 @@ detach_breakpoints (ptid_t ptid)
 
       /* This function must physically remove breakpoints locations
 	 from the specified ptid, without modifying the breakpoint
-	 package's state.  Locations of type bp_loc_other are only
-	 maintained at GDB side.  So, there is no need to remove
-	 these bp_loc_other locations.  Moreover, removing these
+	 package's state.  Locations of type bp_loc_other and
+	 bp_loc_software_watchpoint are only maintained at GDB side,
+	 so there is no need to remove them.  Moreover, removing these
 	 would modify the breakpoint package's state.  */
-      if (bl->loc_type == bp_loc_other)
+      if (bl->loc_type == bp_loc_other
+	  || bl->loc_type == bp_loc_software_watchpoint)
 	continue;
 
       if (bl->inserted)
@@ -5798,7 +5785,8 @@ bpstat_what (bpstat *bs_head)
 	    {
 	      /* Some catchpoints are implemented with breakpoints.
 		 For those, we need to step over the breakpoint.  */
-	      if (bs->bp_location_at->loc_type != bp_loc_other)
+	      if (bs->bp_location_at->loc_type == bp_loc_software_breakpoint
+		  || bs->bp_location_at->loc_type == bp_loc_hardware_breakpoint)
 		this_action = BPSTAT_WHAT_SINGLE;
 	    }
 	  break;
@@ -6557,16 +6545,12 @@ breakpoint_address_bits (struct breakpoint *b)
 {
   int print_address_bits = 0;
 
-  /* Software watchpoints that aren't watching memory don't have an
-     address to print.  */
-  if (is_no_memory_software_watchpoint (b))
-    return 0;
-
   for (bp_location *loc : b->locations ())
     {
-      int addr_bit;
+      if (!bl_address_is_meaningful (loc))
+	continue;
 
-      addr_bit = gdbarch_addr_bit (loc->gdbarch);
+      int addr_bit = gdbarch_addr_bit (loc->gdbarch);
       if (addr_bit > print_address_bits)
 	print_address_bits = addr_bit;
     }
@@ -7161,6 +7145,7 @@ bp_location_from_bp_type (bptype type)
     case bp_access_watchpoint:
       return bp_loc_hardware_watchpoint;
     case bp_watchpoint:
+      return bp_loc_software_watchpoint;
     case bp_catchpoint:
     case bp_tracepoint:
     case bp_fast_tracepoint:
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index a4ead8b4d4e..7375e976dc6 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -314,6 +314,7 @@ enum bp_loc_type
 {
   bp_loc_software_breakpoint,
   bp_loc_hardware_breakpoint,
+  bp_loc_software_watchpoint,
   bp_loc_hardware_watchpoint,
   bp_loc_other			/* Miscellaneous...  */
 };
-- 
2.36.0

[PATCH 16/23] Make catchpoint inherit breakpoint, eliminate init_raw_breakpoint

[Pedro Alves]

struct catchpoint's ctor currently calls init_raw_breakpoint, which is
a bit weird, as that ctor-like function takes a sal argument, but
catchpoints don't have code locations.

Instead, make struct catchpoint's ctor add the catchpoint's dummy
location using add_dummy_location.

init_raw_breakpoint uses add_location under the hood, and with a dummy
sal it would ultimately use the breakpoint's gdbarch for the
location's gdbarch, so replace the references to loc->gdbarch (which
is now NULL) in syscall_catchpoint to references to the catchpoint's
gdbarch.

struct catchpoint's ctor was the last user of init_raw_breakpoint, so
this commit eliminates the latter.

Since catchpoint locations aren't code locations, make struct
catchpoint inherit struct breakpoint instead of base_breakpoint.  This
let's us delete the tracepoint::re_set override too.

Change-Id: Ib428bf71efb09fdaf399c56e4372b0f41d9c5869
---
 gdb/break-catch-syscall.c |  6 +++---
 gdb/breakpoint.c          | 31 +++----------------------------
 gdb/breakpoint.h          |  7 +------
 3 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/gdb/break-catch-syscall.c b/gdb/break-catch-syscall.c
index af55ecb1b5c..06d48466de7 100644
--- a/gdb/break-catch-syscall.c
+++ b/gdb/break-catch-syscall.c
@@ -197,7 +197,7 @@ syscall_catchpoint::print_it (const bpstat *bs) const
      must print "called syscall" or "returned from syscall".  */
   struct target_waitstatus last;
   struct syscall s;
-  struct gdbarch *gdbarch = bs->bp_location_at->gdbarch;
+  struct gdbarch *gdbarch = b->gdbarch;
 
   get_last_target_status (nullptr, nullptr, &last);
 
@@ -242,7 +242,7 @@ syscall_catchpoint::print_one (bp_location **last_loc) const
 {
   struct value_print_options opts;
   struct ui_out *uiout = current_uiout;
-  struct gdbarch *gdbarch = loc->gdbarch;
+  struct gdbarch *gdbarch = loc->owner->gdbarch;
 
   get_user_print_options (&opts);
   /* Field 4, the address, is omitted (which makes the columns not
@@ -293,7 +293,7 @@ syscall_catchpoint::print_one (bp_location **last_loc) const
 void
 syscall_catchpoint::print_mention () const
 {
-  struct gdbarch *gdbarch = loc->gdbarch;
+  struct gdbarch *gdbarch = loc->owner->gdbarch;
 
   if (!syscalls_to_be_caught.empty ())
     {
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6dfb89f51bc..bb433463c55 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7266,30 +7266,6 @@ get_sal_arch (struct symtab_and_line sal)
   return NULL;
 }
 
-/* Low level routine for partially initializing a breakpoint of type
-   BPTYPE.  The newly created breakpoint's address, section, source
-   file name, and line number are provided by SAL.
-
-   It is expected that the caller will complete the initialization of
-   the newly created breakpoint struct as well as output any status
-   information regarding the creation of a new breakpoint.  */
-
-static void
-init_raw_breakpoint (struct breakpoint *b, struct symtab_and_line sal,
-		     enum bptype bptype)
-{
-  b->add_location (sal);
-
-  if (bptype != bp_catchpoint)
-    gdb_assert (sal.pspace != NULL);
-
-  /* Store the program space that was used to set the breakpoint,
-     except for ordinary breakpoints, which are independent of the
-     program space.  */
-  if (bptype != bp_breakpoint && bptype != bp_hardware_breakpoint)
-    b->pspace = sal.pspace;
-}
-
 /* Call this routine when stepping and nexting to enable a breakpoint
    if we do a longjmp() or 'throw' in TP.  FRAME is the frame which
    initiated the operation.  */
@@ -7751,12 +7727,11 @@ disable_breakpoints_in_freed_objfile (struct objfile *objfile)
 
 catchpoint::catchpoint (struct gdbarch *gdbarch, bool temp,
 			const char *cond_string_)
-  : base_breakpoint (gdbarch, bp_catchpoint)
+  : breakpoint (gdbarch, bp_catchpoint)
 {
-  symtab_and_line sal;
-  sal.pspace = current_program_space;
+  add_dummy_location (this, current_program_space);
 
-  init_raw_breakpoint (this, sal, bp_catchpoint);
+  pspace = current_program_space;
 
   if (cond_string_ != nullptr)
     cond_string = make_unique_xstrdup (cond_string_);
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 7375e976dc6..21dd8d53b2d 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1030,18 +1030,13 @@ struct tracepoint : public base_breakpoint
 
 /* The base class for catchpoints.  */
 
-struct catchpoint : public base_breakpoint
+struct catchpoint : public breakpoint
 {
   /* If TEMP is true, then make the breakpoint temporary.  If
      COND_STRING is not NULL, then store it in the breakpoint.  */
   catchpoint (struct gdbarch *gdbarch, bool temp, const char *cond_string);
 
   ~catchpoint () override = 0;
-
-  void re_set () override
-  {
-    /* For catchpoints, the default is to do nothing.  */
-  }
 };
 
 \f
-- 
2.36.0

[PATCH 17/23] Move common bits of catchpoint/exception_catchpoint to breakpoint's ctor

[Pedro Alves]

Move common bits of catchpoint and exception_catchpoint to
breakpoint's ctor, to avoid duplicating code.

Change-Id: I3a115180f4d496426522f1d89a3875026aea3cf2
---
 gdb/break-catch-throw.c |  6 +-----
 gdb/breakpoint.c        | 24 ++++++++++++++++++------
 gdb/breakpoint.h        | 10 ++--------
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 90fc3e6d325..a6f477b712a 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -73,7 +73,7 @@ struct exception_catchpoint : public base_breakpoint
 			bool temp, const char *cond_string_,
 			enum exception_event_kind kind_,
 			std::string &&except_rx)
-    : base_breakpoint (gdbarch, bp_catchpoint),
+    : base_breakpoint (gdbarch, bp_catchpoint, temp, cond_string_),
       kind (kind_),
       exception_rx (std::move (except_rx)),
       pattern (exception_rx.empty ()
@@ -81,10 +81,6 @@ struct exception_catchpoint : public base_breakpoint
 	       : new compiled_regex (exception_rx.c_str (), REG_NOSUB,
 				     _("invalid type-matching regexp")))
   {
-    if (cond_string_ != nullptr)
-      cond_string = make_unique_xstrdup (cond_string_);
-    disposition = temp ? disp_del : disp_donttouch;
-
     pspace = current_program_space;
     re_set ();
   }
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index bb433463c55..4dce43b1e6b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7725,17 +7725,29 @@ disable_breakpoints_in_freed_objfile (struct objfile *objfile)
 
 /* See breakpoint.h.  */
 
+breakpoint::breakpoint (struct gdbarch *gdbarch_, enum bptype bptype,
+			bool temp, const char *cond_string_)
+  : type (bptype),
+    disposition (temp ? disp_del : disp_donttouch),
+    gdbarch (gdbarch_),
+    language (current_language->la_language),
+    input_radix (::input_radix),
+    cond_string (cond_string_ != nullptr
+		 ? make_unique_xstrdup (cond_string_)
+		 : nullptr),
+    related_breakpoint (this)
+{
+}
+
+/* See breakpoint.h.  */
+
 catchpoint::catchpoint (struct gdbarch *gdbarch, bool temp,
-			const char *cond_string_)
-  : breakpoint (gdbarch, bp_catchpoint)
+			const char *cond_string)
+  : breakpoint (gdbarch, bp_catchpoint, temp, cond_string)
 {
   add_dummy_location (this, current_program_space);
 
   pspace = current_program_space;
-
-  if (cond_string_ != nullptr)
-    cond_string = make_unique_xstrdup (cond_string_);
-  disposition = temp ? disp_del : disp_donttouch;
 }
 
 void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 21dd8d53b2d..af69af6863b 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -620,14 +620,8 @@ using bp_location_range = next_range<bp_location>;
 
 struct breakpoint
 {
-  breakpoint (struct gdbarch *gdbarch_, enum bptype bptype)
-    : type (bptype),
-      gdbarch (gdbarch_),
-      language (current_language->la_language),
-      input_radix (::input_radix),
-      related_breakpoint (this)
-  {
-  }
+  breakpoint (struct gdbarch *gdbarch_, enum bptype bptype,
+	      bool temp = true, const char *cond_string = nullptr);
 
   DISABLE_COPY_AND_ASSIGN (breakpoint);
 
-- 
2.36.0

[PATCH 18/23] Move add_location(sal) to base_breakpoint

[Pedro Alves]

After the previous patches, only base_breakpoint subclasses use
add_location(sal), so we can move it to base_breakpoint (a.k.a. base
class for code breakpoints).

This requires a few casts here and there, but always at spots where
you can see from context what the breakpoint's type actually is.

I inlined new_single_step_breakpoint into its only caller exactly for
this reason.

I did try to propagate more use of base_breakpoint to avoid casts, but
that turned out unwieldy for this patch.

Change-Id: I49d959322b0fdce5a88a216bb44730fc5dd7c6f8
---
 gdb/breakpoint.c | 45 +++++++++++++++++++--------------------------
 gdb/breakpoint.h |  8 ++++----
 gdb/elfread.c    |  6 +++---
 gdb/minsyms.c    |  4 ++--
 gdb/symtab.h     |  5 +++--
 5 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4dce43b1e6b..613f6a875e4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -87,7 +87,7 @@
 static void map_breakpoint_numbers (const char *,
 				    gdb::function_view<void (breakpoint *)>);
 
-static void breakpoint_re_set_default (struct breakpoint *);
+static void breakpoint_re_set_default (base_breakpoint *);
 
 static void
   create_sals_from_location_default (struct event_location *location,
@@ -5860,10 +5860,10 @@ bpstat_run_callbacks (bpstat *bs_head)
 	  handle_jit_event (bs->bp_location_at->address);
 	  break;
 	case bp_gnu_ifunc_resolver:
-	  gnu_ifunc_resolver_stop (b);
+	  gnu_ifunc_resolver_stop ((base_breakpoint *) b);
 	  break;
 	case bp_gnu_ifunc_resolver_return:
-	  gnu_ifunc_resolver_return_stop (b);
+	  gnu_ifunc_resolver_return_stop ((base_breakpoint *) b);
 	  break;
 	}
     }
@@ -7867,24 +7867,6 @@ enable_breakpoints_after_startup (void)
   breakpoint_re_set ();
 }
 
-/* Create a new single-step breakpoint for thread THREAD, with no
-   locations.  */
-
-static struct breakpoint *
-new_single_step_breakpoint (int thread, struct gdbarch *gdbarch)
-{
-  std::unique_ptr<breakpoint> b (new momentary_breakpoint (gdbarch,
-							   bp_single_step));
-
-  b->disposition = disp_donttouch;
-  b->frame_id = null_frame_id;
-
-  b->thread = thread;
-  gdb_assert (b->thread != 0);
-
-  return add_to_breakpoint_chain (std::move (b));
-}
-
 /* Allocate a new momentary breakpoint.  */
 
 static momentary_breakpoint *
@@ -8057,7 +8039,7 @@ handle_automatic_hardware_breakpoints (bp_location *bl)
 }
 
 bp_location *
-breakpoint::add_location (const symtab_and_line &sal)
+base_breakpoint::add_location (const symtab_and_line &sal)
 {
   struct bp_location *new_loc, **tmp;
   CORE_ADDR adjusted_address;
@@ -12480,7 +12462,7 @@ hoist_existing_locations (struct breakpoint *b, struct program_space *pspace)
    untouched.  */
 
 void
-update_breakpoint_locations (struct breakpoint *b,
+update_breakpoint_locations (base_breakpoint *b,
 			     struct program_space *filter_pspace,
 			     gdb::array_view<const symtab_and_line> sals,
 			     gdb::array_view<const symtab_and_line> sals_end)
@@ -12688,7 +12670,7 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
    locations.  */
 
 static void
-breakpoint_re_set_default (struct breakpoint *b)
+breakpoint_re_set_default (base_breakpoint *b)
 {
   struct program_space *filter_pspace = current_program_space;
   std::vector<symtab_and_line> expanded, expanded_end;
@@ -13399,15 +13381,26 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
 
   if (tp->control.single_step_breakpoints == NULL)
     {
+      std::unique_ptr<breakpoint> b
+	(new momentary_breakpoint (gdbarch, bp_single_step));
+
+      b->disposition = disp_donttouch;
+
+      b->thread = tp->global_num;
+      gdb_assert (b->thread != 0);
+
       tp->control.single_step_breakpoints
-	= new_single_step_breakpoint (tp->global_num, gdbarch);
+	= add_to_breakpoint_chain (std::move (b));
     }
 
   sal = find_pc_line (pc, 0);
   sal.pc = pc;
   sal.section = find_pc_overlay (pc);
   sal.explicit_pc = 1;
-  tp->control.single_step_breakpoints->add_location (sal);
+
+  auto *ss_bp
+    = static_cast<momentary_breakpoint *> (tp->control.single_step_breakpoints);
+  ss_bp->add_location (sal);
 
   update_global_location_list (UGLL_INSERT);
 }
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index af69af6863b..807c97a5bed 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -733,9 +733,6 @@ struct breakpoint
     /* Nothing to do.  */
   }
 
-  /* Add a location for SAL to this breakpoint.  */
-  bp_location *add_location (const symtab_and_line &sal);
-
   /* Return a range of this breakpoint's locations.  */
   bp_location_range locations () const;
 
@@ -874,6 +871,9 @@ struct base_breakpoint : public breakpoint
 
   ~base_breakpoint () override = 0;
 
+  /* Add a location for SAL to this breakpoint.  */
+  bp_location *add_location (const symtab_and_line &sal);
+
   void re_set () override;
   int insert_location (struct bp_location *) override;
   int remove_location (struct bp_location *,
@@ -1383,7 +1383,7 @@ extern void until_break_command (const char *, int, int);
 /* Initialize a struct bp_location.  */
 
 extern void update_breakpoint_locations
-  (struct breakpoint *b,
+  (base_breakpoint *b,
    struct program_space *filter_pspace,
    gdb::array_view<const symtab_and_line> sals,
    gdb::array_view<const symtab_and_line> sals_end);
diff --git a/gdb/elfread.c b/gdb/elfread.c
index b136c605b1a..27203722802 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -927,7 +927,7 @@ elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
 /* Handle inferior hit of bp_gnu_ifunc_resolver, see its definition.  */
 
 static void
-elf_gnu_ifunc_resolver_stop (struct breakpoint *b)
+elf_gnu_ifunc_resolver_stop (base_breakpoint *b)
 {
   struct breakpoint *b_return;
   struct frame_info *prev_frame = get_prev_frame (get_current_frame ());
@@ -978,7 +978,7 @@ elf_gnu_ifunc_resolver_stop (struct breakpoint *b)
 /* Handle inferior hit of bp_gnu_ifunc_resolver_return, see its definition.  */
 
 static void
-elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
+elf_gnu_ifunc_resolver_return_stop (base_breakpoint *b)
 {
   thread_info *thread = inferior_thread ();
   struct gdbarch *gdbarch = get_frame_arch (get_current_frame ());
@@ -1008,7 +1008,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
 			    "gnu-indirect-function breakpoint type %d"),
 			  (int) b->type);
 	}
-      b = b_next;
+      b = (base_breakpoint *) b_next;
     }
   gdb_assert (b->type == bp_gnu_ifunc_resolver);
   gdb_assert (b->loc->next == NULL);
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index cbd0ad22392..217ee047446 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1019,7 +1019,7 @@ stub_gnu_ifunc_resolve_name (const char *function_name,
 /* See elf_gnu_ifunc_resolver_stop for its real implementation.  */
 
 static void
-stub_gnu_ifunc_resolver_stop (struct breakpoint *b)
+stub_gnu_ifunc_resolver_stop (base_breakpoint *b)
 {
   internal_error (__FILE__, __LINE__,
 		  _("elf_gnu_ifunc_resolver_stop cannot be reached."));
@@ -1028,7 +1028,7 @@ stub_gnu_ifunc_resolver_stop (struct breakpoint *b)
 /* See elf_gnu_ifunc_resolver_return_stop for its real implementation.  */
 
 static void
-stub_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
+stub_gnu_ifunc_resolver_return_stop (base_breakpoint *b)
 {
   internal_error (__FILE__, __LINE__,
 		  _("elf_gnu_ifunc_resolver_return_stop cannot be reached."));
diff --git a/gdb/symtab.h b/gdb/symtab.h
index b1cf84f756f..5218be587de 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -55,6 +55,7 @@ struct obj_section;
 struct cmd_list_element;
 class probe;
 struct lookup_name_info;
+struct base_breakpoint;
 
 /* How to match a lookup name against a symbol search name.  */
 enum class symbol_name_match_type
@@ -2227,10 +2228,10 @@ struct gnu_ifunc_fns
 				 CORE_ADDR *function_address_p);
 
   /* See elf_gnu_ifunc_resolver_stop for its real implementation.  */
-  void (*gnu_ifunc_resolver_stop) (struct breakpoint *b);
+  void (*gnu_ifunc_resolver_stop) (base_breakpoint *b);
 
   /* See elf_gnu_ifunc_resolver_return_stop for its real implementation.  */
-  void (*gnu_ifunc_resolver_return_stop) (struct breakpoint *b);
+  void (*gnu_ifunc_resolver_return_stop) (base_breakpoint *b);
 };
 
 #define gnu_ifunc_resolve_addr gnu_ifunc_fns_p->gnu_ifunc_resolve_addr
-- 
2.36.0

[PATCH 19/23] Add/tweak intro comments of struct breakpoint and several subclasses

[Pedro Alves]

This tweaks the intro comments of the following classes:

 internal_breakpoint
 momentary_breakpoint
 breakpoint
 base_breakpoint
 watchpoint
 catchpoint

Change-Id: If6b31f51ebbb81705fbe5b8435f60ab2c88a98c8
---
 gdb/breakpoint.c | 12 ++++++++++--
 gdb/breakpoint.h | 13 +++++++------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 613f6a875e4..0d239f50fcc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -271,7 +271,10 @@ struct ordinary_breakpoint : public base_breakpoint
   void print_recreate (struct ui_file *fp) const override;
 };
 
-/* Internal breakpoints.  */
+/* Internal breakpoints.  These typically have a lifetime the same as
+   the program, and they end up installed on the breakpoint chain with
+   a negative breakpoint number.  They're visible in "maint info
+   breakpoints", but not "info breakpoints".  */
 struct internal_breakpoint : public base_breakpoint
 {
   internal_breakpoint (struct gdbarch *gdbarch,
@@ -294,7 +297,12 @@ struct internal_breakpoint : public base_breakpoint
   void print_mention () const override;
 };
 
-/* Momentary breakpoints.  */
+/* Momentary breakpoints.  These typically have a lifetime of some run
+   control command only, are always thread-specific, and have 0 for
+   breakpoint number.  I.e., there can be many momentary breakpoints
+   on the breakpoint chain and they all same the same number (zero).
+   They're visible in "maint info breakpoints", but not "info
+   breakpoints".  */
 struct momentary_breakpoint : public base_breakpoint
 {
   using base_breakpoint::base_breakpoint;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 807c97a5bed..8735396e8d4 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -616,7 +616,7 @@ using bp_location_range = next_range<bp_location>;
    useful for a hack I had to put in; I'm going to leave it in because
    I can see how there might be times when it would indeed be useful */
 
-/* This is for all kinds of breakpoints.  */
+/* Abstract base class representing all kinds of breakpoints.  */
 
 struct breakpoint
 {
@@ -846,9 +846,9 @@ struct breakpoint
   void print_recreate_thread (struct ui_file *fp) const;
 };
 
-/* The structure to be inherit by all kinds of breakpoints (real
-   breakpoints, i.e., user "break" breakpoints, internal and momentary
-   breakpoints, etc.).  */
+/* Abstract base class representing code breakpoints.  User "break"
+   breakpoints, internal and momentary breakpoints, etc.  IOW, any
+   kind of breakpoint whose locations are created from SALs.  */
 struct base_breakpoint : public breakpoint
 {
   using breakpoint::breakpoint;
@@ -887,7 +887,8 @@ struct base_breakpoint : public breakpoint
 	struct program_space *search_pspace) override;
 };
 
-/* An instance of this type is used to represent a watchpoint.  */
+/* An instance of this type is used to represent a watchpoint,
+   a.k.a. a data breakpoint.  */
 
 struct watchpoint : public breakpoint
 {
@@ -1022,7 +1023,7 @@ struct tracepoint : public base_breakpoint
   int static_trace_marker_id_idx = 0;
 };
 
-/* The base class for catchpoints.  */
+/* The abstract base class for catchpoints.  */
 
 struct catchpoint : public breakpoint
 {
-- 
2.36.0

[PATCH 20/23] Momentary breakpoints should have no breakpoint number

[Pedro Alves]

Momentary breakpoints have no breakpoint number, their breakpoint
number should be always 0, to avoid constantly incrementing (or
decrementing) the internal breakpoint count.

Indeed, set_momentary_breakpoint installs the created breakpoint
without a number.

However, momentary_breakpoint_from_master incorrectly gives an
internal breakpoint number to the new breakpoint.  This commit fixes
that.

Change-Id: Iedcae5432cdf232db9e9a6e1a646d358abd34f95
---
 gdb/breakpoint.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0d239f50fcc..4f664cbd7aa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7945,7 +7945,6 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
 
   copy->enable_state = bp_enabled;
   copy->disposition = disp_donttouch;
-  copy->number = internal_breakpoint_number--;
 
   breakpoint *b = add_to_breakpoint_chain (std::move (copy));
   update_global_location_list_nothrow (UGLL_DONT_INSERT);
-- 
2.36.0

[PATCH 21/23] Make sure momentary breakpoints are always thread-specific

[Pedro Alves]

This adds a new ctor to momentary_breakpoints with a few parameters
that are always necessary for momentary breakpoints.

In particular, I noticed that set_std_terminate_breakpoint doesn't
make the breakpoint be thread specific, which looks like a bug to me.

The point of that breakpoint is to intercept std::terminate calls that
happen as result of the called thread throwing an exception that won't
be caught by the dummy frame.  If some other thread calls
std::terminate, IMO, it's no different from some other thread calling
exit/_exit, for example.

Change-Id: Ifc5ff4a6d6e58b8c4854d00b86725382d38a1a02
---
 gdb/breakpoint.c | 83 +++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4f664cbd7aa..f4642c2d8e7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -118,7 +118,7 @@ static breakpoint *add_to_breakpoint_chain (std::unique_ptr<breakpoint> &&b);
 static struct breakpoint *
   momentary_breakpoint_from_master (struct breakpoint *orig,
 				    enum bptype type,
-				    int loc_enabled);
+				    int loc_enabled, int thread);
 
 static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);
 
@@ -305,7 +305,25 @@ struct internal_breakpoint : public base_breakpoint
    breakpoints".  */
 struct momentary_breakpoint : public base_breakpoint
 {
-  using base_breakpoint::base_breakpoint;
+  momentary_breakpoint (struct gdbarch *gdbarch_, enum bptype bptype,
+			program_space *pspace_,
+			const struct frame_id &frame_id_,
+			int thread_)
+    : base_breakpoint (gdbarch_, bptype)
+  {
+    /* If FRAME_ID is valid, it should be a real frame, not an inlined
+       or tail-called one.  */
+    gdb_assert (!frame_id_artificial_p (frame_id));
+
+    /* Momentary breakpoints are always thread-specific.  */
+    gdb_assert (thread_ > 0);
+
+    pspace = pspace_;
+    enable_state = bp_enabled;
+    disposition = disp_donttouch;
+    frame_id = frame_id_;
+    thread = thread_;
+  }
 
   void re_set () override;
   void check_status (struct bpstat *bs) override;
@@ -7293,12 +7311,9 @@ set_longjmp_breakpoint (struct thread_info *tp, struct frame_id frame)
 	    || b->type == bp_exception_master))
       {
 	enum bptype type = b->type == bp_longjmp_master ? bp_longjmp : bp_exception;
-	struct breakpoint *clone;
-
 	/* longjmp_breakpoint_ops ensures INITIATING_FRAME is cleared again
 	   after their removal.  */
-	clone = momentary_breakpoint_from_master (b, type, 1);
-	clone->thread = thread;
+	momentary_breakpoint_from_master (b, type, 1, thread);
       }
 
   tp->initiating_frame = frame;
@@ -7340,11 +7355,10 @@ set_longjmp_breakpoint_for_call_dummy (void)
   for (breakpoint *b : all_breakpoints ())
     if (b->pspace == current_program_space && b->type == bp_longjmp_master)
       {
-	struct breakpoint *new_b;
-
-	new_b = momentary_breakpoint_from_master (b, bp_longjmp_call_dummy,
-						  1);
-	new_b->thread = inferior_thread ()->global_num;
+	int thread = inferior_thread ()->global_num;
+	breakpoint *new_b
+	  = momentary_breakpoint_from_master (b, bp_longjmp_call_dummy,
+					      1, thread);
 
 	/* Link NEW_B into the chain of RETVAL breakpoints.  */
 
@@ -7473,7 +7487,8 @@ set_std_terminate_breakpoint (void)
     if (b->pspace == current_program_space
 	&& b->type == bp_std_terminate_master)
       {
-	momentary_breakpoint_from_master (b, bp_std_terminate, 1);
+	momentary_breakpoint_from_master (b, bp_std_terminate, 1,
+					  inferior_thread ()->global_num);
       }
 }
 
@@ -7877,13 +7892,17 @@ enable_breakpoints_after_startup (void)
 
 /* Allocate a new momentary breakpoint.  */
 
+template<typename... Arg>
 static momentary_breakpoint *
-new_momentary_breakpoint (struct gdbarch *gdbarch, enum bptype type)
+new_momentary_breakpoint (struct gdbarch *gdbarch, enum bptype type,
+			  Arg&&... args)
 {
   if (type == bp_longjmp || type == bp_exception)
-    return new longjmp_breakpoint (gdbarch, type);
+    return new longjmp_breakpoint (gdbarch, type,
+				   std::forward<Arg> (args)...);
   else
-    return new momentary_breakpoint (gdbarch, type);
+    return new momentary_breakpoint (gdbarch, type,
+				     std::forward<Arg> (args)...);
 }
 
 /* Set a momentary breakpoint of type TYPE at address specified by
@@ -7899,15 +7918,10 @@ set_momentary_breakpoint (struct gdbarch *gdbarch, struct symtab_and_line sal,
   gdb_assert (!frame_id_artificial_p (frame_id));
 
   std::unique_ptr<momentary_breakpoint> b
-    (new_momentary_breakpoint (gdbarch, type));
+    (new_momentary_breakpoint (gdbarch, type, sal.pspace, frame_id,
+			       inferior_thread ()->global_num));
 
   b->add_location (sal);
-  b->pspace = sal.pspace;
-  b->enable_state = bp_enabled;
-  b->disposition = disp_donttouch;
-  b->frame_id = frame_id;
-
-  b->thread = inferior_thread ()->global_num;
 
   breakpoint_up bp (add_to_breakpoint_chain (std::move (b)));
 
@@ -7923,10 +7937,12 @@ set_momentary_breakpoint (struct gdbarch *gdbarch, struct symtab_and_line sal,
 static struct breakpoint *
 momentary_breakpoint_from_master (struct breakpoint *orig,
 				  enum bptype type,
-				  int loc_enabled)
+				  int loc_enabled,
+				  int thread)
 {
   std::unique_ptr<breakpoint> copy
-    (new_momentary_breakpoint (orig->gdbarch, type));
+    (new_momentary_breakpoint (orig->gdbarch, type, orig->pspace,
+			       orig->frame_id, thread));
   copy->loc = copy->allocate_location ();
   set_breakpoint_location_function (copy->loc);
 
@@ -7939,12 +7955,6 @@ momentary_breakpoint_from_master (struct breakpoint *orig,
   copy->loc->line_number = orig->loc->line_number;
   copy->loc->symtab = orig->loc->symtab;
   copy->loc->enabled = loc_enabled;
-  copy->frame_id = orig->frame_id;
-  copy->thread = orig->thread;
-  copy->pspace = orig->pspace;
-
-  copy->enable_state = bp_enabled;
-  copy->disposition = disp_donttouch;
 
   breakpoint *b = add_to_breakpoint_chain (std::move (copy));
   update_global_location_list_nothrow (UGLL_DONT_INSERT);
@@ -7961,7 +7971,8 @@ clone_momentary_breakpoint (struct breakpoint *orig)
   if (orig == NULL)
     return NULL;
 
-  return momentary_breakpoint_from_master (orig, orig->type, 0);
+  return momentary_breakpoint_from_master (orig, orig->type, 0,
+					   orig->thread);
 }
 
 breakpoint_up
@@ -13389,12 +13400,10 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
   if (tp->control.single_step_breakpoints == NULL)
     {
       std::unique_ptr<breakpoint> b
-	(new momentary_breakpoint (gdbarch, bp_single_step));
-
-      b->disposition = disp_donttouch;
-
-      b->thread = tp->global_num;
-      gdb_assert (b->thread != 0);
+	(new momentary_breakpoint (gdbarch, bp_single_step,
+				   current_program_space,
+				   null_frame_id,
+				   tp->global_num));
 
       tp->control.single_step_breakpoints
 	= add_to_breakpoint_chain (std::move (b));
-- 
2.36.0

[PATCH 22/23] Test "set multiple-symbols on" creating multiple breakpoints

[Pedro Alves]

To look for code paths that lead to create_breakpoints_sal creating
multiple breakpoints, I ran the whole testsuite with this hack:

  --- a/gdb/breakpoint.c
  +++ b/gdb/breakpoint.c
  @@ -8377,8 +8377,7 @@ create_breakpoints_sal (struct gdbarch *gdbarch,
			  int from_tty,
			  int enabled, int internal, unsigned flags)
   {
  -  if (canonical->pre_expanded)
  -    gdb_assert (canonical->lsals.size () == 1);
  +  gdb_assert (canonical->lsals.size () == 1);

surprisingly, the assert never failed...

The way to get to create_breakpoints_sal with multiple lsals is to use
"set multiple-symbols ask" and then select multiple options from the
menu, like so:

 (gdb) set multiple-symbols ask
 (gdb) b overload1arg
 [0] cancel
 [1] all
 [2] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg()
 [3] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(char)
 [4] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(double)
 [5] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(float)
 [6] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(int)
 [7] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(long)
 [8] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(short)
 [9] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(signed char)
 [10] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(unsigned char)
 [11] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(unsigned int)
 [12] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(unsigned long)
 [13] /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc:foo::overload1arg(unsigned short)
 > 2-3
 Breakpoint 2 at 0x1532: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc, line 107.
 Breakpoint 3 at 0x154b: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.cp/ovldbreak.cc, line 110.
 warning: Multiple breakpoints were set.
 Use the "delete" command to delete unwanted breakpoints.

... which would trigger the assert.

This commit makes gdb.cp/ovldbreak.exp test this scenario.  It does
that by making set_bp_overloaded take a list of expected created
breakpoints rather than just one breakpoint.  It converts the
procedure to use gdb_test_multiple instead of send_gdb/gdb_expect
along the way.

Change-Id: Id87d1e08feb6670440d926f5344e5081f5e37c8e
---
 gdb/testsuite/gdb.cp/ovldbreak.exp | 100 +++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 25 deletions(-)

diff --git a/gdb/testsuite/gdb.cp/ovldbreak.exp b/gdb/testsuite/gdb.cp/ovldbreak.exp
index 681edf204d3..06adf82ecbb 100644
--- a/gdb/testsuite/gdb.cp/ovldbreak.exp
+++ b/gdb/testsuite/gdb.cp/ovldbreak.exp
@@ -67,43 +67,75 @@ proc take_gdb_out_of_choice_menu {} {
 
 
 
-# This procedure sets an overloaded breakpoint.
-# When I ask for such a breakpoint, gdb gives me a menu of 'cancel' 'all'
-# and a bunch of choices.  I then choose from that menu by number.
+# This procedure sets an overloaded breakpoint.  When users ask for
+# such a breakpoint, gdb gives a menu of 'cancel' 'all' and one choice
+# per overload.  Users can then choose from that menu by number.
+#
+# NAME is the spec to use to create the breakpoint.  EXPECTEDMENU is
+# the expected menu.  MYCHOICE is the choice selected.  Can be more
+# than one overload, e.g. "2-3".  BPNUMBER is the expected next
+# breakpoint created.  LINENUMBERS is a list of line numbers, one
+# element per expected breakpoint created.
 
-proc set_bp_overloaded {name expectedmenu mychoice bpnumber linenumber} {
-    global gdb_prompt hex srcfile
+proc set_bp_overloaded {name expectedmenu mychoice bpnumber linenumbers} {
+    global gdb_prompt hex decimal srcfile
 
     # Get into the overload menu.
-    send_gdb "break $name\n"
-    gdb_expect {
-        -re "$expectedmenu" {
-            pass "bp menu for $name choice $mychoice"
-
-            # Choose my choice.
-            send_gdb "$mychoice\n"
-            gdb_expect {
-                -re "Breakpoint $bpnumber at $hex: file.*$srcfile, line $linenumber.\r\n$gdb_prompt $" {
-                    pass "set bp $bpnumber on $name $mychoice line $linenumber"
-                }
-                -re ".*$gdb_prompt $" {
-                    fail "set bp $bpnumber on $name $mychoice line $linenumber (bad bp)"
+    gdb_test_multiple "break $name" "bp menu for $name choice $mychoice" {
+	-re "$expectedmenu" {
+	    pass $gdb_test_name
+
+	    set any "\[^\r\n\]*"
+
+	    # True if we've seen a bad breakpoint.
+	    set bad_bp 0
+
+	    # How many breakpoints we expect to see.
+	    set expected_bps [llength $linenumbers]
+
+	    # The count of seen breakpoints.
+	    set seen_bps 0
+
+	    # Choose my choice.
+	    gdb_test_multiple "$mychoice" "set bp $bpnumber on $name $mychoice line $linenumbers" {
+		-re "Breakpoint ($decimal) at $hex: file$any$srcfile, line ($decimal).\r\n" {
+
+		    set got_num $expect_out(1,string)
+		    set got_line $expect_out(2,string)
+
+		    if {$seen_bps >= $expected_bps} {
+			set bad_bp 1
+		    } else {
+			set linenumber [lindex $linenumbers $seen_bps]
+
+			if {$got_num != $bpnumber || $got_line != $linenumber} {
+			    set bad_bp 1
+			}
+
+			incr bpnumber
+			incr seen_bps
+		    }
+		    exp_continue
+		}
+		-re "$gdb_prompt $" {
+		    gdb_assert {!$bad_bp && $seen_bps == $expected_bps} \
+			$gdb_test_name
                 }
                 timeout {
-                    fail "set bp $bpnumber on $name $mychoice line $linenumber (timeout)"
+                    fail "$gdb_test_name (timeout)"
                     take_gdb_out_of_choice_menu
                 }
             }
         }
         -re ".*\r\n> " {
-            fail "bp menu for $name choice $mychoice (bad menu)"
+            fail "$gdb_test_name (bad menu)"
             take_gdb_out_of_choice_menu
         }
         -re ".*$gdb_prompt $" {
-            fail "bp menu for $name choice $mychoice (no menu)"
+            fail "$gdb_test_name (no menu)"
         }
         timeout {
-            fail "bp menu for $name choice $mychoice (timeout)"
+            fail "$gdb_test_name (timeout)"
             take_gdb_out_of_choice_menu
         }
     }
@@ -198,8 +230,10 @@ append menu_overload1arg {[\r\n]*> $}
 # of the multiple-choice menu when breaking on an overloaded method.
 gdb_test_no_output "set multiple-symbols ask"
 
-# Set breakpoints on foo::overload1arg, one by one.
+# The last breakpoint created.
 set bpnum 1
+
+# Set breakpoints on foo::overload1arg, one by one.
 set method "foo::overload1arg"
 for {set idx 0} {$idx < [llength $overloads]} {incr idx} {
     set type [lindex $overloads $idx]
@@ -257,6 +291,21 @@ gdb_expect {
 
 gdb_test "info break" $bptable "breakpoint info (after cancel)"
 
+# Test that if the user selects multiple entries from the option list,
+# GDB creates one breakpoint per entry.
+with_test_prefix "multiple breakpoints" {
+    set method "foo::overload1arg"
+
+    set expected_lines {}
+    for {set i 0} {$i < 2} {incr i} {
+	set type [lindex $overloads $i]
+	lappend expected_lines $line($type_map("$type"))
+    }
+    set_bp_overloaded $method $menu_overload1arg \
+	"2-3" [incr bpnum] $expected_lines
+    incr bpnum
+}
+
 # Delete these breakpoints.
 
 send_gdb "delete breakpoints\n"
@@ -284,6 +333,7 @@ gdb_test "info breakpoints" "No breakpoints or watchpoints." "breakpoint info (a
 # Test choice "all".
 # This is copy-and-paste from set_bp_overloaded.
 
+incr bpnum
 send_gdb "break foo::overload1arg\n" 
 gdb_expect {
     -re "$menu_overload1arg" {
@@ -291,7 +341,7 @@ gdb_expect {
         # Choose all.
         send_gdb "1\n"
         gdb_expect {
-	    -re "Breakpoint $decimal at $hex: foo::overload1arg. .12 locations.\r\n.*$gdb_prompt $" {
+	    -re "Breakpoint $bpnum at $hex: foo::overload1arg. .12 locations.\r\n.*$gdb_prompt $" {
                 pass "set bp on overload1arg all"
             }
             -re ".*$gdb_prompt $" {
@@ -380,7 +430,7 @@ array set might_fail {
 }
 
 foreach type $all_types {
-    continue_to_bp_overloaded 14 $might_fail($type) $line($type) \
+    continue_to_bp_overloaded $bpnum $might_fail($type) $line($type) \
 	$type $arguments($type)
 }
 
-- 
2.36.0

[PATCH 23/23] Rename base_breakpoint -> code_breakpoint

[Pedro Alves]

Even after the previous patches reworking the inheritance of several
breakpoint types, the present breakpoint hierarchy looks a bit
surprising, as we have "breakpoint" as the superclass, and then
"base_breakpoint" inherits from "breakpoint".  Like so, simplified:

   breakpoint
       base_breakpoint
          ordinary_breakpoint
	  internal_breakpoint
	  momentary_breakpoint
	  ada_catchpoint
	  exception_catchpoint
       tracepoint
       watchpoint
       catchpoint
	  exec_catchpoint
	  ...

The surprising part to me is having "base_breakpoint" being a subclass
of "breakpoint".  I'm just refering to naming here -- I mean, you'd
expect that it would be the top level baseclass that would be called
"base".

Just flipping the names of breakpoint and base_breakpoint around
wouldn't be super great for us, IMO, given we think of every type of
*point as a breakpoint at the user visible level.  E.g., "info
breakpoints" shows watchpoints, tracepoints, etc.  So it makes to call
the top level class breakpoint.

Instead, I propose renaming base_breakpoint to code_breakpoint.  The
previous patches made sure that all code breakpoints inherit from
base_breakpoint, so it's fitting.  Also, "code breakpoint" contrasts
nicely with a watchpoint also being typically known as a "data
breakpoint".

After this commit, the resulting hierarchy looks like:

   breakpoint
       code_breakpoint
          ordinary_breakpoint
	  internal_breakpoint
	  momentary_breakpoint
	  ada_catchpoint
	  exception_catchpoint
       tracepoint
       watchpoint
       catchpoint
	  exec_catchpoint
	  ...

... which makes a lot more sense to me.

I've left this patch as last in the series in case people want to
bikeshed on the naming.

"code" has a nice property that it's exactly as many letters as
"base", so this patch didn't require any reindentation.  :-)

Change-Id: Id8dc06683a69fad80d88e674f65e826d6a4e3f66
---
 gdb/ada-lang.c                   |  8 ++---
 gdb/break-catch-throw.c          |  6 ++--
 gdb/breakpoint.c                 | 62 ++++++++++++++++----------------
 gdb/breakpoint.h                 | 16 ++++-----
 gdb/elfread.c                    |  6 ++--
 gdb/mi/mi-cmd-break.c            |  4 +--
 gdb/minsyms.c                    |  4 +--
 gdb/python/py-finishbreakpoint.c |  2 +-
 gdb/symtab.h                     |  6 ++--
 9 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 5ddca104dc3..7e4988be5d0 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12104,7 +12104,7 @@ static std::string ada_exception_catchpoint_cond_string
 
 /* An instance of this type is used to represent an Ada catchpoint.  */
 
-struct ada_catchpoint : public base_breakpoint
+struct ada_catchpoint : public code_breakpoint
 {
   ada_catchpoint (struct gdbarch *gdbarch_,
 		  enum ada_exception_catchpoint_kind kind,
@@ -12113,12 +12113,12 @@ struct ada_catchpoint : public base_breakpoint
 		  bool tempflag,
 		  bool enabled,
 		  bool from_tty)
-    : base_breakpoint (gdbarch_, bp_catchpoint),
+    : code_breakpoint (gdbarch_, bp_catchpoint),
       m_kind (kind)
   {
     add_location (sal);
 
-    /* Unlike most base_breakpoint types, Ada catchpoints are
+    /* Unlike most code_breakpoint types, Ada catchpoints are
        pspace-specific.  */
     gdb_assert (sal.pspace != nullptr);
     this->pspace = sal.pspace;
@@ -12247,7 +12247,7 @@ ada_catchpoint::re_set ()
 {
   /* Call the base class's method.  This updates the catchpoint's
      locations.  */
-  this->base_breakpoint::re_set ();
+  this->code_breakpoint::re_set ();
 
   /* Reparse the exception conditional expressions.  One for each
      location.  */
diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index a6f477b712a..66cf80be1c5 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -65,15 +65,15 @@ static const struct exception_names exception_functions[] =
 
 /* The type of an exception catchpoint.  Unlike most catchpoints, this
    one is implemented with code breakpoints, so it inherits struct
-   base_breakpoint, not struct catchpoint.  */
+   code_breakpoint, not struct catchpoint.  */
 
-struct exception_catchpoint : public base_breakpoint
+struct exception_catchpoint : public code_breakpoint
 {
   exception_catchpoint (struct gdbarch *gdbarch,
 			bool temp, const char *cond_string_,
 			enum exception_event_kind kind_,
 			std::string &&except_rx)
-    : base_breakpoint (gdbarch, bp_catchpoint, temp, cond_string_),
+    : code_breakpoint (gdbarch, bp_catchpoint, temp, cond_string_),
       kind (kind_),
       exception_rx (std::move (except_rx)),
       pattern (exception_rx.empty ()
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f4642c2d8e7..ce425568dd5 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -87,7 +87,7 @@
 static void map_breakpoint_numbers (const char *,
 				    gdb::function_view<void (breakpoint *)>);
 
-static void breakpoint_re_set_default (base_breakpoint *);
+static void breakpoint_re_set_default (code_breakpoint *);
 
 static void
   create_sals_from_location_default (struct event_location *location,
@@ -225,7 +225,7 @@ static void tracepoint_probe_create_sals_from_location
      (struct event_location *location,
       struct linespec_result *canonical);
 
-const struct breakpoint_ops base_breakpoint_ops =
+const struct breakpoint_ops code_breakpoint_ops =
 {
   create_sals_from_location_default,
   create_breakpoints_sal,
@@ -252,7 +252,7 @@ breakpoint::~breakpoint ()
 {
 }
 
-base_breakpoint::~base_breakpoint ()
+code_breakpoint::~code_breakpoint ()
 {
 }
 
@@ -261,9 +261,9 @@ catchpoint::~catchpoint ()
 }
 
 /* The structure to be used in regular breakpoints.  */
-struct ordinary_breakpoint : public base_breakpoint
+struct ordinary_breakpoint : public code_breakpoint
 {
-  using base_breakpoint::base_breakpoint;
+  using code_breakpoint::code_breakpoint;
 
   int resources_needed (const struct bp_location *) override;
   enum print_stop_action print_it (const bpstat *bs) const override;
@@ -275,11 +275,11 @@ struct ordinary_breakpoint : public base_breakpoint
    the program, and they end up installed on the breakpoint chain with
    a negative breakpoint number.  They're visible in "maint info
    breakpoints", but not "info breakpoints".  */
-struct internal_breakpoint : public base_breakpoint
+struct internal_breakpoint : public code_breakpoint
 {
   internal_breakpoint (struct gdbarch *gdbarch,
 		       enum bptype type, CORE_ADDR address)
-    : base_breakpoint (gdbarch, type)
+    : code_breakpoint (gdbarch, type)
   {
     symtab_and_line sal;
     sal.pc = address;
@@ -303,13 +303,13 @@ struct internal_breakpoint : public base_breakpoint
    on the breakpoint chain and they all same the same number (zero).
    They're visible in "maint info breakpoints", but not "info
    breakpoints".  */
-struct momentary_breakpoint : public base_breakpoint
+struct momentary_breakpoint : public code_breakpoint
 {
   momentary_breakpoint (struct gdbarch *gdbarch_, enum bptype bptype,
 			program_space *pspace_,
 			const struct frame_id &frame_id_,
 			int thread_)
-    : base_breakpoint (gdbarch_, bptype)
+    : code_breakpoint (gdbarch_, bptype)
   {
     /* If FRAME_ID is valid, it should be a real frame, not an inlined
        or tail-called one.  */
@@ -1290,11 +1290,11 @@ is_tracepoint (const struct breakpoint *b)
    TYPE.  */
 
 template<typename... Arg>
-static std::unique_ptr<base_breakpoint>
+static std::unique_ptr<code_breakpoint>
 new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type,
 			  Arg&&... args)
 {
-  base_breakpoint *b;
+  code_breakpoint *b;
 
   switch (type)
     {
@@ -1325,7 +1325,7 @@ new_breakpoint_from_type (struct gdbarch *gdbarch, bptype type,
       gdb_assert_not_reached ("invalid type");
     }
 
-  return std::unique_ptr<base_breakpoint> (b);
+  return std::unique_ptr<code_breakpoint> (b);
 }
 
 /* A helper function that validates that COMMANDS are valid for a
@@ -5886,10 +5886,10 @@ bpstat_run_callbacks (bpstat *bs_head)
 	  handle_jit_event (bs->bp_location_at->address);
 	  break;
 	case bp_gnu_ifunc_resolver:
-	  gnu_ifunc_resolver_stop ((base_breakpoint *) b);
+	  gnu_ifunc_resolver_stop ((code_breakpoint *) b);
 	  break;
 	case bp_gnu_ifunc_resolver_return:
-	  gnu_ifunc_resolver_return_stop ((base_breakpoint *) b);
+	  gnu_ifunc_resolver_return_stop ((code_breakpoint *) b);
 	  break;
 	}
     }
@@ -8057,7 +8057,7 @@ handle_automatic_hardware_breakpoints (bp_location *bl)
 }
 
 bp_location *
-base_breakpoint::add_location (const symtab_and_line &sal)
+code_breakpoint::add_location (const symtab_and_line &sal)
 {
   struct bp_location *new_loc, **tmp;
   CORE_ADDR adjusted_address;
@@ -8215,7 +8215,7 @@ update_dprintf_commands (const char *args, int from_tty,
 	update_dprintf_command_list (b);
 }
 
-base_breakpoint::base_breakpoint (struct gdbarch *gdbarch_,
+code_breakpoint::code_breakpoint (struct gdbarch *gdbarch_,
 				  enum bptype type_,
 				  gdb::array_view<const symtab_and_line> sals,
 				  event_location_up &&location_,
@@ -8359,7 +8359,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       int enabled, int internal, unsigned flags,
 		       int display_canonical)
 {
-  std::unique_ptr<base_breakpoint> b
+  std::unique_ptr<code_breakpoint> b
     = new_breakpoint_from_type (gdbarch,
 				type,
 				sals,
@@ -8736,14 +8736,14 @@ breakpoint_ops_for_event_location_type (enum event_location_type location_type,
       if (location_type == PROBE_LOCATION)
 	return &tracepoint_probe_breakpoint_ops;
       else
-	return &base_breakpoint_ops;
+	return &code_breakpoint_ops;
     }
   else
     {
       if (location_type == PROBE_LOCATION)
 	return &bkpt_probe_breakpoint_ops;
       else
-	return &base_breakpoint_ops;
+	return &code_breakpoint_ops;
     }
 }
 
@@ -8756,7 +8756,7 @@ breakpoint_ops_for_event_location (const struct event_location *location,
   if (location != nullptr)
     return breakpoint_ops_for_event_location_type
       (event_location_type (location), is_tracepoint);
-  return &base_breakpoint_ops;
+  return &code_breakpoint_ops;
 }
 
 /* See breakpoint.h.  */
@@ -9091,7 +9091,7 @@ dprintf_command (const char *arg, int from_tty)
 		     0, bp_dprintf,
 		     0 /* Ignore count */,
 		     pending_break_support,
-		     &base_breakpoint_ops,
+		     &code_breakpoint_ops,
 		     from_tty,
 		     1 /* enabled */,
 		     0 /* internal */,
@@ -11465,7 +11465,7 @@ breakpoint::decode_location (struct event_location *location,
 /* Default breakpoint_ops methods.  */
 
 void
-base_breakpoint::re_set ()
+code_breakpoint::re_set ()
 {
   /* FIXME: is this still reachable?  */
   if (breakpoint_event_location_empty_p (this))
@@ -11479,7 +11479,7 @@ base_breakpoint::re_set ()
 }
 
 int
-base_breakpoint::insert_location (struct bp_location *bl)
+code_breakpoint::insert_location (struct bp_location *bl)
 {
   CORE_ADDR addr = bl->target_info.reqstd_address;
 
@@ -11503,7 +11503,7 @@ base_breakpoint::insert_location (struct bp_location *bl)
 }
 
 int
-base_breakpoint::remove_location (struct bp_location *bl,
+code_breakpoint::remove_location (struct bp_location *bl,
 				  enum remove_bp_reason reason)
 {
   if (bl->probe.prob != nullptr)
@@ -11519,7 +11519,7 @@ base_breakpoint::remove_location (struct bp_location *bl,
 }
 
 int
-base_breakpoint::breakpoint_hit (const struct bp_location *bl,
+code_breakpoint::breakpoint_hit (const struct bp_location *bl,
 				 const address_space *aspace,
 				 CORE_ADDR bp_addr,
 				 const target_waitstatus &ws)
@@ -11655,7 +11655,7 @@ ordinary_breakpoint::print_recreate (struct ui_file *fp) const
 }
 
 std::vector<symtab_and_line>
-base_breakpoint::decode_location (struct event_location *location,
+code_breakpoint::decode_location (struct event_location *location,
 				  struct program_space *search_pspace)
 {
   if (event_location_type (location) == PROBE_LOCATION)
@@ -12480,7 +12480,7 @@ hoist_existing_locations (struct breakpoint *b, struct program_space *pspace)
    untouched.  */
 
 void
-update_breakpoint_locations (base_breakpoint *b,
+update_breakpoint_locations (code_breakpoint *b,
 			     struct program_space *filter_pspace,
 			     gdb::array_view<const symtab_and_line> sals,
 			     gdb::array_view<const symtab_and_line> sals_end)
@@ -12688,7 +12688,7 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
    locations.  */
 
 static void
-breakpoint_re_set_default (base_breakpoint *b)
+breakpoint_re_set_default (code_breakpoint *b)
 {
   struct program_space *filter_pspace = current_program_space;
   std::vector<symtab_and_line> expanded, expanded_end;
@@ -13519,7 +13519,7 @@ ftrace_command (const char *arg, int from_tty)
 		     bp_fast_tracepoint /* type_wanted */,
 		     0 /* Ignore count */,
 		     pending_break_support,
-		     &base_breakpoint_ops,
+		     &code_breakpoint_ops,
 		     from_tty,
 		     1 /* enabled */,
 		     0 /* internal */, 0);
@@ -13544,7 +13544,7 @@ strace_command (const char *arg, int from_tty)
     }
   else
     {
-      ops = &base_breakpoint_ops;
+      ops = &code_breakpoint_ops;
       location = string_to_event_location (&arg, current_language);
       type = bp_static_tracepoint;
     }
@@ -13627,7 +13627,7 @@ create_tracepoint_from_upload (struct uploaded_tp *utp)
 			  utp->type /* type_wanted */,
 			  0 /* Ignore count */,
 			  pending_break_support,
-			  &base_breakpoint_ops,
+			  &code_breakpoint_ops,
 			  0 /* from_tty */,
 			  utp->enabled /* enabled */,
 			  0 /* internal */,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 8735396e8d4..566f1285e46 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -849,7 +849,7 @@ struct breakpoint
 /* Abstract base class representing code breakpoints.  User "break"
    breakpoints, internal and momentary breakpoints, etc.  IOW, any
    kind of breakpoint whose locations are created from SALs.  */
-struct base_breakpoint : public breakpoint
+struct code_breakpoint : public breakpoint
 {
   using breakpoint::breakpoint;
 
@@ -857,7 +857,7 @@ struct base_breakpoint : public breakpoint
      description of the location, and COND_STRING as condition
      expression.  If LOCATION is NULL then create an "address
      location" from the address in the SAL.  */
-  base_breakpoint (struct gdbarch *gdbarch, bptype type,
+  code_breakpoint (struct gdbarch *gdbarch, bptype type,
 		   gdb::array_view<const symtab_and_line> sals,
 		   event_location_up &&location,
 		   gdb::unique_xmalloc_ptr<char> filter,
@@ -869,7 +869,7 @@ struct base_breakpoint : public breakpoint
 		   int enabled, unsigned flags,
 		   int display_canonical);
 
-  ~base_breakpoint () override = 0;
+  ~code_breakpoint () override = 0;
 
   /* Add a location for SAL to this breakpoint.  */
   bp_location *add_location (const symtab_and_line &sal);
@@ -985,9 +985,9 @@ extern bool is_exception_catchpoint (breakpoint *bp);
 /* An instance of this type is used to represent all kinds of
    tracepoints.  */
 
-struct tracepoint : public base_breakpoint
+struct tracepoint : public code_breakpoint
 {
-  using base_breakpoint::base_breakpoint;
+  using code_breakpoint::code_breakpoint;
 
   int breakpoint_hit (const struct bp_location *bl,
 		      const address_space *aspace, CORE_ADDR bp_addr,
@@ -1384,7 +1384,7 @@ extern void until_break_command (const char *, int, int);
 /* Initialize a struct bp_location.  */
 
 extern void update_breakpoint_locations
-  (base_breakpoint *b,
+  (code_breakpoint *b,
    struct program_space *filter_pspace,
    gdb::array_view<const symtab_and_line> sals,
    gdb::array_view<const symtab_and_line> sals_end);
@@ -1434,7 +1434,7 @@ extern void awatch_command_wrapper (const char *, int, bool);
 extern void rwatch_command_wrapper (const char *, int, bool);
 extern void tbreak_command (const char *, int);
 
-extern const struct breakpoint_ops base_breakpoint_ops;
+extern const struct breakpoint_ops code_breakpoint_ops;
 
 /* Arguments to pass as context to some catch command handlers.  */
 #define CATCH_PERMANENT ((void *) (uintptr_t) 0)
@@ -1463,7 +1463,7 @@ extern void install_breakpoint (int internal, std::unique_ptr<breakpoint> &&b,
 /* Returns the breakpoint ops appropriate for use with with LOCATION and
    according to IS_TRACEPOINT.  Use this to ensure, for example, that you pass
    the correct ops to create_breakpoint for probe locations.  If LOCATION is
-   NULL, returns base_breakpoint_ops.  */
+   NULL, returns code_breakpoint_ops.  */
 
 extern const struct breakpoint_ops *breakpoint_ops_for_event_location
   (const struct event_location *location, bool is_tracepoint);
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 27203722802..32cb27c8967 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -927,7 +927,7 @@ elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
 /* Handle inferior hit of bp_gnu_ifunc_resolver, see its definition.  */
 
 static void
-elf_gnu_ifunc_resolver_stop (base_breakpoint *b)
+elf_gnu_ifunc_resolver_stop (code_breakpoint *b)
 {
   struct breakpoint *b_return;
   struct frame_info *prev_frame = get_prev_frame (get_current_frame ());
@@ -978,7 +978,7 @@ elf_gnu_ifunc_resolver_stop (base_breakpoint *b)
 /* Handle inferior hit of bp_gnu_ifunc_resolver_return, see its definition.  */
 
 static void
-elf_gnu_ifunc_resolver_return_stop (base_breakpoint *b)
+elf_gnu_ifunc_resolver_return_stop (code_breakpoint *b)
 {
   thread_info *thread = inferior_thread ();
   struct gdbarch *gdbarch = get_frame_arch (get_current_frame ());
@@ -1008,7 +1008,7 @@ elf_gnu_ifunc_resolver_return_stop (base_breakpoint *b)
 			    "gnu-indirect-function breakpoint type %d"),
 			  (int) b->type);
 	}
-      b = (base_breakpoint *) b_next;
+      b = (code_breakpoint *) b_next;
     }
   gdb_assert (b->type == bp_gnu_ifunc_resolver);
   gdb_assert (b->loc->next == NULL);
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 1d9fc0de436..d4a7562b281 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -327,12 +327,12 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
   else if (dprintf)
     {
       type_wanted = bp_dprintf;
-      ops = &base_breakpoint_ops;
+      ops = &code_breakpoint_ops;
     }
   else
     {
       type_wanted = hardware ? bp_hardware_breakpoint : bp_breakpoint;
-      ops = &base_breakpoint_ops;
+      ops = &code_breakpoint_ops;
     }
 
   if (is_explicit)
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 217ee047446..4ec62558b69 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1019,7 +1019,7 @@ stub_gnu_ifunc_resolve_name (const char *function_name,
 /* See elf_gnu_ifunc_resolver_stop for its real implementation.  */
 
 static void
-stub_gnu_ifunc_resolver_stop (base_breakpoint *b)
+stub_gnu_ifunc_resolver_stop (code_breakpoint *b)
 {
   internal_error (__FILE__, __LINE__,
 		  _("elf_gnu_ifunc_resolver_stop cannot be reached."));
@@ -1028,7 +1028,7 @@ stub_gnu_ifunc_resolver_stop (base_breakpoint *b)
 /* See elf_gnu_ifunc_resolver_return_stop for its real implementation.  */
 
 static void
-stub_gnu_ifunc_resolver_return_stop (base_breakpoint *b)
+stub_gnu_ifunc_resolver_return_stop (code_breakpoint *b)
 {
   internal_error (__FILE__, __LINE__,
 		  _("elf_gnu_ifunc_resolver_return_stop cannot be reached."));
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index abd5b48a949..df54d3643ab 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -305,7 +305,7 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 			 bp_breakpoint,
 			 0,
 			 AUTO_BOOLEAN_TRUE,
-			 &base_breakpoint_ops,
+			 &code_breakpoint_ops,
 			 0, 1, internal_bp, 0);
     }
   catch (const gdb_exception &except)
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 5218be587de..ac902a4cbbe 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -55,7 +55,7 @@ struct obj_section;
 struct cmd_list_element;
 class probe;
 struct lookup_name_info;
-struct base_breakpoint;
+struct code_breakpoint;
 
 /* How to match a lookup name against a symbol search name.  */
 enum class symbol_name_match_type
@@ -2228,10 +2228,10 @@ struct gnu_ifunc_fns
 				 CORE_ADDR *function_address_p);
 
   /* See elf_gnu_ifunc_resolver_stop for its real implementation.  */
-  void (*gnu_ifunc_resolver_stop) (base_breakpoint *b);
+  void (*gnu_ifunc_resolver_stop) (code_breakpoint *b);
 
   /* See elf_gnu_ifunc_resolver_return_stop for its real implementation.  */
-  void (*gnu_ifunc_resolver_return_stop) (base_breakpoint *b);
+  void (*gnu_ifunc_resolver_return_stop) (code_breakpoint *b);
 };
 
 #define gnu_ifunc_resolve_addr gnu_ifunc_fns_p->gnu_ifunc_resolve_addr
-- 
2.36.0

Re: [PATCH 09/23] init_breakpoint_sal -> base_breakpoint::base_breakpoint

[Lancelot SIX]

Hi,

Some styling remarks below.

On Mon, May 16, 2022 at 07:40:16PM +0100, Pedro Alves wrote:
> This converts init_breakpoint_sal to a base_breakpoint constructor.
> 
> It removes a use of init_raw_breakpoint.
> 
> To avoid manually adding a bunch of parameters to
> new_breakpoint_from_type, and manually passing them down to the
> constructors of a number of different base_breakpoint subclasses, make
> new_breakpoint_from_type a variable template function.
> 
> Change-Id: I4cc24133ac4c292f547289ec782fc78e5bbe2510
> ---
>  gdb/breakpoint.c | 239 +++++++++++++++++++++++------------------------
>  gdb/breakpoint.h |  16 ++++
>  2 files changed, 133 insertions(+), 122 deletions(-)
> @@ -8326,81 +8331,68 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
>  
>    gdb_assert (!sals.empty ());
>  
> -  for (const auto &sal : sals)
> -    {
> -      struct bp_location *loc;
> +  thread = thread_;
> +  task = task_;
>  
> -      if (from_tty)
> -	{
> -	  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
> -	  if (!loc_gdbarch)
> -	    loc_gdbarch = gdbarch;
> +  cond_string = std::move (cond_string_);
> +  extra_string = std::move (extra_string_);
> +  ignore_count = ignore_count_;
> +  enable_state = enabled_ ? bp_enabled : bp_disabled;
> +  disposition = disposition_;
>  
> -	  describe_other_breakpoints (loc_gdbarch,
> -				      sal.pspace, sal.pc, sal.section, thread);
> -	}
> +  if (type == bp_static_tracepoint
> +      || type == bp_static_marker_tracepoint)
> +    {
> +      auto *t = static_cast <struct tracepoint *> (this);

I think the space before "<" should be removed (I think I saw this in
one of the previous patches too).

> +      struct static_tracepoint_marker marker;
>  
> -      if (&sal == &sals[0])
> +      if (strace_marker_p (this))
>  	{
> -	  init_raw_breakpoint (b, sal, type);
> -	  b->thread = thread;
> -	  b->task = task;
> +	  /* We already know the marker exists, otherwise, we
> +	     wouldn't see a sal for it.  */
> +	  const char *p
> +	    = &event_location_to_string (location_.get ())[3];

Looks like that the statement can fit on one line now.

> +	  const char *endp;
>  
> -	  b->cond_string = std::move (cond_string);
> -	  b->extra_string = std::move (extra_string);
> -	  b->ignore_count = ignore_count;
> -	  b->enable_state = enabled ? bp_enabled : bp_disabled;
> -	  b->disposition = disposition;
> +	  p = skip_spaces (p);
>  
> -	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
> -	    b->loc->inserted = 1;
> +	  endp = skip_to_space (p);
>  
> -	  if (type == bp_static_tracepoint
> -	      || type == bp_static_marker_tracepoint)
> -	    {
> -	      auto *t = static_cast <struct tracepoint *> (b);
> -	      struct static_tracepoint_marker marker;
> -
> -	      if (strace_marker_p (b))
> -		{
> -		  /* We already know the marker exists, otherwise, we
> -		     wouldn't see a sal for it.  */
> -		  const char *p
> -		    = &event_location_to_string (b->location.get ())[3];
> -		  const char *endp;
> -
> -		  p = skip_spaces (p);
> -
> -		  endp = skip_to_space (p);
> -
> -		  t->static_trace_marker_id.assign (p, endp - p);
> -
> -		  gdb_printf (_("Probed static tracepoint "
> -				"marker \"%s\"\n"),
> -			      t->static_trace_marker_id.c_str ());
> -		}
> -	      else if (target_static_tracepoint_marker_at (sal.pc, &marker))
> -		{
> -		  t->static_trace_marker_id = std::move (marker.str_id);
> +	  t->static_trace_marker_id.assign (p, endp - p);
>  
> -		  gdb_printf (_("Probed static tracepoint "
> -				"marker \"%s\"\n"),
> -			      t->static_trace_marker_id.c_str ());
> -		}
> -	      else
> -		warning (_("Couldn't determine the static "
> -			   "tracepoint marker to probe"));
> -	    }
> +	  gdb_printf (_("Probed static tracepoint "
> +			"marker \"%s\"\n"),

This literal string can also fit into one line, no need to split it.

> +		      t->static_trace_marker_id.c_str ());
> +	}
> +      else if (target_static_tracepoint_marker_at (sals[0].pc, &marker))
> +	{
> +	  t->static_trace_marker_id = std::move (marker.str_id);
>  
> -	  loc = b->loc;
> +	  gdb_printf (_("Probed static tracepoint "
> +			"marker \"%s\"\n"),

Same here.

> +		      t->static_trace_marker_id.c_str ());
>  	}
>        else
> +	warning (_("Couldn't determine the static "
> +		   "tracepoint marker to probe"));

And probably here too.

> +    }
> +
> +  for (const auto &sal : sals)
> +    {
> +      if (from_tty)
>  	{
> -	  loc = b->add_location (sal);
> -	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
> -	    loc->inserted = 1;
> +	  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
> +	  if (!loc_gdbarch)

Should be "loc_gdbarch == nullptr"

> +	    loc_gdbarch = gdbarch;
> +
> +	  describe_other_breakpoints (loc_gdbarch,
> +				      sal.pspace, sal.pc, sal.section, thread);
>  	}
>  
> +      bp_location *new_loc = add_location (sal);
> +      if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
> +	new_loc->inserted = 1;
> +
>        /* Do not set breakpoint locations conditions yet.  As locations
>  	 are inserted, they get sorted based on their addresses.  Let
>  	 the list stabilize to have reliable location numbers.  */

Also, down the line, there are some leftover NULL which could become
nullptr.  This is from code which just god re-indented, but while at
updating it, it could be nice to fix this too.

Best,
Lancelot.

Re: [PATCH 20/23] Momentary breakpoints should have no breakpoint number

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Momentary breakpoints have no breakpoint number, their breakpoint
Pedro> number should be always 0, to avoid constantly incrementing (or
Pedro> decrementing) the internal breakpoint count.

Pedro> Indeed, set_momentary_breakpoint installs the created breakpoint
Pedro> without a number.

Pedro> However, momentary_breakpoint_from_master incorrectly gives an
Pedro> internal breakpoint number to the new breakpoint.  This commit fixes
Pedro> that.

I think it would probably be good to document this somewhere.
Like, maybe the deleted line could be replaced with a comment.

Tom

Re: [PATCH 23/23] Rename base_breakpoint -> code_breakpoint

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Instead, I propose renaming base_breakpoint to code_breakpoint.  The
Pedro> previous patches made sure that all code breakpoints inherit from
Pedro> base_breakpoint, so it's fitting.  Also, "code breakpoint" contrasts
Pedro> nicely with a watchpoint also being typically known as a "data
Pedro> breakpoint".

Seems reasonable to me.

Pedro> I've left this patch as last in the series in case people want to
Pedro> bikeshed on the naming.

I think this name is fine.

Tom

Re: [PATCH 00/23] More breakpoints cleanups

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> The main goal of this series was to eliminate init_raw_breakpoint.
Pedro> It slowly tweaks a number of things to be able to do that.

I looked through these again today, and sent one minor comment.
This all looks good to me, thank you for doing it.

Tom

Re: [PATCH 09/23] init_breakpoint_sal -> base_breakpoint::base_breakpoint

[Pedro Alves]

On 2022-05-20 14:40, Lancelot SIX wrote:
> Hi,
> 
> Some styling remarks below.
> 
> On Mon, May 16, 2022 at 07:40:16PM +0100, Pedro Alves wrote:
>> This converts init_breakpoint_sal to a base_breakpoint constructor.
>>
>> It removes a use of init_raw_breakpoint.
>>
>> To avoid manually adding a bunch of parameters to
>> new_breakpoint_from_type, and manually passing them down to the
>> constructors of a number of different base_breakpoint subclasses, make
>> new_breakpoint_from_type a variable template function.
>>
>> Change-Id: I4cc24133ac4c292f547289ec782fc78e5bbe2510
>> ---
>>  gdb/breakpoint.c | 239 +++++++++++++++++++++++------------------------
>>  gdb/breakpoint.h |  16 ++++
>>  2 files changed, 133 insertions(+), 122 deletions(-)
>> @@ -8326,81 +8331,68 @@ init_breakpoint_sal (base_breakpoint *b, struct gdbarch *gdbarch,
>>  
>>    gdb_assert (!sals.empty ());
>>  
>> -  for (const auto &sal : sals)
>> -    {
>> -      struct bp_location *loc;
>> +  thread = thread_;
>> +  task = task_;
>>  
>> -      if (from_tty)
>> -	{
>> -	  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
>> -	  if (!loc_gdbarch)
>> -	    loc_gdbarch = gdbarch;
>> +  cond_string = std::move (cond_string_);
>> +  extra_string = std::move (extra_string_);
>> +  ignore_count = ignore_count_;
>> +  enable_state = enabled_ ? bp_enabled : bp_disabled;
>> +  disposition = disposition_;
>>  
>> -	  describe_other_breakpoints (loc_gdbarch,
>> -				      sal.pspace, sal.pc, sal.section, thread);
>> -	}
>> +  if (type == bp_static_tracepoint
>> +      || type == bp_static_marker_tracepoint)
>> +    {
>> +      auto *t = static_cast <struct tracepoint *> (this);
> 
> I think the space before "<" should be removed (I think I saw this in
> one of the previous patches too).

Thanks, I fixed it in the patch that introduced it (patch #6).

I then did:

 $ git diff upstream/master... | grep static_cast
 +      auto *t = static_cast<struct tracepoint *> (this);
 +    = static_cast<momentary_breakpoint *> (tp->control.single_step_breakpoints);

with the whole series applied, so it looks like this was the only case.

> 
>> +      struct static_tracepoint_marker marker;
>>  
>> -      if (&sal == &sals[0])
>> +      if (strace_marker_p (this))
>>  	{
>> -	  init_raw_breakpoint (b, sal, type);
>> -	  b->thread = thread;
>> -	  b->task = task;
>> +	  /* We already know the marker exists, otherwise, we
>> +	     wouldn't see a sal for it.  */
>> +	  const char *p
>> +	    = &event_location_to_string (location_.get ())[3];
> 
> Looks like that the statement can fit on one line now.

Done.

> 
>> +	  const char *endp;
>>  
>> -	  b->cond_string = std::move (cond_string);
>> -	  b->extra_string = std::move (extra_string);
>> -	  b->ignore_count = ignore_count;
>> -	  b->enable_state = enabled ? bp_enabled : bp_disabled;
>> -	  b->disposition = disposition;
>> +	  p = skip_spaces (p);
>>  
>> -	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
>> -	    b->loc->inserted = 1;
>> +	  endp = skip_to_space (p);
>>  
>> -	  if (type == bp_static_tracepoint
>> -	      || type == bp_static_marker_tracepoint)
>> -	    {
>> -	      auto *t = static_cast <struct tracepoint *> (b);
>> -	      struct static_tracepoint_marker marker;
>> -
>> -	      if (strace_marker_p (b))
>> -		{
>> -		  /* We already know the marker exists, otherwise, we
>> -		     wouldn't see a sal for it.  */
>> -		  const char *p
>> -		    = &event_location_to_string (b->location.get ())[3];
>> -		  const char *endp;
>> -
>> -		  p = skip_spaces (p);
>> -
>> -		  endp = skip_to_space (p);
>> -
>> -		  t->static_trace_marker_id.assign (p, endp - p);
>> -
>> -		  gdb_printf (_("Probed static tracepoint "
>> -				"marker \"%s\"\n"),
>> -			      t->static_trace_marker_id.c_str ());
>> -		}
>> -	      else if (target_static_tracepoint_marker_at (sal.pc, &marker))
>> -		{
>> -		  t->static_trace_marker_id = std::move (marker.str_id);
>> +	  t->static_trace_marker_id.assign (p, endp - p);
>>  
>> -		  gdb_printf (_("Probed static tracepoint "
>> -				"marker \"%s\"\n"),
>> -			      t->static_trace_marker_id.c_str ());
>> -		}
>> -	      else
>> -		warning (_("Couldn't determine the static "
>> -			   "tracepoint marker to probe"));
>> -	    }
>> +	  gdb_printf (_("Probed static tracepoint "
>> +			"marker \"%s\"\n"),
> 
> This literal string can also fit into one line, no need to split it.

Done.

> 
>> +		      t->static_trace_marker_id.c_str ());
>> +	}
>> +      else if (target_static_tracepoint_marker_at (sals[0].pc, &marker))
>> +	{
>> +	  t->static_trace_marker_id = std::move (marker.str_id);
>>  
>> -	  loc = b->loc;
>> +	  gdb_printf (_("Probed static tracepoint "
>> +			"marker \"%s\"\n"),
> 
> Same here.

Done.

> 
>> +		      t->static_trace_marker_id.c_str ());
>>  	}
>>        else
>> +	warning (_("Couldn't determine the static "
>> +		   "tracepoint marker to probe"));
> 
> And probably here too.

Done.

> 
>> +    }
>> +
>> +  for (const auto &sal : sals)
>> +    {
>> +      if (from_tty)
>>  	{
>> -	  loc = b->add_location (sal);
>> -	  if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
>> -	    loc->inserted = 1;
>> +	  struct gdbarch *loc_gdbarch = get_sal_arch (sal);
>> +	  if (!loc_gdbarch)
> 
> Should be "loc_gdbarch == nullptr"
> 
>> +	    loc_gdbarch = gdbarch;

Done.

>> +
>> +	  describe_other_breakpoints (loc_gdbarch,
>> +				      sal.pspace, sal.pc, sal.section, thread);
>>  	}
>>  
>> +      bp_location *new_loc = add_location (sal);
>> +      if ((flags & CREATE_BREAKPOINT_FLAGS_INSERTED) != 0)
>> +	new_loc->inserted = 1;
>> +
>>        /* Do not set breakpoint locations conditions yet.  As locations
>>  	 are inserted, they get sorted based on their addresses.  Let
>>  	 the list stabilize to have reliable location numbers.  */
> 
> Also, down the line, there are some leftover NULL which could become
> nullptr.  This is from code which just god re-indented, but while at
> updating it, it could be nice to fix this too.

All done locally.

Thanks!

Re: [PATCH 20/23] Momentary breakpoints should have no breakpoint number

[Pedro Alves]

On 2022-05-20 17:00, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> Momentary breakpoints have no breakpoint number, their breakpoint
> Pedro> number should be always 0, to avoid constantly incrementing (or
> Pedro> decrementing) the internal breakpoint count.
> 
> Pedro> Indeed, set_momentary_breakpoint installs the created breakpoint
> Pedro> without a number.
> 
> Pedro> However, momentary_breakpoint_from_master incorrectly gives an
> Pedro> internal breakpoint number to the new breakpoint.  This commit fixes
> Pedro> that.
> 
> I think it would probably be good to document this somewhere.
> Like, maybe the deleted line could be replaced with a comment.

I think if we do it here, we'd need to do it in set_momentary_breakpoint
too.  I had already added such a comment on top of struct momentary_breakpoint,
though, in patch #19:

-/* Momentary breakpoints.  */
+/* Momentary breakpoints.  These typically have a lifetime of some run
+   control command only, are always thread-specific, and have 0 for
+   breakpoint number.  I.e., there can be many momentary breakpoints
+   on the breakpoint chain and they all same the same number (zero).
+   They're visible in "maint info breakpoints", but not "info
+   breakpoints".  */
 struct momentary_breakpoint : public base_breakpoint
 {

Maybe we could find a spot to put an assert at.  Seems difficult without
relying on checking breakpoint types, though...

Re: [PATCH 00/23] More breakpoints cleanups

[Pedro Alves]

On 2022-05-20 17:06, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> The main goal of this series was to eliminate init_raw_breakpoint.
> Pedro> It slowly tweaks a number of things to be able to do that.
> 
> I looked through these again today, and sent one minor comment.
> This all looks good to me, thank you for doing it.

Thanks Tom.  I've merged this after addressing comments and retesting.