gdb: create_breakpoint: asserts relating to extra_string/parse_extra

The goal of this commit is to better define the API for
create_breakpoint especially around the use of extra_string and
parse_extra.  This will be useful in the next commit when I plan to
make some changes to create_breakpoint.

This commit makes one possibly breaking change: until this commit it
was possible to create thread-specific dprintf breakpoint like this:

  (gdb) dprintf call_me, thread 1 "%s", "hello"
  Dprintf 2 at 0x401152: file /tmp/hello.c, line 8.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       dprintf        keep y   0x0000000000401152 in call_me at /tmp/hello.c:8 thread 1
          stop only in thread 1
          printf "%s", "hello"
  (gdb)

This feature of dprintf was not documented, was not tested, and is
slightly different in syntax to how we create thread specific
breakpoints and/or watchpoints -- the thread condition appears after
the first ','.

I believe that this worked at all was simply by luck.  We happen to
pass the parse_extra flag as true from dprintf_command to
create_breakpoint.

So in this commit I made the choice to change this.  We now pass
parse_extra as false from dprintf_command to create_breakpoint.  With
this done it is assumed that the only thing in the extra_string is the
dprintf format and arguments.

Beyond this change I've updated the comment on create_breakpoint in
breakpoint.h, and I've then added some asserts into
create_breakpoint as well as moving around some of the error
handling.

 - We now assert on the incoming argument values,

 - I've moved an error check to sit after the call to
   find_condition_and_thread_for_sals, this ensures the extra_string
   was parsed correctly,

In dprintf_command:

 - We now throw an error if there is no format string after the
   dprintf location.  This error was already being thrown, but was
   being caught later in the process.  With this change we catch the
   missing string earlier,

 - And, as mentioned earlier, we pass parse_extra as false when
   calling create_breakpoint,

In create_tracepoint_from_upload:

 - We now throw an error if the parsed location doesn't completely
   consume the addr_str variable.  This error has now effectively
   moved out of create_breakpoint.

1 files changed, 28 insertions, 13 deletions

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0da666d..044d424 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9232,6 +9232,16 @@ create_breakpoint (struct gdbarch *gdbarch,
   if (extra_string != NULL && *extra_string == '\0')
     extra_string = NULL;
 
+  /* A bp_dprintf must always have an accompanying EXTRA_STRING containing
+     the dprintf format and arguments -- PARSE_EXTRA should always be false
+     in this case.
+
+     For all other breakpoint types, EXTRA_STRING should be nullptr unless
+     PARSE_EXTRA is true.  */
+  gdb_assert ((type_wanted == bp_dprintf)
+	      ? (extra_string != nullptr && !parse_extra)
+	      : (extra_string == nullptr || parse_extra));
+
   try
     {
       ops->create_sals_from_location_spec (locspec, &canonical);
@@ -9295,6 +9305,8 @@ create_breakpoint (struct gdbarch *gdbarch,
 
       if (parse_extra)
 	{
+	  gdb_assert (type_wanted != bp_dprintf);
+
 	  gdb::unique_xmalloc_ptr<char> rest;
 	  gdb::unique_xmalloc_ptr<char> cond;
 
@@ -9303,15 +9315,15 @@ create_breakpoint (struct gdbarch *gdbarch,
 	  find_condition_and_thread_for_sals (lsal.sals, extra_string,
 					      &cond, &thread, &inferior,
 					      &task, &rest);
+
+	  if (rest.get () != nullptr && *(rest.get ()) != '\0')
+	    error (_("Garbage '%s' at end of command"), rest.get ());
+
 	  cond_string_copy = std::move (cond);
 	  extra_string_copy = std::move (rest);
 	}
       else
 	{
-	  if (type_wanted != bp_dprintf
-	      && extra_string != NULL && *extra_string != '\0')
-		error (_("Garbage '%s' at end of location"), extra_string);
-
 	  /* Check the validity of the condition.  We should error out
 	     if the condition is invalid at all of the locations and
 	     if it is not forced.  In the PARSE_EXTRA case above, this
@@ -9522,21 +9534,18 @@ dprintf_command (const char *arg, int from_tty)
 
   /* If non-NULL, ARG should have been advanced past the location;
      the next character must be ','.  */
-  if (arg != NULL)
+  if (arg == nullptr || arg[0] != ',' || arg[1] == '\0')
+    error (_("Format string required"));
+  else
     {
-      if (arg[0] != ',' || arg[1] == '\0')
-	error (_("Format string required"));
-      else
-	{
-	  /* Skip the comma.  */
-	  ++arg;
-	}
+      /* Skip the comma.  */
+      ++arg;
     }
 
   create_breakpoint (get_current_arch (),
 		     locspec.get (),
 		     NULL, -1, -1,
-		     arg, false, 1 /* parse arg */,
+		     arg, false, 0 /* parse arg */,
 		     0, bp_dprintf,
 		     0 /* Ignore count */,
 		     pending_break_support,
@@ -14142,6 +14151,12 @@ create_tracepoint_from_upload (struct uploaded_tp *utp)
 
   location_spec_up locspec = string_to_location_spec (&addr_str,
 						      current_language);
+
+
+  gdb_assert (addr_str != nullptr);
+  if (*addr_str != '\0')
+    error (_("Garbage '%s' at end of location"), addr_str);
+
   if (!create_breakpoint (get_current_arch (),
 			  locspec.get (),
 			  utp->cond_string.get (), -1, -1, addr_str,