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

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

[Guinevere Larsen]

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.

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

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

[Andrew Burgess]

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

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

[Guinevere Larsen]

On 2/4/26 6:47 PM, Andrew Burgess wrote:
> 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.

I decided to look for the original patch thread, to ping the relevant 
people and get their opinions, but here it is: 
https://inbox.sourceware.org/gdb-patches/20241230185639.370189-1-maskray@sourceware.org/

It seems like the patch was never fully approved, Eli just approved the 
documentation changes and the contributor must have misunderstood that 
for a full approval... Making the case for reverting 
02646a4c561ec8849111 even stronger

-- 
Cheers,
Guinevere Larsen
It/she

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

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

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

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

CC-ing Fangrui Song (original patch author).

I'm planning to revert commit:

  commit 02646a4c561ec88491114b87950cbb827c7d614c
  Date:   Sun Dec 29 14:57:44 2024 -0800
  
      skip -gfile: call fnmatch without FNM_FILE_NAME

In the gdb-17-branch and master branch on Wednesday.  As the bug the
Guinevere mentions, this patch has introduced a regression, and as I
believe I have shown, this patch has changed GDB behaviour in a
non-backward compatible way.

I'll then take a look at implementing support for '**' as mentioned
above as I believe this will solve the original issue in a backward
compatible way.

If any other global maintainer disagrees with this plan, speak out and
I'll not go ahead with this.

Thanks,
Andrew



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

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

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> Andrew Burgess <aburgess@redhat.com> writes:
>
>> 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.
>
> CC-ing Fangrui Song (original patch author).
>
> I'm planning to revert commit:
>
>   commit 02646a4c561ec88491114b87950cbb827c7d614c
>   Date:   Sun Dec 29 14:57:44 2024 -0800
>   
>       skip -gfile: call fnmatch without FNM_FILE_NAME
>
> In the gdb-17-branch and master branch on Wednesday.  As the bug the
> Guinevere mentions, this patch has introduced a regression, and as I
> believe I have shown, this patch has changed GDB behaviour in a
> non-backward compatible way.
>
> I'll then take a look at implementing support for '**' as mentioned
> above as I believe this will solve the original issue in a backward
> compatible way.
>
> If any other global maintainer disagrees with this plan, speak out and
> I'll not go ahead with this.

I've now reverted this patch in gdb-17-branch and master.

I've posted this series:

  https://inbox.sourceware.org/gdb-patches/cover.1770819471.git.aburgess@redhat.com

which implements the '**' idea.  Feedback welcome on that thread.

Thanks,
Andrew