[PATCH 0/3] More minor breakpoint cleanups

[PATCH 0/3] More minor breakpoint cleanups

[Tom Tromey]

I was curious if it is possible to remove some of the "pure virtual"
methods from breakpoint.  In the end, I managed to do this for one of
them by rearranging some code a little.

Regression tested on x86-64 Fedora 34.

Tom

[PATCH 1/3] Change breakpoint_re_set_default to a method

[Tom Tromey]

breakpoint_re_set_default is only ever called from breakpoint re_set
methods, so make it a protected method on code_breakpoint.
---
 gdb/breakpoint.c | 18 ++++++++----------
 gdb/breakpoint.h |  5 +++++
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ed932a19ed7..cc0d527fd30 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -87,8 +87,6 @@
 static void map_breakpoint_numbers (const char *,
 				    gdb::function_view<void (breakpoint *)>);
 
-static void breakpoint_re_set_default (code_breakpoint *);
-
 static void
   create_sals_from_location_default (struct event_location *location,
 				     struct linespec_result *canonical);
@@ -11496,7 +11494,7 @@ code_breakpoint::re_set ()
       return;
     }
 
-  breakpoint_re_set_default (this);
+  re_set_default ();
 }
 
 int
@@ -11932,7 +11930,7 @@ tracepoint_probe_create_sals_from_location
 void
 dprintf_breakpoint::re_set ()
 {
-  breakpoint_re_set_default (this);
+  re_set_default ();
 
   /* extra_string should never be non-NULL for dprintf.  */
   gdb_assert (extra_string != NULL);
@@ -12708,28 +12706,28 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
    breakpoints.  Reevaluate the breakpoint and recreate its
    locations.  */
 
-static void
-breakpoint_re_set_default (code_breakpoint *b)
+void
+code_breakpoint::re_set_default ()
 {
   struct program_space *filter_pspace = current_program_space;
   std::vector<symtab_and_line> expanded, expanded_end;
 
   int found;
-  std::vector<symtab_and_line> sals = location_to_sals (b, b->location.get (),
+  std::vector<symtab_and_line> sals = location_to_sals (this, location.get (),
 							filter_pspace, &found);
   if (found)
     expanded = std::move (sals);
 
-  if (b->location_range_end != NULL)
+  if (location_range_end != NULL)
     {
       std::vector<symtab_and_line> sals_end
-	= location_to_sals (b, b->location_range_end.get (),
+	= location_to_sals (this, location_range_end.get (),
 			    filter_pspace, &found);
       if (found)
 	expanded_end = std::move (sals_end);
     }
 
-  update_breakpoint_locations (b, filter_pspace, expanded, expanded_end);
+  update_breakpoint_locations (this, filter_pspace, expanded, expanded_end);
 }
 
 /* Default method for creating SALs from an address string.  It basically
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 566f1285e46..4fdd50324e3 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -885,6 +885,11 @@ struct code_breakpoint : public breakpoint
   std::vector<symtab_and_line> decode_location
        (struct event_location *location,
 	struct program_space *search_pspace) override;
+
+protected:
+
+  /* Helper method that does the basic work of re_set.  */
+  void re_set_default ();
 };
 
 /* An instance of this type is used to represent a watchpoint,
-- 
2.34.1

Re: [PATCH 1/3] Change breakpoint_re_set_default to a method

[Pedro Alves]

On 2022-05-28 03:42, Tom Tromey wrote:
> breakpoint_re_set_default is only ever called from breakpoint re_set
> methods, so make it a protected method on code_breakpoint.

OK.

[PATCH 2/3] Change location_to_sals to a method

[Tom Tromey]

location_to_sals is only ever called for code breakpoints, so make it
a protected method there.
---
 gdb/breakpoint.c | 58 ++++++++++++++++++++++++------------------------
 gdb/breakpoint.h |  8 +++++++
 2 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index cc0d527fd30..ee34733e6cd 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -12621,9 +12621,10 @@ update_breakpoint_locations (code_breakpoint *b,
 /* Find the SaL locations corresponding to the given LOCATION.
    On return, FOUND will be 1 if any SaL was found, zero otherwise.  */
 
-static std::vector<symtab_and_line>
-location_to_sals (struct breakpoint *b, struct event_location *location,
-		  struct program_space *search_pspace, int *found)
+std::vector<symtab_and_line>
+code_breakpoint::location_to_sals (struct event_location *location,
+				   struct program_space *search_pspace,
+				   int *found)
 {
   struct gdb_exception exception;
 
@@ -12631,7 +12632,7 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
 
   try
     {
-      sals = b->decode_location (location, search_pspace);
+      sals = decode_location (location, search_pspace);
     }
   catch (gdb_exception_error &e)
     {
@@ -12645,13 +12646,13 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
 	 breakpoint being disabled, and don't want to see more
 	 errors.  */
       if (e.error == NOT_FOUND_ERROR
-	  && (b->condition_not_parsed
-	      || (b->loc != NULL
+	  && (condition_not_parsed
+	      || (loc != NULL
 		  && search_pspace != NULL
-		  && b->loc->pspace != search_pspace)
-	      || (b->loc && b->loc->shlib_disabled)
-	      || (b->loc && b->loc->pspace->executing_startup)
-	      || b->enable_state == bp_disabled))
+		  && loc->pspace != search_pspace)
+	      || (loc && loc->shlib_disabled)
+	      || (loc && loc->pspace->executing_startup)
+	      || enable_state == bp_disabled))
 	not_found_and_ok = 1;
 
       if (!not_found_and_ok)
@@ -12662,7 +12663,7 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
 	     have separate 'warning emitted' flag.  Since this
 	     happens only when a binary has changed, I don't know
 	     which approach is better.  */
-	  b->enable_state = bp_disabled;
+	  enable_state = bp_disabled;
 	  throw;
 	}
 
@@ -12673,26 +12674,26 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
     {
       for (auto &sal : sals)
 	resolve_sal_pc (&sal);
-      if (b->condition_not_parsed && b->extra_string != NULL)
+      if (condition_not_parsed && extra_string != NULL)
 	{
-	  gdb::unique_xmalloc_ptr<char> cond_string, extra_string;
+	  gdb::unique_xmalloc_ptr<char> local_cond, local_extra;
 	  int thread, task;
 
-	  find_condition_and_thread_for_sals (sals, b->extra_string.get (),
-					      &cond_string, &thread,
-					      &task, &extra_string);
-	  gdb_assert (b->cond_string == NULL);
-	  if (cond_string)
-	    b->cond_string = std::move (cond_string);
-	  b->thread = thread;
-	  b->task = task;
-	  if (extra_string)
-	    b->extra_string = std::move (extra_string);
-	  b->condition_not_parsed = 0;
+	  find_condition_and_thread_for_sals (sals, extra_string.get (),
+					      &local_cond, &thread,
+					      &task, &local_extra);
+	  gdb_assert (cond_string == nullptr);
+	  if (local_cond != nullptr)
+	    cond_string = std::move (local_cond);
+	  thread = thread;
+	  task = task;
+	  if (local_extra != nullptr)
+	    extra_string = std::move (local_extra);
+	  condition_not_parsed = 0;
 	}
 
-      if (b->type == bp_static_tracepoint)
-	sals[0] = update_static_tracepoint (b, sals[0]);
+      if (type == bp_static_tracepoint)
+	sals[0] = update_static_tracepoint (this, sals[0]);
 
       *found = 1;
     }
@@ -12713,7 +12714,7 @@ code_breakpoint::re_set_default ()
   std::vector<symtab_and_line> expanded, expanded_end;
 
   int found;
-  std::vector<symtab_and_line> sals = location_to_sals (this, location.get (),
+  std::vector<symtab_and_line> sals = location_to_sals (location.get (),
 							filter_pspace, &found);
   if (found)
     expanded = std::move (sals);
@@ -12721,8 +12722,7 @@ code_breakpoint::re_set_default ()
   if (location_range_end != NULL)
     {
       std::vector<symtab_and_line> sals_end
-	= location_to_sals (this, location_range_end.get (),
-			    filter_pspace, &found);
+	= location_to_sals (location_range_end.get (), filter_pspace, &found);
       if (found)
 	expanded_end = std::move (sals_end);
     }
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 4fdd50324e3..5ce6edf6efc 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -890,6 +890,14 @@ struct code_breakpoint : public breakpoint
 
   /* Helper method that does the basic work of re_set.  */
   void re_set_default ();
+
+  /* Find the SaL locations corresponding to the given LOCATION.
+     On return, FOUND will be 1 if any SaL was found, zero otherwise.  */
+
+  std::vector<symtab_and_line> location_to_sals
+       (struct event_location *location,
+	struct program_space *search_pspace,
+	int *found);
 };
 
 /* An instance of this type is used to represent a watchpoint,
-- 
2.34.1

Re: [PATCH 2/3] Change location_to_sals to a method

[Pedro Alves]

On 2022-05-28 03:42, Tom Tromey wrote:

>  
> @@ -12673,26 +12674,26 @@ location_to_sals (struct breakpoint *b, struct event_location *location,
>      {
>        for (auto &sal : sals)
>  	resolve_sal_pc (&sal);
> -      if (b->condition_not_parsed && b->extra_string != NULL)
> +      if (condition_not_parsed && extra_string != NULL)
>  	{
> -	  gdb::unique_xmalloc_ptr<char> cond_string, extra_string;
> +	  gdb::unique_xmalloc_ptr<char> local_cond, local_extra;
>  	  int thread, task;

These thread/task are masking the breakpoint fields, and then ...

>  
> -	  find_condition_and_thread_for_sals (sals, b->extra_string.get (),
> -					      &cond_string, &thread,
> -					      &task, &extra_string);
> -	  gdb_assert (b->cond_string == NULL);
> -	  if (cond_string)
> -	    b->cond_string = std::move (cond_string);
> -	  b->thread = thread;
> -	  b->task = task;
> -	  if (extra_string)
> -	    b->extra_string = std::move (extra_string);
> -	  b->condition_not_parsed = 0;
> +	  find_condition_and_thread_for_sals (sals, extra_string.get (),
> +					      &local_cond, &thread,
> +					      &task, &local_extra);
> +	  gdb_assert (cond_string == nullptr);
> +	  if (local_cond != nullptr)
> +	    cond_string = std::move (local_cond);
> +	  thread = thread;
> +	  task = task;

... here you end up with self assignments.

Re: [PATCH 2/3] Change location_to_sals to a method

[Tom Tromey]

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

>> int thread, task;

Pedro> These thread/task are masking the breakpoint fields, and then ...

Ugh, I even looked for this.

I looked at turning on -Wshadow instead of -Wshadow=local, since that
would fix this problem.  It's a bit of a slog, and we'd be at the mercy
of system headers too, but maybe we want to do this?

Meanwhile I filed a gcc feature request to add a new -Wshadow that is
like local but also handles data members:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105798

Tom

Re: [PATCH 2/3] Change location_to_sals to a method

[Pedro Alves]

On 2022-06-01 05:50, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
>>> int thread, task;
> 
> Pedro> These thread/task are masking the breakpoint fields, and then ...
> 
> Ugh, I even looked for this.
> 
> I looked at turning on -Wshadow instead of -Wshadow=local, since that
> would fix this problem.  It's a bit of a slog, and we'd be at the mercy
> of system headers too, but maybe we want to do this?

ISTR that we tried -Wshadow early on, and it ran into lots of issues with
system headers.  Like, e.g., there's a system function named "index" in strings.h,
which is easy to run into.  

And then there were also with issues with variables shadowing functions.

ISTR Alan filing a GCC bug about it, to ask to not warn about shadowing
system header stuff, or maybe it was about not warning about variables
shadowing functions.  

/me searches.

Ah, here, it was actually a GCC fix:

  https://gcc.gnu.org/legacy-ml/gcc-patches/2011-08/msg02017.html

That predates gcc 4.8, I think, our oldest supported GCC version,
so we can assume all GCCs have that.  

OTOH, I'm not sure that patch affects only the C frontend, or whether
that code is shared with the C++ frontend.  g++ maybe behaves differently,
but hopefully not.

AFAICS, bfd uses -Wshadow, not -Wshadow=local.

Searching in my gdb-patches folder, I found a ton of -Wshadow patches,
from Andrey Smirnov from back in 2011.  I had forgotten how many those were.
It was a series with over 300 patches.  I think in the end most were dropped,
for handling system header conflicts, which was found not worth it.

Maybe we could give -Wshadow another try.

> 
> Meanwhile I filed a gcc feature request to add a new -Wshadow that is
> like local but also handles data members:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105798
> 
> Tom
> 

Re: [PATCH 2/3] Change location_to_sals to a method

[Tom Tromey]

Pedro> Maybe we could give -Wshadow another try.

I looked into this.  I got pretty far along, and it wouldn't be too hard
to finish.  However, GCC does issue complaints about some conflicts that
don't and probably wouldn't ever find real bugs.

As simple example is that the 'gdbarch_tdep' function conflicts with
'struct gdbarch_tdep' (or maybe with its purported constructor).  There
are a surprising number of these because it's common to give the same
name to a frame cache struct and corresponding unwinding function.

Anyway, I can finish the series if you think it's worthwhile.
I started hacking up a compiler patch instead but TBH I doubt I'll
finish that.

Tom

Re: [PATCH 2/3] Change location_to_sals to a method

[Pedro Alves]

On 2022-06-10 22:34, Tom Tromey wrote:
> Pedro> Maybe we could give -Wshadow another try.
> 
> I looked into this.  I got pretty far along, and it wouldn't be too hard
> to finish.  However, GCC does issue complaints about some conflicts that
> don't and probably wouldn't ever find real bugs.
> 
> As simple example is that the 'gdbarch_tdep' function conflicts with
> 'struct gdbarch_tdep' (or maybe with its purported constructor).  There
> are a surprising number of these because it's common to give the same
> name to a frame cache struct and corresponding unwinding function.

Ouch.  :-/  

I guess this may run into "struct gdbarch *gdbarch" issues too, if
struct gdbarch ever gained a constructor?

> 
> Anyway, I can finish the series if you think it's worthwhile.

Not really sure.

> I started hacking up a compiler patch instead but TBH I doubt I'll
> finish that.

The compiler idea seemed promising to me.  Maybe someone else picks it
up.

Re: [PATCH 2/3] Change location_to_sals to a method

[Tom Tromey]

>> int thread, task;

Pedro> These thread/task are masking the breakpoint fields, and then ...
...
Pedro> ... here you end up with self assignments.

I've fixed these.

In the end my -Wshadow experiments seem half-baked, so I haven't pursued
them.  I do have a compiler hack, but it's not suitable for putting into
the compiler.  Also it causes a lot of somewhat questionable changes,
like requiring us to rename the parameters to constructors in a lot of
places.

Tom

[PATCH 3/3] Move decode_location to code_breakpoint

[Tom Tromey]

breakpoint::decode_location just asserts if called.  It turned out to
be relatively easy to remove this method from breakpoint and instead
move the base implementation to code_breakpoint.
---
 gdb/breakpoint.c |  7 -------
 gdb/breakpoint.h | 23 ++++++++++-------------
 2 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ee34733e6cd..8c6507ae9fd 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11474,13 +11474,6 @@ breakpoint::print_recreate (struct ui_file *fp) const
   internal_error_pure_virtual_called ();
 }
 
-std::vector<symtab_and_line>
-breakpoint::decode_location (struct event_location *location,
-			     struct program_space *search_pspace)
-{
-  internal_error_pure_virtual_called ();
-}
-
 /* Default breakpoint_ops methods.  */
 
 void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 5ce6edf6efc..663c939c162 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -709,16 +709,6 @@ struct breakpoint
   /* Print to FP the CLI command that recreates this breakpoint.  */
   virtual void print_recreate (struct ui_file *fp) const;
 
-  /* Given the location (second parameter), this method decodes it and
-     returns the SAL locations related to it.  For ordinary
-     breakpoints, it calls `decode_line_full'.  If SEARCH_PSPACE is
-     not NULL, symbol search is restricted to just that program space.
-
-     This function is called inside `location_to_sals'.  */
-  virtual std::vector<symtab_and_line> decode_location
-    (struct event_location *location,
-     struct program_space *search_pspace);
-
   /* Return true if this breakpoint explains a signal.  See
      bpstat_explains_signal.  */
   virtual bool explains_signal (enum gdb_signal)
@@ -882,12 +872,19 @@ struct code_breakpoint : public breakpoint
 		      const address_space *aspace,
 		      CORE_ADDR bp_addr,
 		      const target_waitstatus &ws) override;
-  std::vector<symtab_and_line> decode_location
-       (struct event_location *location,
-	struct program_space *search_pspace) override;
 
 protected:
 
+  /* Given the location (second parameter), this method decodes it and
+     returns the SAL locations related to it.  For ordinary
+     breakpoints, it calls `decode_line_full'.  If SEARCH_PSPACE is
+     not NULL, symbol search is restricted to just that program space.
+
+     This function is called inside `location_to_sals'.  */
+  virtual std::vector<symtab_and_line> decode_location
+    (struct event_location *location,
+     struct program_space *search_pspace);
+
   /* Helper method that does the basic work of re_set.  */
   void re_set_default ();
 
-- 
2.34.1

Re: [PATCH 3/3] Move decode_location to code_breakpoint

[Pedro Alves]

On 2022-05-28 03:42, Tom Tromey wrote:

> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 5ce6edf6efc..663c939c162 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -709,16 +709,6 @@ struct breakpoint
>    /* Print to FP the CLI command that recreates this breakpoint.  */
>    virtual void print_recreate (struct ui_file *fp) const;
>  
> -  /* Given the location (second parameter), this method decodes it and
> -     returns the SAL locations related to it.  For ordinary
> -     breakpoints, it calls `decode_line_full'.  If SEARCH_PSPACE is
> -     not NULL, symbol search is restricted to just that program space.
> -
> -     This function is called inside `location_to_sals'.  */
> -  virtual std::vector<symtab_and_line> decode_location
> -    (struct event_location *location,
> -     struct program_space *search_pspace);
> -
>    /* Return true if this breakpoint explains a signal.  See
>       bpstat_explains_signal.  */
>    virtual bool explains_signal (enum gdb_signal)
> @@ -882,12 +872,19 @@ struct code_breakpoint : public breakpoint
>  		      const address_space *aspace,
>  		      CORE_ADDR bp_addr,
>  		      const target_waitstatus &ws) override;
> -  std::vector<symtab_and_line> decode_location
> -       (struct event_location *location,
> -	struct program_space *search_pspace) override;
>  
>  protected:
>  
> +  /* Given the location (second parameter), this method decodes it and

I've noticed that "second" here got stale when this was first
converted to a method.  I think we should just remove the parenthetical
remark and be done with it.  Would you mind doing that while at it?

The patch looks OK, regardless.

Pedro Alves

> +     returns the SAL locations related to it.  For ordinary
> +     breakpoints, it calls `decode_line_full'.  If SEARCH_PSPACE is
> +     not NULL, symbol search is restricted to just that program space.
> +
> +     This function is called inside `location_to_sals'.  */
> +  virtual std::vector<symtab_and_line> decode_location
> +    (struct event_location *location,
> +     struct program_space *search_pspace);
> +
>    /* Helper method that does the basic work of re_set.  */
>    void re_set_default ();
>  

Re: [PATCH 3/3] Move decode_location to code_breakpoint

[Tom Tromey]

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

>> +  /* Given the location (second parameter), this method decodes it and

Pedro> I've noticed that "second" here got stale when this was first
Pedro> converted to a method.  I think we should just remove the parenthetical
Pedro> remark and be done with it.  Would you mind doing that while at it?

Pedro> The patch looks OK, regardless.

I made this change.

Tom

Re: [PATCH 0/3] More minor breakpoint cleanups

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I was curious if it is possible to remove some of the "pure virtual"
Tom> methods from breakpoint.  In the end, I managed to do this for one of
Tom> them by rearranging some code a little.

Tom> Regression tested on x86-64 Fedora 34.

I've updated this per the review, rebased it, and renamed a few things
(because the baseline changed).  I've re-regression tested it and I am
checking it in.

Tom