Re: [RFA 8/9] Explicit locations v2 - MI for explicit locations

[Keith Seitz <keiths at redhat dot com>]

On 05/13/2014 10:35 AM, Vladimir Prus wrote:
> On 05/08/2014 10:04 PM, Keith Seitz wrote:
> > +    {"s", EXPLICIT_SOURCE_OPT, 1},
> > +    {"m", EXPLICIT_FUNC_OPT, 1},
> > +    {"l", EXPLICIT_LABEL_OPT, 1},
> > +    {"o", EXPLICIT_LINE_OPT, 1},
>
> Maybe it's me, but 'm' and 'o' do not appear like logical abbreviation.

Those letters were refinements based on previous feedback. m = method, l 
= label, o = offset (which is what "line" really is; it is both relative 
and absolute values; it's also what it's called internally).

I was just following the existing conventions:

 static const struct mi_opt opts[] =
  {
    {"h", HARDWARE_OPT, 0},
    {"t", TEMP_OPT, 0},
    {"c", CONDITION_OPT, 1},
    {"i", IGNORE_COUNT_OPT, 1},
    {"p", THREAD_OPT, 1},
    {"f", PENDING_OPT, 0},
    {"d", DISABLE_OPT, 0},
    {"a", TRACEPOINT_OPT, 0},
    { 0, 0, 0 }

I didn't find m and o any less logical than "p", "f", or "a". In the 
end, though, since none of this matters to a machine, I simply picked 
something semi-logical. At least to me. :-)

> Maybe, it's time to just switch to fully-spelled option names?

I can do that. Do you want me to submit a patch to add long aliases for 
the existing options?

The proposed list of options would then be:
Current: h, t, c, i, p, f, d, a
New aliases: hardware, temporary, condition, ignorecount, thread, 
pending, disable, tracepoint
New explicit locations: source, function, label, line

I've updated the patch and changed the explicit options to those listed 
immediately above.

Thank you for the feedback!
Keith

Changes since last version:
8-ui-mi:
  - use --source, --function, --label, --line instead of
    "-s", "-m", "-l", "-o"
  - update tests

ChangeLog
2014-05-29  Keith Seitz  <keiths@redhat.com>

	* mi/mi-cmd-break.c (mi_cmd_break_insert_1): Add support for
	explicit locations, options "--source", "--function",
	"--label", and "--line".

testsuite/ChangeLog
2014-05-29  Keith Seitz  <keiths@redhat.com>

	* gdb.mi/mi-break.exp (test_explicit_breakpoints): New proc.
	(at toplevel): Call test_explicit_breakpoints.
	* gdb.mi/mi-dprintf.exp: Add tests for explicit dprintf
	breakpoints.
	* lib/mi-support.exp (mi_make_breakpoint): Add support for
	breakpoint conditions, "-cond".

diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 327b2de..0d0488c 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -184,6 +184,8 @@ mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)
   enum bptype type_wanted;
   struct event_location *location;
   struct breakpoint_ops *ops;
+  int is_explicit = 0;
+  struct explicit_location explicit;
   char *extra_string = NULL;
 
   enum opt
@@ -191,6 +193,8 @@ mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)
       HARDWARE_OPT, TEMP_OPT, CONDITION_OPT,
       IGNORE_COUNT_OPT, THREAD_OPT, PENDING_OPT, DISABLE_OPT,
       TRACEPOINT_OPT,
+      EXPLICIT_SOURCE_OPT, EXPLICIT_FUNC_OPT,
+      EXPLICIT_LABEL_OPT, EXPLICIT_LINE_OPT
     };
   static const struct mi_opt opts[] =
   {
@@ -202,6 +206,10 @@ mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)
     {"f", PENDING_OPT, 0},
     {"d", DISABLE_OPT, 0},
     {"a", TRACEPOINT_OPT, 0},
+    {"-source" , EXPLICIT_SOURCE_OPT, 1},
+    {"-function", EXPLICIT_FUNC_OPT, 1},
+    {"-label", EXPLICIT_LABEL_OPT, 1},
+    {"-line", EXPLICIT_LINE_OPT, 1},
     { 0, 0, 0 }
   };
 
@@ -210,6 +218,8 @@ mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)
   int oind = 0;
   char *oarg;
 
+  initialize_explicit_location (&explicit);
+
   while (1)
     {
       int opt = mi_getopt ("-break-insert", argc, argv,
@@ -242,16 +252,31 @@ mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)
 	case TRACEPOINT_OPT:
 	  tracepoint = 1;
 	  break;
+	case EXPLICIT_SOURCE_OPT:
+	  is_explicit = 1;
+	  explicit.source_filename = oarg;
+	  break;
+	case EXPLICIT_FUNC_OPT:
+	  is_explicit = 1;
+	  explicit.function_name = oarg;
+	  break;
+	case EXPLICIT_LABEL_OPT:
+	  is_explicit = 1;
+	  explicit.label_name = oarg;
+	  break;
+	case EXPLICIT_LINE_OPT:
+	  is_explicit = 1;
+	  explicit.line_offset = linespec_parse_line_offset (oarg);
+	  break;
 	}
     }
 
