Re: [patch] Fix multiple-symbols=ask regression [Re: [patchv2 7/11] Mechanical symtab->filename -> symtab_to_filename]

[Joel Brobecker <brobecker at adacore dot com>]

Hi Jan,

> > I have a change of behavior which I think might be unintended.
> 
> I agree, thanks for catching it.
> 

My pleasure :).

> 	set filename-display relative
> 	(gdb) PASS: gdb.linespec/break-ask.exp: set filename-display relative
> 	break twodup
> 	Breakpoint 1 at 0x40062f: twodup. (2 locations)
> 	(gdb) FAIL: gdb.linespec/break-ask.exp: break twodup relative
[causes missing multiple-choice menu]

Argh, yes, definitely not intended.

> I believe "set filename-display" should not affect GDB behavior this way; with
> the patch below it displays:
> 
> 	break twodup
> 	[0] cancel
> 	[1] all
> 	[2] thefile.cc:twodup()
> 	[3] thefile.cc:twodup()
> 	> PASS: gdb.linespec/break-ask.exp: break twodup relative

And if I understand your patch correctly, it would also honor
"set filename-display" right?

> Therefore linespec_sals->canonical is always stored in absolute form.

It does make sense to me that something named canonical would be
as precise as possible, and thus use the filename's fullpath.

> gdb/
> 2013-03-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* linespec.c (struct linespec_canonical_name): New.
> 	(struct linespec_state): Change canonical_names type to it.
> 	(add_sal_to_sals): Change variable canonical_name to canonical.  Change
> 	xrealloc element size.  Initialize the different CANONICAL fields.
> 	(canonical_to_fullform): New.
> 	(filter_results): Use it.  Add variables canonical, fullform and
> 	cleanup.
> 	(struct decode_line_2_item, decode_line_2_compare_items): New.
> 	(decode_line_2): Remove variables iter and item_names, add variables
> 	items and items_count.  Modify the code for these new variables.
> 
> gdb/testsuite/
> 2013-03-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.linespec/base/one/thefile.cc (twodup): New.
> 	(m): Call it.
> 	* gdb.linespec/base/two/thefile.cc (dupname): New.
> 	(n): Call it.
> 	* gdb.linespec/break-ask.exp: New file.
> 	* gdb.linespec/lspec.cc (body_elsewhere): New comment marker.

Quickly tested on x86_64-linux with our testsuite, and no regression
either.

I only have some minor comments based on the time it took me to try
to understand your patch. Also, this is really a nitpick but the length
of the lines in your comments is a little too long, and I don't see
a real reason to exceed our soft limit in those cases (70 characters).

Other than that, it looks good to me.

> +/* linespec represented as a symtab-related string.  */

/* A canonical linespec [...].  */

?

