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

[PATCH] 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.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33872
---
 gdb/skip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/skip.c b/gdb/skip.c
index 1255c7d2152..ba2f22058ec 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -548,7 +548,8 @@ 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);
+      result = gdb_filename_fnmatch (m_file.c_str (), fullname, FNM_NOESCAPE)
+		== 0;
     }
 
   if (debug_skip)

base-commit: a6d46a04ea3e165c0ce2f11cd9d68bdcb81f4e4e
-- 
2.52.0

Re: [PATCH] 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.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33872
>
> ---
>  gdb/skip.c | 3 ++-

This needs a test please.  The fix itself looks reasonable enough
though.

Once there's a test, this should be merged to gdb-17-branch and master.

Thanks,
Andrew

>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/skip.c b/gdb/skip.c
> index 1255c7d2152..ba2f22058ec 100644
> --- a/gdb/skip.c
> +++ b/gdb/skip.c
> @@ -548,7 +548,8 @@ 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);
> +      result = gdb_filename_fnmatch (m_file.c_str (), fullname, FNM_NOESCAPE)
> +		== 0;
>      }
>  
>    if (debug_skip)
>
> base-commit: a6d46a04ea3e165c0ce2f11cd9d68bdcb81f4e4e
> -- 
> 2.52.0

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

[Tom Tromey]

>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:

Guinevere> -      result = gdb_filename_fnmatch (m_file.c_str (), fullname, FNM_NOESCAPE);
Guinevere> +      result = gdb_filename_fnmatch (m_file.c_str (), fullname, FNM_NOESCAPE)
Guinevere> +		== 0;

Wrapped expressions like this need the RHS parenthesized.

thanks,
Tom

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

[Guinevere Larsen]

On 2/3/26 11:27 AM, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
> Guinevere> -      result = gdb_filename_fnmatch (m_file.c_str (), fullname, FNM_NOESCAPE);
> Guinevere> +      result = gdb_filename_fnmatch (m_file.c_str (), fullname, FNM_NOESCAPE)
> Guinevere> +		== 0;
>
> Wrapped expressions like this need the RHS parenthesized.
>
> thanks,
> Tom
>
I ended up changing it to


result = gdb_filename_fnmatch
                 (m_file.c_str (), fullname, FNM_NOESCAPE) == 0;


I find that much prettier, if it is alright with our coding style

-- 
Cheers,
Guinevere Larsen
It/she

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

[Tom Tromey]

>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:

Guinevere> result = gdb_filename_fnmatch
Guinevere>                 (m_file.c_str (), fullname, FNM_NOESCAPE) == 0;

Guinevere> I find that much prettier, if it is alright with our coding style

This isn't really the style either I'm afraid.
See https://www.gnu.org/prep/standards/standards.html#Formatting


Tom

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

[Guinevere Larsen]

On 2/4/26 12:26 PM, Tom Tromey wrote:
>>>>>> "Guinevere" == Guinevere Larsen <guinevere@redhat.com> writes:
> Guinevere> result = gdb_filename_fnmatch
> Guinevere>                 (m_file.c_str (), fullname, FNM_NOESCAPE) == 0;
>
> Guinevere> I find that much prettier, if it is alright with our coding style
>
> This isn't really the style either I'm afraid.
> See https://www.gnu.org/prep/standards/standards.html#Formatting
>
>
> Tom
>
Oh sorry. Will update it to

result = gdb_filename_fnmatch (m_file.c_str (), fullname,
FNM_NOESCAPE) == 0;

I just want to avoid having a super long line and a tiny line following 
it. If this one still isn't ok, I'll go with your original suggestion...

-- 
Cheers,
Guinevere Larsen
It/she