-  if (oind >= argc)
+  if (oind >= argc && !is_explicit)
     error (_("-%s-insert: Missing <location>"),
 	   dprintf ? "dprintf" : "break");
-  address = argv[oind];
   if (dprintf)
     {
-      int format_num = oind + 1;
+      int format_num = is_explicit ? oind : oind + 1;
 
       if (hardware || tracepoint)
 	error (_("-dprintf-insert: does not support -h or -a"));
@@ -260,11 +285,21 @@ mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)
 
       extra_string = mi_argv_to_format (argv + format_num, argc - format_num);
       make_cleanup (xfree, extra_string);
+      address = argv[oind];
     }
   else
     {
-      if (oind < argc - 1)
-	error (_("-break-insert: Garbage following <location>"));
+      if (is_explicit)
+	{
+	  if (oind < argc)
+	    error (_("-break-insert: Garbage following explicit location"));
+	}
+      else
+	{
+	  if (oind < argc - 1)
+	    error (_("-break-insert: Garbage following <location>"));
+	  address = argv[oind];
+	}
     }
 
   /* Now we have what we need, let's insert the breakpoint!  */
@@ -293,7 +328,10 @@ mi_cmd_break_insert_1 (int dprintf, char *command, char **argv, int argc)
       ops = &bkpt_breakpoint_ops;
     }
 