> +struct linespec_canonical_name
> +{
> +  /* Remaining text part of the linespec string.  */
> +  char *suffix;

I had to read the patch in order to figure out what this was about.
It seems to me that, if you were to move the description of the
linespec string that you have below for the "symtab" field to
the global type description, and describe how the pieces fit
together to build the canonical linespec string, then you'd only
a need minimal of the fields of this struct?

> +
> +  /* If NULL then SUFFIX is the whole linespec string.
> +     Otherwise "SYMTAB:SUFFIX" is the linespec string.  SYMTAB can be converted
> +     for example by symtab_to_fullname or symtab_to_filename_for_display.  */
> +  struct symtab *symtab;

> +/* Convert CANONICAL to a string using symtab_to_fullname for SYMTAB.
                           ^^^^^^^^

its (associated?) string representation ?

> +   Caller has to xfree the result.  */

The caller must xfree the result?

> +/* Represent struct linespec_canonical_name with constant strings.  */
> +
> +struct decode_line_2_item

It took me quite a while to figure out what you meant by the above.
How about:

/* A structure that contains two string representations of a struct
   linespec_canonical_name:
     - one where the the symtab's fullname is used;
     - one where the filename followed the "set filename-display"
       setting.  */


> +{
> +  /* Provide a string form using symtab_to_fullname.  One has to call xfree
> +     for it.  It is set to NULL if it was already chosen by a user.  */
> +  char *fullform;

     /* The form using symtab_to_fullname,  [It is set to NULL if
       it was already chosen by a user.]

       It must be xfree'ed after use.  */

I don't understand the comment about "It is set to NULL if it was
already chosen by a user.", and looking at the code where it is set
to NULL, I don't quite see what you are trying to say or do with it.
What does "chosen by the user" mean?  I wish I had a little more time
to try to figure this out. Does it look like what you really want is
an extra flag (int selected = 0, set to 1 when chosen)?

> +  /* Provide a string form using symtab_to_filename_for_display.  One has to
> +     call xfree for it.  */
> +  char *displayform;

     /* The form using symtab_to_filename_for_display.
        It must be xfree'ed after use.  */

(I am not fond of "One has to".

> +/* Helper for qsort to sort decode_line_2_item entries by DISPLAYFORM and
> +   secondarily by FULLFORM.  */
> +
> +static int
> +decode_line_2_compare_items (const void *ap, const void *bp)
> +{
> +  const struct decode_line_2_item *a = ap;
> +  const struct decode_line_2_item *b = bp;
> +  int retval;
> +
> +  retval = strcmp (a->displayform, b->displayform);
> +  if (retval != 0)
> +    return retval;
> +
> +  return strcmp (a->fullform, b->fullform);

If fullform can be NULL, are you sure this is OK to do?

> +}
> +
>  /* Handle multiple results in RESULT depending on SELECT_MODE.  This
>     will either return normally, throw an exception on multiple
>     results, or present a menu to the user.  On return, the SALS vector
> @@ -1257,58 +1329,78 @@ decode_line_2 (struct linespec_state *self,
>  	       struct symtabs_and_lines *result,
>  	       const char *select_mode)
>  {
> -  const char *iter;
>    char *args, *prompt;
>    int i;
>    struct cleanup *old_chain;
> -  VEC (const_char_ptr) *item_names = NULL, *filters = NULL;
> +  VEC (const_char_ptr) *filters = NULL;
>    struct get_number_or_range_state state;
> +  struct decode_line_2_item *items;
> +  int items_count;
>  
>    gdb_assert (select_mode != multiple_symbols_all);
>    gdb_assert (self->canonical != NULL);
> +  gdb_assert (result->nelts >= 1);
> +
> +  old_chain = make_cleanup (VEC_cleanup (const_char_ptr), &filters);
>  
> -  old_chain = make_cleanup (VEC_cleanup (const_char_ptr), &item_names);
> -  make_cleanup (VEC_cleanup (const_char_ptr), &filters);
> -  for (i = 0; i < result->nelts; ++i)
> +  /* Prepare ITEMS array.  */
> +  items_count = result->nelts;
> +  items = xmalloc (sizeof (*items) * items_count);
> +  make_cleanup (xfree, items);
> +  for (i = 0; i < items_count; ++i)
>      {
> -      int j, found = 0;
> -      const char *iter;
> +      const struct linespec_canonical_name *canonical;
> +      struct decode_line_2_item *item;
>  
> -      gdb_assert (self->canonical_names[i] != NULL);
> -      for (j = 0; VEC_iterate (const_char_ptr, item_names, j, iter); ++j)
> +      canonical = &self->canonical_names[i];
> +      gdb_assert (canonical->suffix != NULL);
> +      item = &items[i];
> +
> +      item->fullform = canonical_to_fullform (canonical);
> +      make_cleanup (xfree, item->fullform);
> +
> +      if (canonical->symtab == NULL)
> +	item->displayform = canonical->suffix;
> +      else
>  	{
> -	  if (strcmp (iter, self->canonical_names[i]) == 0)
> -	    {
> -	      found = 1;
> -	      break;
> -	    }
> +	  const char *fn_for_display;
> +	  

Trailing spaces here...

> +	  fn_for_display = symtab_to_filename_for_display (canonical->symtab);
> +	  item->displayform = xstrprintf ("%s:%s", fn_for_display,
> +					  canonical->suffix);
> +	  make_cleanup (xfree, item->displayform);
>  	}
> +    }
> +
> +  /* Sort the list of method names.  */
> +  qsort (items, items_count, sizeof (*items), decode_line_2_compare_items);
>  
> -      if (!found)
> -	VEC_safe_push (const_char_ptr, item_names, self->canonical_names[i]);
> +  /* Remove entries with the same FULLFORM.  */
> +  if (items_count >= 2)
> +    {
> +      struct decode_line_2_item *dst, *src;
> +
> +      dst = items;
> +      for (src = &items[1]; src < &items[items_count]; src++)
> +	if (strcmp (src->fullform, dst->fullform) != 0)
> +	  *++dst = *src;
> +      items_count = dst + 1 - items;
>      }
>  
> -  if (select_mode == multiple_symbols_cancel
> -      && VEC_length (const_char_ptr, item_names) > 1)
> +  if (select_mode == multiple_symbols_cancel && items_count > 1)
>      error (_("canceled because the command is ambiguous\n"
>  	     "See set/show multiple-symbol."));
>    

Same here...


-- 
Joel