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

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

[Bernd Edlinger]

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

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

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

[Andrew Burgess]

* 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

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

[Bernd Edlinger]

On 1/31/21 6:03 PM, Andrew Burgess wrote:
> * 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 for your review.

I'll add this to the commit message, hope it clarifies the patch:

The motivation for this patch, is that I became aware that the "real"
get_alias_set function (in gcc's cc1) had a two-PC breakpoint
location with gdb-9 but a three-PC breakpoint location with gdb-10.
The third breakpoint location was not an entry point but somewhere in the
error handling, which is named "get_alias_set.cold".

Bisection pointed to the above commit.  Prior to this commit the
static "get_alias_set.cold" was ignored, since there is also global
get_alias_set function.  Basically the previous behavior was ignore
any static function, when there is a global function with the same name.
The current behavior is to consider a static function and a global
function with the same name as different entities.

But a function named with ".cold" is not a real function at all.
It is just a wrapper for all the cold code paths in that function.
Therefore this patch checks if a function with the same name is
found and skip the ".cold" overload in that case.


Bernd.