-  location = string_to_event_location (&address, current_language);
+  if (is_explicit)
+    location = new_explicit_location (&explicit);
+  else
+    location = string_to_event_location (&address, current_language);
   make_cleanup_delete_event_location (location);
 
   create_breakpoint (get_current_arch (), location, condition, thread,
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index d9ab757..720007b 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -305,6 +305,86 @@ proc test_breakpoint_commands {} {
     mi_expect_stop "exited-normally" "" "" "" "" "" "test hitting breakpoint with commands"
 }
 
+# Test explicit breakpoints.  These tests only test the MI portion of the
+# code.  In-depth testing of explicit breakpoints is accomplished in
+# gdb.linespec tests.
+
+proc test_explicit_breakpoints {} {
+    global srcfile
+    global line_callee3_head line_callee4_head
+    global line_callee2_body line_main_body
+
+    mi_delete_breakpoints
+
+    # First check mixed explicit/parsed linespecs.
+    mi_gdb_test "-break-insert --function main $srcfile:$line_callee3_head" \
+	".*Garbage following explicit linespec"
+
+    # Insert some breakpoints and list them
+    # Also, disable some so they do not interfere with other tests
+    # Tests:
+    # -break-insert -t --function main
+    # -break-insert -t --source basics.c --function callee2
+    # -break-insert -t --source basics.c --line $line_callee3_head
+    # -break-insert -t --source srcfile --line $line_callee4_head
+    # -break-list
+
+    set bps {}
+    lappend bps [mi_create_breakpoint "-t --function main" \
+		     "insert temp explicit breakpoint in main" \
+		     -func main -file ".*$srcfile" -line $line_main_body]
+
+    lappend bps \
+	[mi_create_breakpoint "-t --source $srcfile --function callee2" \
+	     "insert temp explicit breakpoint at $srcfile:callee2" \
+	     -func callee2 -file ".*$srcfile" -line $line_callee2_body]
+
+    lappend bps \
+	[mi_create_breakpoint "-t --source $srcfile --line $line_callee3_head" \
+	     "insert temp explicit breakpoint at $srcfile:$line_callee3_head" \
+	     -func callee3 -file ".*$srcfile" -line $line_callee3_head]
+
+    lappend bps \
+	[mi_create_breakpoint \
+	     "-t --source \"$srcfile\" --line  $line_callee4_head" \
+	     "insert temp explicit breakpoint at \"$srcfile\":$line_callee4_head" \
+	     -func callee4 -file ".*$srcfile" -line $line_callee4_head]
+
+    mi_gdb_test "-break-list" "\\^done,[mi_make_breakpoint_table $bps]" \
+	"list of explicit breakpoints"
+
+    mi_gdb_test "-break-delete" \
+	    "\\^done" \
+	    "delete temp breakpoints"
+
+    mi_create_breakpoint "-c \"intarg == 3\" --function callee2" \
+	"insert explicit conditional breakpoint in callee2" \
+	-func callee2 ".*$srcfile" -line $line_callee2_body \
+	-cond "intarg == 3"
+
+    # mi_create_breakpoint cannot deal with displaying canonical
+    # linespecs.
+    mi_gdb_test \
+	"-break-insert -c \"foo == 3\" --source $srcfile --function main --label label" \
+	".*No symbol \"foo\" in current context.*"
+
+    mi_gdb_test \
+	"-break-insert --source foobar.c --line 3" \
+	".*No source file named foobar.c.*"
+
+    mi_gdb_test \
+	"-break-insert --source $srcfile --function foobar" \
+	".*Function \"foobar\" not defined in \"$srcfile\".*"
+
+    mi_gdb_test \
+	"-break-insert --source $srcfile --function main --label foobar" \
+	".*No label \"foobar\" defined in function \"main\".*"
+
+    mi_gdb_test \
+	"-break-insert --source $srcfile" \
+	".*Source filename requires function, label, or line offset.*"
+}
+
 test_tbreak_creation_and_listing
 test_rbreak_creation_and_listing
 
@@ -318,5 +398,7 @@ test_breakpoint_commands
 
 test_abreak_creation
 
+test_explicit_breakpoints
+
 mi_gdb_exit
 return 0
diff --git a/gdb/testsuite/gdb.mi/mi-dprintf.exp b/gdb/testsuite/gdb.mi/mi-dprintf.exp
index d60d66c..faaeb90 100644
--- a/gdb/testsuite/gdb.mi/mi-dprintf.exp
+++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp
@@ -47,6 +47,13 @@ mi_gdb_test "[incr i]-dprintf-insert 29" \
     "$i\\^error,msg=\"-dprintf-insert: Missing <format>\"" "mi insert second breakpoint without format string"
 
 mi_gdb_test "-break-insert main" ".*" "mi insert breakpoint main"
+
+mi_gdb_test "-dprintf-insert --function main \"hello\"" \
+    "\\^done,bkpt={.*}" "explicit dprintf at main"
+
+mi_gdb_test "-dprintf-insert --source $srcfile --line $dp_location1 \"hello\"" \
+    "\\^done,bkpt={.*}" "explicit breakpoint at $srcfile:$dp_location1"
+
 mi_delete_breakpoints
 
 set bps [mi_make_breakpoint -type dprintf -func foo -file ".*mi-dprintf.c" \
@@ -61,7 +68,7 @@ mi_gdb_test "[incr i]-dprintf-insert $dp_location1 \"arg=%d, g=%d\\n\" arg g" \
 		   "$i\\^done,$bps" "mi insert dprintf dp_location1"
 
 set bps {}
-lappend bps [mi_make_breakpoint -number 3 -type dprintf -func foo \
+lappend bps [mi_make_breakpoint -type dprintf -func foo \
 		 -file ".*mi-dprintf.c" -fullname ".*mi-dprintf.c"]
 lappend bps [mi_make_breakpoint -type dprintf -func foo \
 		 -file ".*mi-dprintf.c" -fullname ".*mi-dprintf.c" \
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index ad58775..66290f1 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -2353,7 +2353,7 @@ proc mi_build_kv_pairs {attr_list {joiner ,}} {
 #
 # All arguments for the breakpoint may be specified using the options
 # number, type, disp, enabled, addr, func, file, fullanme, line,
-# thread-groups, times, ignore, script, and original-location.
+# thread-groups, cond, times, ignore, script, and original-location.
 #
 # Only if -script and -ignore are given will they appear in the output.
 # Otherwise, this procedure will skip them using ".*".
@@ -2368,17 +2368,27 @@ proc mi_make_breakpoint {args} {
     parse_args {{number .*} {type .*} {disp .*} {enabled .*} {addr .*}
 	{func .*} {file .*} {fullname .*} {line .*}
 	{thread-groups \\\[.*\\\]} {times .*} {ignore 0}
-	{script ""} {original-location .*}}
+	{script ""} {original-location .*} {cond ""}}
 
     set attr_list {}
     foreach attr [list number type disp enabled addr func file \
-		      fullname line thread-groups times] {
+		      fullname line thread-groups] {
 	lappend attr_list $attr [set $attr]
     }
 
     set result "bkpt={[mi_build_kv_pairs $attr_list]"
 
     # There are always exceptions.
+
+    # If COND is not preset, do not output it.
+    if {[string length $cond] > 0} {
+	append result ","
+	append result [mi_build_kv_pairs [list "cond" $cond]]
+    }
+
+    append result ","
+    append result [mi_build_kv_pairs [list "times" ".*"]]
+
     # If SCRIPT and IGNORE are not present, do not output them.
     if {$ignore != 0} {
 	append result ","