Make breakpoint/location number parsing error output consistent
authorPedro Alves <palves@redhat.com>
Tue, 7 Nov 2017 11:00:32 +0000 (11:00 +0000)
committerPedro Alves <palves@redhat.com>
Tue, 7 Nov 2017 11:07:19 +0000 (11:07 +0000)
... and also make GDB catch a few more cases of invalid input.

This fixes the inconsistency in GDB's output (e.g., "bad" vs "Bad")
exposed by the new tests added in the previous commit.

Also, makes the "0-0" and "inverted range" cases be loud errors.

Also makes GDB reject negative breakpoint number in ranges.  We
already rejected negative number literals, but you could still subvert
that via convenience variables, like:

  (gdb) set $bp -1
  (gdb) disable $bp.1-2

The change to get_number_trailer fixes a bug exposed by the new tests.
The function did not handle parsing "-$num".  [This wasn't visible in
the gdb.multi/tids.exp (which has similar tests) because the TID range
parsing is implemented differently.]

gdb/ChangeLog:
2017-11-07  Pedro Alves  <palves@redhat.com>

* breakpoint.c (extract_bp_kind): New enum.
(extract_bp_num, extract_bp_or_bp_range): New functions, partially
factored out from ...
(extract_bp_number_and_location): ... here.
* cli/cli-utils.c (get_number_trailer): Handle '-$variable'.

gdb/testsuite/ChangeLog:
2017-11-07  Pedro Alves  <palves@redhat.com>

* gdb.base/ena-dis-br.exp (test_ena_dis_br): Adjust test.
* gdb.cp/ena-dis-br-range.exp: Adjust tests.
(disable_invalid, disable_inverted, disable_negative): New
procedures.
("bad numbers"): New set of tests.

gdb/ChangeLog
gdb/breakpoint.c
gdb/cli/cli-utils.c
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.base/ena-dis-br.exp
gdb/testsuite/gdb.cp/ena-dis-br-range.exp

index 5bac526f7607ea7e26655cae3b2d1b82e508c0e4..e1842d694b222d55aa0f09e477a9cf88f634acb5 100644 (file)
@@ -1,3 +1,11 @@
+2017-11-07  Pedro Alves  <palves@redhat.com>
+
+       * breakpoint.c (extract_bp_kind): New enum.
+       (extract_bp_num, extract_bp_or_bp_range): New functions, partially
+       factored out from ...
+       (extract_bp_number_and_location): ... here.
+       * cli/cli-utils.c (get_number_trailer): Handle '-$variable'.
+
 2017-11-07  Pedro Alves  <palves@redhat.com>
 
        * breakpoint.c (extract_bp_number_and_location): Change return
index a94a0d0ccc6ce1a4abc9f6c3c83e87640c2561ba..9106f09a74d6190596ada5138d12829f5ea23b6c 100644 (file)
@@ -14212,6 +14212,87 @@ find_location_by_number (int bp_num, int loc_num)
   error (_("Bad breakpoint location number '%d'"), loc_num);
 }
 
+/* Modes of operation for extract_bp_num.  */
+enum class extract_bp_kind
+{
+  /* Extracting a breakpoint number.  */
+  bp,
+
+  /* Extracting a location number.  */
+  loc,
+};
+
+/* Extract a breakpoint or location number (as determined by KIND)
+   from the string starting at START.  TRAILER is a character which
+   can be found after the number.  If you don't want a trailer, use
+   '\0'.  If END_OUT is not NULL, it is set to point after the parsed
+   string.  This always returns a positive integer.  */
+
+static int
+extract_bp_num (extract_bp_kind kind, const char *start,
+               int trailer, const char **end_out = NULL)
+{
+  const char *end = start;
+  int num = get_number_trailer (&end, trailer);
+  if (num < 0)
+    error (kind == extract_bp_kind::bp
+          ? _("Negative breakpoint number '%.*s'")
+          : _("Negative breakpoint location number '%.*s'"),
+          int (end - start), start);
+  if (num == 0)
+    error (kind == extract_bp_kind::bp
+          ? _("Bad breakpoint number '%.*s'")
+          : _("Bad breakpoint location number '%.*s'"),
+          int (end - start), start);
+
+  if (end_out != NULL)
+    *end_out = end;
+  return num;
+}
+
+/* Extract a breakpoint or location range (as determined by KIND) in
+   the form NUM1-NUM2 stored at &ARG[arg_offset].  Returns a std::pair
+   representing the (inclusive) range.  The returned pair's elements
+   are always positive integers.  */
+
+static std::pair<int, int>
+extract_bp_or_bp_range (extract_bp_kind kind,
+                       const std::string &arg,
+                       std::string::size_type arg_offset)
+{
+  std::pair<int, int> range;
+  const char *bp_loc = &arg[arg_offset];
+  std::string::size_type dash = arg.find ('-', arg_offset);
+  if (dash != std::string::npos)
+    {
+      /* bp_loc is a range (x-z).  */
+      if (arg.length () == dash + 1)
+       error (kind == extract_bp_kind::bp
+              ? _("Bad breakpoint number at or near: '%s'")
+              : _("Bad breakpoint location number at or near: '%s'"),
+              bp_loc);
+
+      const char *end;
+      const char *start_first = bp_loc;
+      const char *start_second = &arg[dash + 1];
+      range.first = extract_bp_num (kind, start_first, '-');
+      range.second = extract_bp_num (kind, start_second, '\0', &end);
+
+      if (range.first > range.second)
+       error (kind == extract_bp_kind::bp
+              ? _("Inverted breakpoint range at '%.*s'")
+              : _("Inverted breakpoint location range at '%.*s'"),
+              int (end - start_first), start_first);
+    }
+  else
+    {
+      /* bp_loc is a single value.  */
+      range.first = extract_bp_num (kind, bp_loc, '\0');
+      range.second = range.first;
+    }
+  return range;
+}
+
 /* Extract the breakpoint/location range specified by ARG.  Returns
    the breakpoint range in BP_NUM_RANGE, and the location range in
    BP_LOC_RANGE.
@@ -14237,69 +14318,23 @@ extract_bp_number_and_location (const std::string &arg,
       /* Handle 'x.y' and 'x.y-z' cases.  */
 
       if (arg.length () == dot + 1 || dot == 0)
