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

  On 02/01/2016 11:51 AM, Keith Seitz wrote:

> I started looking at this last week, and I have an analysis of the
> problems. It all basically boils down to: we cannot (reliably) win in
> this situation.

Well, it turns out I'm wrong. Or at least as not-wrong as the test suite
demonstrates.

> Aside: It seems to me that instead of doing basename searches and using
> CWD, this whole thing should really do/support directory mappings, e.g.,
> the user can say, "whenever you see /home/keiths/tmp/foo, look instead
> in /home/keiths/tmp/bar." But I don't think that really addresses the
> problem at hand.

And the astute reader knows that we already have this! "set
substitute-path" Does the trick.

> The only solution I've come up with is changing the whole API so that in
> this specific case (searching for a known symtab), we reject searching
> the source path. As you can imagine, that could open up an entirely new
> "can of worms." [And that change would be quite intrusive.]

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.

So first I want to point out that this "bug" has existed for quite some
time. I've only test back to 7.7 (earlier releases do not build on my
machine without non-trivial intervention), but all suffer from this problem.

Having said that, the scheme I've come up with restricts this "hack" to
create_sals_line_offset, where the problem is observed.

It is also here where we know we are attempting to fetch a list of
symtabs for the default source file, meaning all symtabs with the "same"
file name as default symtab.

So the simple (but apparently working?) algorithm simply compares the
default symtab's fullname vs a reconstruction of the SYMTAB_DIRNAME and
basename of the "collected" symtab. [It also considers substitute-path.]

I would consider this fairly risky for inclusion into a release without
sufficient testing in HEAD, but when it comes to releases, I tend to be
quite conservative.

I've appended the patch below.  This patch causes no regressions in the
test suite, and it fixes 19474.

Before:

$ ./gdb ~/tmp/f
Reading symbols from /home/keiths/tmp/f...done.
(gdb) cd ~/tmp
Working directory /home/keiths/tmp.
(gdb) start
Temporary breakpoint 1 at 0x4006e6: file f.c, line 6.
Starting program: /home/keiths/tmp/f

Temporary breakpoint 1, main () at f.c:6
6	  return foo () + bar_in_main ();
(gdb) b 12
Breakpoint 2 at 0x4006ff: /home/keiths/tmp/f.c:12. (2 locations)


After:
$ ./gdb ~/tmp/f
Reading symbols from /home/keiths/tmp/f...done.
(gdb) cd ~/tmp
Working directory /home/keiths/tmp.
(gdb) start
Temporary breakpoint 1 at 0x4006e6: file f.c, line 6.
Starting program: /home/keiths/tmp/f

Temporary breakpoint 1, main () at f.c:6
6	  return foo () + bar_in_main ();
(gdb) b 12
Breakpoint 2 at 0x4006ff: file f.c, line 12.

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).

Anyone have a better idea?

Keith

 static struct symtabs_and_lines
@@ -1851,6 +1911,7 @@ create_sals_line_offset (struct linespec_state *self,
       VEC_free (symtab_ptr, ls->file_symtabs);
       ls->file_symtabs = collect_symtabs_from_filename (fullname,
 							self->search_pspace);
+      ls->file_symtabs = filter_default_symtabs (fullname,
ls->file_symtabs);
       use_default = 1;
     }
  

On 02/03/2016 04:45 PM, Keith Seitz wrote:
> Anyone have a better idea?

Well, same idea, but slightly faster...

> +  /* 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);

I think that the base name of the default symtab will be the same as the
base name of any collected symtabs, so we need only really compare
directory names here.

Any other ideas?

Keith
  

[I am back from vacation...]

Hi Keith,
Thanks for looking at this bug.

On 04/02/16 00:45, Keith Seitz wrote:
> It is also here where we know we are attempting to fetch a list of
> symtabs for the default source file, meaning all symtabs with the "same"
> file name as default symtab.
>
> So the simple (but apparently working?) algorithm simply compares the
> default symtab's fullname vs a reconstruction of the SYMTAB_DIRNAME and
> basename of the "collected" symtab. [It also considers substitute-path.]

The algorithm/rationale sounds good to me.  I had a similar fix in my
side, but it causes regressions in the following test cases, in which
DW_AT_comp_dir is empty or incorrect.

  gdb.arch/amd64-entry-value-param.exp
  gdb.cp/namelessclass.exp
  gdb.dwarf2/dw2-common-block.exp
  gdb.dwarf2/dw2-single-line-discriminators.exp

but your patch looks better.

>
> I would consider this fairly risky for inclusion into a release without
> sufficient testing in HEAD, but when it comes to releases, I tend to be
> quite conservative.
>

Yes, I agree.  The problem itself can't be fixed properly without a big
structural change.  We don't have to ship this patch to 7.11, IMO.  It
is OK for mainline, however.

> I've appended the patch below.  This patch causes no regressions in the
> test suite, and it fixes 19474.
>

>
> 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).
>
> Anyone have a better idea?

I don't have any.  I agree that the directory name comparison in you
next mail is better.
  

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
  

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
>