Re: [PATCHv3] gdb/c++: fix handling of breakpoints on @plt symbols

[Simon Marchi <simark@simark.ca>]

> diff --git a/gdb/testsuite/gdb.cp/breakpoint-shlib-func-lib.cc b/gdb/testsuite/gdb.cp/breakpoint-shlib-func-lib.cc
> new file mode 100644
> index 00000000000..7219f7c5a23
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/breakpoint-shlib-func-lib.cc
> @@ -0,0 +1,21 @@
> +/* Copyright 2022-2023 Free Software Foundation, Inc.
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +extern int foo ();

I don't think this extern declaration is needed.

> +# The breakpoint should now be showing in `foo` for real.
> +gdb_test "info breakpoints" \
> +    "\r\n$decimal\\s+\[^\r\n\]+ in foo\\(\\) at \[^\r\n\]+\r\n.*" \
> +    "check breakpoints after starting the inferior"
> +
> +# Now we can delete the breakpoints.
> +delete_breakpoints
> +
> +# And recreate the foo breakpoint, we should only get one location,
> +# the actual location.
> +gdb_test "break foo" "Breakpoint $decimal at \[^\r\n\]+" \
> +    "recreate foo breakpoint"
> +
> +# Check the breakpoint was recreated correctly.
> +gdb_test "info breakpoints" \
> +    "\r\n$decimal\\s+\[^\r\n\]+ in foo\\(\\) at \[^\r\n\]+" \
> +    "check breakpoints after recreation"

For completeness, you could maybe resume and confirm you are stopped at
foo (and not foo@plt).

Otherwise, it all LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> diff --git a/gdb/utils.c b/gdb/utils.c
> index 95adbe58e4a..76c0df36a80 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -2402,7 +2402,31 @@ strncmp_iw_with_mode (const char *string1, const char *string2,
>  	  return 0;
>  	}
>        else
> -	return (*string1 != '\0' && *string1 != '(');
> +	{
> +	  if (*string1 == '(')
> +	    {
> +	      int p_count = 0;
> +
> +	      do
> +		{
> +		  if (*string1 == '(')
> +		    ++p_count;
> +		  else if (*string1 == ')')
> +		    --p_count;
> +		  ++string1;
> +		}
> +	      while (*string1 != '\0' && p_count > 0);
> +
> +	      /* There maybe things like 'const' after the parameters,
> +		 which we do want to ignore.  However, if there's an '@'
> +		 then this likely indicates something like '@plt' which we
> +		 should not ignore.  */
> +	      return *string1 == '@';
> +	    }
> +
> +	  return *string1 == '\0' ? 0 : 1;
> +	}

Not really relevant for your patch but... just chatting.  I don't know
if it would be really inefficient, but reading this function, I dream of
some util function that would parse a demangled C++ function symbol into
some structured type (listing the namespaces, name, abi tags,
parameters, whether it is const or not, etc).  It would then be much
easier to work on that, rather than doing everything string-based.

Simon