-       error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
-
-      const char *ptb = arg.c_str ();
-      int bp_num = get_number_trailer (&ptb, '.');
-      if (bp_num == 0)
-       error (_("Bad breakpoint number '%s'"), arg.c_str ());
+       error (_("Bad breakpoint number at or near: '%s'"), arg.c_str ());
 
-      bp_num_range.first = bp_num;
-      bp_num_range.second = bp_num;
+      bp_num_range.first
+       = extract_bp_num (extract_bp_kind::bp, arg.c_str (), '.');
+      bp_num_range.second = bp_num_range.first;
 
-      const char *bp_loc = &arg[dot + 1];
-      std::string::size_type dash = arg.find ('-', dot + 1);
-      if (dash != std::string::npos)
-       {
-         /* bp_loc is a range (x-z).  */
-         if (arg.length () == dash + 1)
-           error (_("bad breakpoint number at or near: '%s'"), bp_loc);
-
-         const char *ptlf = bp_loc;
-         bp_loc_range.first = get_number_trailer (&ptlf, '-');
-
-         const char *ptls = &arg[dash + 1];
-         bp_loc_range.second = get_number_trailer (&ptls, '\0');
-       }
-      else
-       {
-         /* bp_loc is a single value.  */
-         const char *ptls = bp_loc;
-         bp_loc_range.first = get_number_trailer (&ptls, '\0');
-         if (bp_loc_range.first == 0)
-           error (_("bad breakpoint number at or near '%s'"), arg.c_str ());
-         bp_loc_range.second = bp_loc_range.first;
-       }
+      bp_loc_range = extract_bp_or_bp_range (extract_bp_kind::loc,
+                                            arg, dot + 1);
     }
   else
     {
       /* Handle x and x-y cases.  */
-      std::string::size_type dash = arg.find ('-');
-      if (dash != std::string::npos)
-       {
-         if (arg.length () == dash + 1 || dash == 0)
-           error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
-
-         const char *ptf = arg.c_str ();
-         bp_num_range.first = get_number_trailer (&ptf, '-');
 
-         const char *pts = &arg[dash + 1];
-         bp_num_range.second = get_number_trailer (&pts, '\0');
-       }
-      else
-       {
-         const char *ptf = arg.c_str ();
-         bp_num_range.first = get_number (&ptf);
-         if (bp_num_range.first == 0)
-           error (_("bad breakpoint number at or near '%s'"), arg.c_str ());
-         bp_num_range.second = bp_num_range.first;
-       }
+      bp_num_range = extract_bp_or_bp_range (extract_bp_kind::bp, arg, 0);
       bp_loc_range.first = 0;
       bp_loc_range.second = 0;
     }
-
-  if (bp_num_range.first == 0 || bp_num_range.second == 0)
-    error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
 }
 
 /* Enable or disable a breakpoint location BP_NUM.LOC_NUM.  ENABLE
index d5273b5f7f52535d97e11ca863741c7611d0b989..6b363780a298c67176e3657f618881b00bf37d47 100644 (file)
@@ -30,6 +30,13 @@ get_number_trailer (const char **pp, int trailer)
 {
   int retval = 0;      /* default */
   const char *p = *pp;
