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

[Andrew Burgess <aburgess@redhat.com>]

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