Re: [RFC] breakpoints/19474 [was Re: RFC: branching for GDB 7.11 soon? (possibly Wed)]

[Pedro Alves <palves@redhat.com>]

On 02/04/2016 12:45 AM, Keith Seitz wrote:

> At Pedro's urging, I found another, simpler solution. The big problem
> with just about all solutions I was attempting to implement is
> [p]symtab_to_fululname/find_and_open_source/openp. These functions are
> just completely unaware of what the caller is attempting to do.

Thanks!


> I'm sure there are corner cases and a whole bunch of other problems with
> this approach, but at least it is isolated to one place (for better or
> worse).

Things to watch out for, out of the blue:

- relative SYMTAB_DIRNAMEs -- is there such a thing?

- symlinks.  Say, what happens if foo/f.c is a symlink to f.c (perhaps
  each is compiled differently, with -DWHATNOT, for example).

> +static VEC (symtab_ptr) *
> +filter_default_symtabs (const char *fullname, VEC (symtab_ptr) *symtabs)
> +{
> +  int ix;
> +  struct symtab *symtab;
> +  VEC (symtab_ptr) *filtered_symtabs = NULL;
> +  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);

I think filtered_symtabs should be guarded with a cleanup as well.

> +
> +  /* Iterate through the symtabs, searching for matches to FULLNAME.  */
> +  for (ix = 0; VEC_iterate (symtab_ptr, symtabs, ix, symtab); ++ix)
> +    {
> +      const char *basename = lbasename (fullname);
> +      char *symtab_with_dir;
> +
> +      if (SYMTAB_DIRNAME (symtab) == NULL)
> +	continue;
> +
> +      symtab_with_dir = concat (SYMTAB_DIRNAME (symtab), SLASH_STRING,
> +				basename, NULL);
> +      make_cleanup (xfree, symtab_with_dir);
> +      if (streq (fullname, symtab_with_dir))
> +	VEC_safe_push (symtab_ptr, filtered_symtabs, symtab);
> +      else
> +	{
> +	  /* Now try any path substitution rules.  */
> +	  symtab_with_dir = rewrite_source_path (symtab_with_dir);
> +	  if (symtab_with_dir != NULL)
> +	    {
> +	      make_cleanup (xfree, symtab_with_dir);
> +	      if (streq (fullname, symtab_with_dir))
> +		VEC_safe_push (symtab_ptr, filtered_symtabs, symtab);
> +	    }
> +	}

This creates two cleanups per iteration here.  Only two for the whole
loop would suffice, if you used free_current_contents instead of xfree.

> +    }
> +
> +  /* If we found no matches, use whatever symtabs were originally
> +     "collected."  */
> +  if (filtered_symtabs == NULL)

Pedantically, checking VEC_empty instead would be better, as an initial

  VEC_reserve (symtab_ptr, filtered_symtabs, VEC_size (symtab_ptr, symtab));

would make a NULL check wrong.  That reserve might make sense if most of
the time we'll match all symtabs.

> +    {
> +      /* Found no exact matches -- use the original list.  */
> +      filtered_symtabs = symtabs;
> +    }
> +  else
> +    VEC_free (symtab_ptr, symtabs);
> +
> +  do_cleanups (back_to);
> +  return filtered_symtabs;
> +}

Thanks,
Pedro Alves