+  bool negative = false;
+
+  if (*p == '-')
+    {
+      ++p;
+      negative = true;
+    }
 
   if (*p == '$')
     {
@@ -70,11 +77,10 @@ get_number_trailer (const char **pp, int trailer)
     }
   else
     {
-      if (*p == '-')
-       ++p;
+      const char *p1 = p;
       while (*p >= '0' && *p <= '9')
        ++p;
-      if (p == *pp)
+      if (p == p1)
        /* There is no number here.  (e.g. "cond a == b").  */
        {
          /* Skip non-numeric token.  */
@@ -84,7 +90,7 @@ get_number_trailer (const char **pp, int trailer)
          retval = 0;
        }
       else
-       retval = atoi (*pp);
+       retval = atoi (p1);
     }
   if (!(isspace (*p) || *p == '\0' || *p == trailer))
     {
@@ -95,7 +101,7 @@ get_number_trailer (const char **pp, int trailer)
     }
   p = skip_spaces (p);
   *pp = p;
-  return retval;
+  return negative ? -retval : retval;
 }
 
 /* See documentation in cli-utils.h.  */
index 5698dac5f35fcebd9df3b998d34cd4014fc45316..2a89733aba9b7d3524028540cd395704a5cecde4 100644 (file)
@@ -1,3 +1,11 @@
+2017-11-07  Pedro Alves  <palves@redhat.com>
+
+       * gdb.base/ena-dis-br.exp (test_ena_dis_br): Adjust test.
+       * gdb.cp/ena-dis-br-range.exp: Adjust tests.
+       (disable_invalid, disable_inverted, disable_negative): New
+       procedures.
+       ("bad numbers"): New set of tests.
+
 2017-11-07  Pedro Alves  <palves@redhat.com>
 
        * gdb.cp/ena-dis-br-range.exp: Add tests.
