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

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

>> 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:

I removed the 'extern' and added an extra check that we hit the expected
breakpoint, then pushed this patch.

Thanks,
Andrew

>
> 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