Re: [PATCH v2] gdb: fix filename matching in skiplist_entry::do_skip_gfile_p

[Andrew Burgess <aburgess@redhat.com>]

Guinevere Larsen <guinevere@redhat.com> writes:

> GDB is incorrectly matching -gfile skip entries.  This is a regression
> introduced by the following commit:
>
>     commit 02646a4c561ec88491114b87950cbb827c7d614c
>     Author: Fangrui Song <maskray@sourceware.org>
>     Date:   Sun Dec 29 14:57:44 2024 -0800
>
>     skip -gfile: call fnmatch without FNM_FILE_NAME
>
> The author made the reasonable, but unfortunately incorrect, assumption
> that glibc's fnmatch, and consequently gdb_filename_fnmatch, will return
> the integer equivalent of a boolean (that is, 0 if the filenames do *not*
> match, non-zero if they match), but that is incorrect.  This made it so
> using `skip -gfile` would skip all functions except the ones that are
> meant to be skipped.  This commit fixes that inverted logic.
>
> This incorrect matching masked a different bug, that we were treating
> all user-provided gfiles as absolute paths, and the gdb.base/skip.exp
> test happened to not catch it because the "skip.c" file is different
> enough to exit earlier in the comparisons. This was solved by making it
> so if the gfile is not absolute, the regex "*/" is prepended, allowing
> for any number of directories before the first user-provided one, but
> ensuring that the beginning of the directory must match what the user
> provided.
>
> The limitation of this approach is that, if a user has given the regex
> "a/c*.c", and the inferior contains both "/b/a/code.c" and
> "/c/a/code.c" and is running on the folder /b, functions from both may
> be skipped when it would be reasonable for the user to expect only
> /b/a/code.c to be skipped.  This is done anyway because I believe the
> likelihood of something like this actually happening is incredibly low,
> and it was the behavior of GDB before the aforementioned commit so we'll
> just be returning to previous behavior.

For what it's worth, I'm not convinced by the original patch.  Sadly
there are no tests with the original patch, but based on the commit
message I think what they original wanted was, given a directory tree
like:

  /usr/include/first_header.h
  /usr/include/dir1/some_header.h
  /usr/include/dir2/other_header.h

They can just:

 (gdb) skip -gfile /usr/include/*.h

And skip all three headers.

The problem I have with this is that I can no longer skip just the files
in one directory, so given:

  /foo/file_a.c
  /foo/bar/file_b.c

I can no longer do:

  (gdb) skip -gfile /foo/*.c

And only skip 'file_a.c', the '*' here will always match both 'file_a.c'
and 'bar/file_b.c', which I think is unfortunate.

I wonder if a better approach would be to add something like bash's '**'
globstar feature, which matches 0 or more sub-directories.  In the
original case the user could then:

  (gdb) skip -gfile /usr/include/**/*.h

This would match all three headers from the original example.  And in
the second example:

  (gdb) skip -gfile /foo/*.c

Would still only match 'file_a.c' as it currently does.  If we wanted to
match both, then:

  (gdb) skip -gfile /foo/**/*.c

Would do the trick.

Unfortunately, fnmatch doesn't support '**' matching, so we'd have to
roll our own, but I think it would be worth it.

Given the above, my vote would be to not merge this fix, but instead
revert commit 02646a4c561ec8849111, then we could make an attempt at
adding a '**' feature.

We could merge this fix, but doing that would make the changed behaviour
that commit 02646a4c561ec8849111 tried to introduced actually work, at
which point people might start depending on it.  Right now
02646a4c561ec8849111 is broken, so revering it, even though it made it
into GDB 17, should be OK I think.

Anyway, that's just my thoughts, not sure how others will feel.

Thanks,
Andrew



>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33872
> ---
>  gdb/skip.c                      | 15 ++++++++++++++-
>  gdb/testsuite/gdb.base/skip.exp | 15 +++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/skip.c b/gdb/skip.c
> index 1255c7d2152..5770d6251a8 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -548,7 +548,20 @@ skiplist_entry::do_skip_gfile_p (const symtab_and_line &function_sal) const
>        /* Note: symtab_to_fullname caches its result, thus we don't have to.  */
>        const char *fullname = symtab_to_fullname (function_sal.symtab);
>  
> -      result = gdb_filename_fnmatch (m_file.c_str (), fullname, FNM_NOESCAPE);
> +      /* If the pattern does not correspond to an absolute path, add a glob
> +	 that accepts any leading path.  While this is not technically
> +	 correct, it is very likely that a user wished to skip /a/b/code.c and
> +	 not skip /c/b/code.c, and both files are relevant to the project.
> +	 Plus, this is how GDB has always worked, so users are likely expecting
> +	 that behavior.  */
> +      std::string glob = "*/";
> +      if (IS_ABSOLUTE_PATH (m_file.c_str ()))
> +	glob = m_file;
> +      else
> +	glob += m_file;
> +
> +      result = gdb_filename_fnmatch
> +		(glob.c_str (), fullname, FNM_NOESCAPE) == 0;
>      }
>  
>    if (debug_skip)
> diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp
> index 4541e61ec00..7cec1fc7021 100644
> --- a/gdb/testsuite/gdb.base/skip.exp
> +++ b/gdb/testsuite/gdb.base/skip.exp
> @@ -336,3 +336,18 @@ with_test_prefix "skip delete completion" {
>      test_gdb_complete_none "skip delete a1"
>      test_gdb_complete_none "skip delete 2-33"
>  }
> +
> +# PR gdb/33872 was a regression that would happen when using
> +# skip -gfile and a full path had to be matched.  The logic had
> +# been inverted and it wold skip all files but the one with the
> +# one we wanted to skip
> +with_test_prefix "PR gdb/33872" {
> +    gdb_test "skip -gfi /unused/path/*" \
> +	"File\\(s\\) /unused/path/\\* will be skipped when stepping\."
> +
> +    if {![runto_main]} {
> +	return
> +    }
> +
> +    gdb_test "step" "bar \\\(\\\) at .*"
> +}
>
> base-commit: a6d46a04ea3e165c0ce2f11cd9d68bdcb81f4e4e
> -- 
> 2.52.0