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

[Yao Qi <qiyaoltc@gmail.com>]

Hi Keith,
Are you still working on this patch?  This problem bites me again in the 7.12
testing, and that leads me digging out this patch.  I don't think we need to
pick it up in 7.12, but it's better to pick it in master after 7.12
release, so that
we have a plenty of time to see how good (or bad) it is.

On Thu, Feb 4, 2016 at 12:31 PM, Pedro Alves <palves@redhat.com> wrote:
> 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
>

-- 
Yao (齐尧)