Re: [PATCH 4/4] Skip .cold functions in search_minsyms_for_name

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2021-01-31 10:13:54 +0100]:

> When a function with the same name is also available,
> that is preferred.
> 
> The .cold function is not the external interface.
> 
> Fixes 77f2120b200 ("Don't drop static function bp locations w/o
> debug info")

I assume that this last sentence means that there's a bug / oversight
with this previous commit.

You should explain what this issue with this commit is and how this
commit fixes it.  For contrast, notice that the commit you reference
took ~200 lines to explain the change, while you've given us just 7.
A good commit message makes reviewing this a lot easier.

Thanks,
Andrew



> 
> gdb:
> 2020-11-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* linespec.c (search_minsyms_for_name): Filter .cold functions when
> 	a real function with the same name is found.
> 
> gdb/testsuite:
> 2020-11-30  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* gdb.cp/step-and-next-inline-1.exp: New test.
> ---
>  gdb/linespec.c                                  | 24 ++++++++++++++++++++
>  gdb/testsuite/gdb.cp/step-and-next-inline-1.exp | 30 +++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.cp/step-and-next-inline-1.exp
> 
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index a9809a5..37feaed 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -4394,6 +4394,30 @@ class symtab_collector
>  	      break;
>  	    }
>  	}
> +      else if (MSYMBOL_TYPE (item.minsym) == mst_file_text
> +	       && strlen (item.minsym->linkage_name ()) > 5)
> +	{
> +	  size_t namelen = strlen (item.minsym->linkage_name ()) - 5;
> +	  if (memcmp (item.minsym->linkage_name () + namelen, ".cold", 5) == 0)
> +	    for (const bound_minimal_symbol &item2 : minsyms)
> +	      {
> +		if (&item2 == &item)
> +		  continue;
> +
> +		if (MSYMBOL_TYPE (item2.minsym) != mst_file_text
> +		    && MSYMBOL_TYPE (item2.minsym) != mst_text)
> +		  continue;
> +
> +		if (strlen (item2.minsym->linkage_name ()) == namelen
> +		    && memcmp (item.minsym->linkage_name (),
> +			       item2.minsym->linkage_name (), namelen) != 0)
> +		  continue;
> +
> +		/* found a real function with the same name as cold part.  */
> +		skip = true;
> +		break;
> +	      }
> +	}
>  
>        if (!skip)
>  	info->result.minimal_symbols->push_back (item);
> diff --git a/gdb/testsuite/gdb.cp/step-and-next-inline-1.exp b/gdb/testsuite/gdb.cp/step-and-next-inline-1.exp
> new file mode 100644
> index 0000000..02200a4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/step-and-next-inline-1.exp
> @@ -0,0 +1,30 @@
> +# Copyright 2021 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/>.
> +
> +standard_testfile step-and-next-inline.cc
> +
> +if [get_compiler_info "c++"] {
> +    unsupported "couldn't find a valid c++ compiler"
> +    return -1
> +}
> +
> +set options {c++ debug nowarnings optimize=-O2}
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile $options] } {
> +    return -1
> +}
> +
> +# Test that b get_alias_set sets only one breakpoint,
> +# thus get_alias_set.cold is filtered away.
> +gdb_test "b get_alias_set" ".*Breakpoint .*$srcfile, line .*" "test1"
> -- 
> 1.9.1