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:

> 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