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

[Guinevere Larsen <guinevere@redhat.com>]

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