index 2bbb734439fcff5f2c2a229a620209034870f28b..865542165d147ec0863267a700e1ad79363490e1 100644 (file)
@@ -398,7 +398,7 @@ proc test_ena_dis_br { what } {
     # Now enable(disable) '$b4.1 fooobaar'.  This should error on
     # fooobaar.
     gdb_test "$what $b4.1 fooobaar" \
-       "bad breakpoint number at or near 'fooobaar'" \
+       "Bad breakpoint number 'fooobaar'" \
        "$what \$b4.1 fooobar"
     set test1 "${what}d \$b4.1"
 
index c2a3017e62083a9aab8c2857e8d46578493958c9..8db53bdf348e7cb6a4c9d06d1cfae61805224ec7 100644 (file)
@@ -109,9 +109,9 @@ gdb_test "enable 2.3-5" "Bad breakpoint location number '5'" \
 gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
     "breakpoint info enable 2.3 to 2.5"
 
-# Check that disabling an reverse location breakpoint range does not
+# Check that disabling an inverted breakpoint location range does not
 # work.
-gdb_test_no_output "disable 2.3-2"
+gdb_test "disable 2.3-2" "Inverted breakpoint location range at '3-2'"
 
 gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
     "breakpoint info disable 2.3-2"
@@ -126,44 +126,111 @@ gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
 
 # Check that disabling an invalid breakpoint location range does not
 # cause trouble.
-gdb_test_no_output "disable 2.8-6"
+gdb_test "disable 2.8-6" "Inverted breakpoint location range at '8-6'"
 
 gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
     "breakpoint info disable 2.8-6"
 
 # Check that invalid/open ranges are handled correctly.
 with_test_prefix "open range" {
-    gdb_test "disable -" "bad breakpoint number at or near: '-'"
-    gdb_test "disable -1" "bad breakpoint number at or near: '-1'"
-    gdb_test "disable 1-" "bad breakpoint number at or near: '1-'"
-    gdb_test "disable 1.-2" "Bad breakpoint location number '-2'"
-    gdb_test "disable 1.2-" "bad breakpoint number at or near: '2-'"
-    gdb_test "disable 1.-2-3" "Bad breakpoint location number '-2'"
-    gdb_test "disable 1-2-3" "bad breakpoint number at or near: '1-2-3'"
+    gdb_test "disable -" "Bad breakpoint number at or near: '-'"
+    gdb_test "disable -1" "Negative breakpoint number '-1'"
+    gdb_test "disable 1-" "Bad breakpoint number at or near: '1-'"
+    gdb_test "disable 1.-2" "Negative breakpoint location number '-2'"
+    gdb_test "disable 1.2-" "Bad breakpoint location number at or near: '2-'"
+    gdb_test "disable 1.-2-3" "Negative breakpoint location number '-2'"
+    gdb_test "disable 1-2-3" "Bad breakpoint number '2-3'"
 }
 
 with_test_prefix "dangling period" {
-    gdb_test "disable 2." "bad breakpoint number at or near: '2.'"
-    gdb_test "disable .2" "bad breakpoint number at or near: '.2'"
-    gdb_test "disable 2.3.4" "bad breakpoint number at or near '2.3.4'"
+    gdb_test "disable 2." "Bad breakpoint number at or near: '2.'"
+    gdb_test "disable .2" "Bad breakpoint number at or near: '.2'"
+    gdb_test "disable 2.3.4" "Bad breakpoint location number '3.4'"
 }
 
 # Check that 0s are handled correctly.
 with_test_prefix "zero" {
-    gdb_test "disable 0" "bad breakpoint number at or near '0'"
-    gdb_test "disable 0.0" "Bad breakpoint number '0.0'"
-    gdb_test "disable 0.1" "Bad breakpoint number '0.1'"
-    gdb_test "disable 0.1-2" "Bad breakpoint number '0.1-2'"
-    gdb_test "disable 2.0" "bad breakpoint number at or near '2.0'"
+    gdb_test "disable 0" "Bad breakpoint number '0'"
+    gdb_test "disable 0.0" "Bad breakpoint number '0'"
+    gdb_test "disable 0.1" "Bad breakpoint number '0'"
+    gdb_test "disable 0.1-2" "Bad breakpoint number '0'"
+    gdb_test "disable 2.0" "Bad breakpoint location number '0'"
+    gdb_test "disable 2.0-0" "Bad breakpoint location number '0'"
+    gdb_test "disable 2.0-1" "Bad breakpoint location number '0'"
+    gdb_test "disable 2.1-0" "Bad breakpoint location number '0'"
+}
+
+# Test "disable BPLIST" with an invalid breakpoint/location BPLIST.
+# PREFIX and SUFFIX are concatenated to form BPLIST.  The invalid part
+# is always in SUFFIX.
+
+proc disable_invalid {prefix suffix} {
+    set bad_re [string_to_regexp $suffix]
+
+    if {$prefix == ""} {
+       gdb_test \
+           "disable $suffix" \
+           "Bad breakpoint number '${bad_re}'"
+    } else {
+       gdb_test \
+           "disable ${prefix}$suffix" \
+           "Bad breakpoint location number '${bad_re}'"
+    }
+}
 
-    # These should really fail...
-    gdb_test_no_output "disable 2.0-0"
-    gdb_test_no_output "enable 2.0-0"
+# Like disable_invalid, but expects an "inverted range" error.
 
-    gdb_test "disable 2.0-1" "Bad breakpoint location number '0'"
+proc disable_inverted {prefix suffix} {
+    set bad_re [string_to_regexp $suffix]
+
+    if {$prefix == ""} {
+       gdb_test \
+           "disable $suffix" \
+           "Inverted breakpoint range at '${bad_re}'"
+    } else {
+       gdb_test \
+           "disable ${prefix}$suffix" \
+           "Inverted breakpoint location range at '${bad_re}'"
+    }
+}
+
+# Like disable_invalid, but expects a "negative number" error.
+
+proc disable_negative {prefix suffix} {
+    set bad_re [string_to_regexp $suffix]
+
+    if {$prefix == ""} {
+       gdb_test \
+           "disable $suffix" \
+           "Negative breakpoint number '${bad_re}'"
+    } else {
+       gdb_test \
+           "disable ${prefix}$suffix" \
+           "Negative breakpoint location number '${bad_re}'"
+    }
+}
 
-    # Likewise, should fail.
-    gdb_test_no_output "disable 2.1-0"
+with_test_prefix "bad numbers" {
+    gdb_test "p \$zero = 0" " = 0"
+    gdb_test "p \$one = 1" " = 1"
+    gdb_test "p \$two = 2" " = 2"
+    gdb_test "p \$minus_one = -1" " = -1"
+    foreach prefix {"" "1." "$one."} {
+       set prefix_re [string_to_regexp $prefix]
+
+       disable_invalid $prefix "foo"
+       disable_invalid $prefix "1foo"
+       disable_invalid $prefix "foo1"
+
+       disable_inverted $prefix "2-1"
+       disable_inverted $prefix "2-\$one"
+       disable_inverted $prefix "\$two-1"
+       disable_inverted $prefix "\$two-\$one"
+
+       disable_negative $prefix "-1"
+       disable_negative $prefix "-\$one"
+       disable_negative $prefix "\$minus_one"
+    }
 }
 
 gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \