Re: [PATCH v2.1 2/3] Make "break foo" find "A::foo", A::B::foo", etc. [C++ and wild matching]

[Pedro Alves <palves@redhat.com>]

On 11/28/2017 02:50 AM, Keith Seitz wrote:
> On 11/27/2017 04:14 PM, Pedro Alves wrote:
>> On 11/28/2017 12:01 AM, Pedro Alves wrote:
>>> On 11/27/2017 05:13 PM, Pedro Alves wrote:
>>>> In v2:
>>>>
>>>>   - Implements Keith's suggestion of making "-qualified" a flag
>>>>     option, instead of being a replacement for "-function".  This
>>>>     means that "break -q filename.cc:function" interprets
>>>>     "filename.cc:function" as a linespec with two components instead
>>>>     of a (bogus) function name.  Basically, this folds in these
>>>>     changes (with some additional cleanup here and there):
>>>>      https://sourceware.org/ml/gdb-patches/2017-11/msg00621.html
>>>>      https://sourceware.org/ml/gdb-patches/2017-11/msg00618.html
>>>
>>> Rereading this, I realized that the "-qualified" options wasn't being saved
>>> by "save breakpoints".  There were a couple problems.  First,
>>> linespec.c:canonicalize_linespec was losing the symbol_name_match_type.
>>> While addressing this, I realized that we don't really need to add
>>> a new field to ls_parser to record the desired symbol_name_match_type,
>>> since we can just use the one in PARSER_EXPLICIT.
>>> The second is that I missed updating the "-qualified" handling in
>>> explicit_to_string_internal to take into account the fact that
>>> "-qualified" now appears with traditional linespecs as well.
> 
> Hahaha. If I had a dime for every time I (almost?) forgot to check
> "save-breakpoints," I'd have a couple of bucks by now...
> 
>>> Below's what I'm folding in to the patch, to address this.  New
>>> testcase included.
>>
>> And here's the resulting patch with that folded in.
>>
> 
> Just one little comment.
> 
>> diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
>> index 833bdc0..6cb1d71 100644
>> --- a/gdb/mi/mi-cmd-break.c
>> +++ b/gdb/mi/mi-cmd-break.c
>> @@ -337,7 +337,8 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
>>      }
>>    else
>>      {
>> -      location = string_to_event_location_basic (&address, current_language);
>> +      location = string_to_event_location_basic (&address, current_language,
>> +						 symbol_name_match_type::WILD);
>>        if (*address)
>>  	error (_("Garbage '%s' at end of location"), address);
>>      }
> 
> At first, the use of ::WILD here seemed odd to me. MI is to be used by user
> interfaces like Eclipse, I wondered why Eclipse would need WILD-card access
> to symbol names.

My rationale here was to make that setting breakpoints via MI (the GUI) and
via CLI (the GDB console) find the same locations, by default.  My thinking is that
setting breakpoints by function name is something that usually frontends don't
do -- they usually set breakpoints by source filename + line number.  But
when setting a breakpoint by function name, in response to the user
typing some function name in some input line widget, it'll be better
to make CLI and the frontend agree on locations.

> 
> But then I remembered that having access to full source code "database" is not
> (or should not) be a requirement for using MI.
> 
> So that leaves me to assume that we intend a follow-on patch to address
> adding a "-qualified"-like flag for MI, too. Not requesting changes. Just
> thinking aloud.

Right, I had this unfinished patchlet here (missing tests, docs, etc...):

MI and --qualified
---

 gdb/mi/mi-cmd-break.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 6cb1d71..75449e8 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -189,7 +189,8 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
       IGNORE_COUNT_OPT, THREAD_OPT, PENDING_OPT, DISABLE_OPT,
       TRACEPOINT_OPT,
       EXPLICIT_SOURCE_OPT, EXPLICIT_FUNC_OPT,
-      EXPLICIT_LABEL_OPT, EXPLICIT_LINE_OPT
+      EXPLICIT_LABEL_OPT, EXPLICIT_LINE_OPT,
+      QUALIFIED_OPT,
     };
   static const struct mi_opt opts[] =
   {
@@ -205,6 +206,7 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
     {"-function", EXPLICIT_FUNC_OPT, 1},
     {"-label", EXPLICIT_LABEL_OPT, 1},
     {"-line", EXPLICIT_LINE_OPT, 1},
+    {"-qualified", QUALIFIED_OPT, 0},
     { 0, 0, 0 }
   };
 
@@ -263,6 +265,9 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
 	  is_explicit = 1;
 	  explicit_loc.line_offset = linespec_parse_line_offset (oarg);
 	  break;
+	case QUALIFIED_OPT:
+	  explicit_loc.func_name_match_type = symbol_name_match_type::FULL;
+	  break;
 	}
     }
 
@@ -337,8 +342,9 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char **argv, int argc)
     }
   else
     {
-      location = string_to_event_location_basic (&address, current_language,
-						 symbol_name_match_type::WILD);
+      location
+	= string_to_event_location_basic (&address, current_language,
+					  explicit_loc.func_name_match_type);
       if (*address)
 	error (_("Garbage '%s' at end of location"), address);
     }


which seems to work fine.  We get:

-break-insert --qualified main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000400501",func="main",file="main.c",fullname="/home/pedro/gdb/tests/main.c",line="5",thread-groups=["i1"],times="0",original-location="-qualified main"}
(gdb) 
-break-insert --qualified --function main
^done,bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000400501",func="main",file="main.c",fullname="/home/pedro/gdb/tests/main.c",line="5",thread-groups=["i1"],times="0",original-location="-qualified -function main"}
(gdb) 

Really not sure it's worth the bother to try to treat single-dash
"-qualified" as part of the linespec, as in:

  "-break-insert LINESPEC" =>
  "-break-insert -qualified function"

(I think we could get that working by using mi_getopt_allow_unknown.)

I assume a frontend would want to wire this up to a toggle checkbox:

 [line widget for function name]  [X] qualified 

> 
> [I would guess the same for the similar python and guile changes.]

*nod*

Here I'm assuming we'll get most of it for free too given Phil's
Python explicit locations work, though I haven't looked much at
it, to be honest.

(My thinking was to focus/settle on the core/CLI bits ignoring
those, and address those after, maybe even after the branch
is cut.)

> 
> Am I in left field?
> 
> That said, LGTM.

Alright, let me know what think of the above.

Thanks,
Pedro Alves