[PATCH] Regression setting breakpoint

[PATCH] Regression setting breakpoint

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 2009 bytes --]

There is a regression when setting a breakpoint which was introduced
in gdb-7.3 by a fix in this patch:
https://sourceware.org/ml/gdb-patches/2010-03/msg00220.html

The regression shows up in C when the linkage name is different
from the internal symbol name.  For example,

    void goo (int b) asm("_goo");
    void goo (int b) {}

will create a function with the linkage name of "_goo" and the DWARF
will describe the source name of "goo".  There is both a DW_AT_name and
DW_AT_linkage_name in the DIE for this function.  Some compilers
(e.g., ICC) have options allowing linkage names to have characters
prepended to the source name so that the linkage name is different
from the source name and can not be computed from it.

Prior to this patch, a breakpoint at "_goo" would find the linkage
symbol, and one at "goo" would find the source symbol.  After the
patch, the source name is replace with the linkage name.

The attached patch fixes this by making die_needs_namespace()
return 0 for any C language program.  There's also a test case.

I'm not sure that this is the right fix for the problem, although
I'm pretty sure that it should not break anything.  There is code
in dwarf2_compute_name() which only calls die_needs_namespace()
for C++, Java, or Fortran programs, which could be moved into
die_needs_namespace().  Possibly the fix should be in dwarf2_physname()
where the call to die_needs_namespace() is done before the check for
DW_AT_linkage_name is done.

Comment:  Much of the code in dwarf2_compute_name(), dwarf2_physname(),
and related functions seems complex, convoluted, and confusing (the
big Three C's).  The source name for a symbol is not the same as
the fully qualified name and it's not the same as the linkage name.
That a language supports FQNs with namespaces does not seem to me
to say that this should have anything to do with what the linkage (aka
physical) name for the symbol is.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

[-- Attachment #2: 0001-Fix-regression-symbol-name-different-from-linkage-na.patch --]
[-- Type: text/x-patch, Size: 3642 bytes --]

From 6e6a7110f950e3d90978637b21fcb913934be9b1 Mon Sep 17 00:00:00 2001
From: Michael Eager <eager@eagercon.com>
Date: Tue, 10 Dec 2013 11:50:27 -0800
Subject: [PATCH] Fix regression symbol name different from linkage name.

gdb:
*  dwarf2read.c (die_needs_namespace): Do nothing for C language.

testsuite:
*  gdb.base/break-linker-symname.c: New.
*  gdb.base/break-linker-symname.exp: New.
---
 gdb/dwarf2read.c                                |    3 ++
 gdb/testsuite/gdb.base/break-linker-symname.c   |   28 +++++++++++++++++++++++
 gdb/testsuite/gdb.base/break-linker-symname.exp |   25 ++++++++++++++++++++
 3 files changed, 56 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/break-linker-symname.c
 create mode 100644 gdb/testsuite/gdb.base/break-linker-symname.exp

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 3974d0b..d2a236c 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -8044,6 +8044,9 @@ die_needs_namespace (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct attribute *attr;
 
+  if (cu->language == language_c)
+    return 0;
+
   switch (die->tag)
     {
     case DW_TAG_namespace:
diff --git a/gdb/testsuite/gdb.base/break-linker-symname.c b/gdb/testsuite/gdb.base/break-linker-symname.c
new file mode 100644
index 0000000..8728ea1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/break-linker-symname.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+void goo (int b) asm("_goo");
+
+void goo (int b) 
+{
+} 
+
+int main (void)
+{
+  goo (4);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/break-linker-symname.exp b/gdb/testsuite/gdb.base/break-linker-symname.exp
new file mode 100644
index 0000000..2a86f37
--- /dev/null
+++ b/gdb/testsuite/gdb.base/break-linker-symname.exp
@@ -0,0 +1,25 @@
+#   Copyright 2013 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/>.
+
+# Test that 'set breakpoint always-inserted 1' is not a brick
+# Also verifies that breakpoint enabling/disabling works properly
+# with duplicated breakpoints.
+
+if { [prepare_for_testing break-linker-symname.exp break-linker-symname break-linker-symname.c] } {
+    return -1
+}
+
+gdb_test "break _goo" "Breakpoint 1.*" "set breakpoint on linker symbol"
+gdb_test "break goo" "Breakpoint 2.*" "set breakpoint on source symbol"
-- 
1.7.1

Re: [PATCH] Regression setting breakpoint

[Sergio Durigan Junior]

On Wednesday, December 11 2013, Michael Eager wrote:

> From 6e6a7110f950e3d90978637b21fcb913934be9b1 Mon Sep 17 00:00:00 2001
> From: Michael Eager <eager@eagercon.com>
> Date: Tue, 10 Dec 2013 11:50:27 -0800
> Subject: [PATCH] Fix regression symbol name different from linkage name.
>
> gdb:
> *  dwarf2read.c (die_needs_namespace): Do nothing for C language.
>
> testsuite:
> *  gdb.base/break-linker-symname.c: New.
> *  gdb.base/break-linker-symname.exp: New.

Not really a code review, but the ChangeLog entries should be of the
form:

2014-01-16  Your Name  <you@email.com>

	* file.c (function): What changed.

Also...

> diff --git a/gdb/testsuite/gdb.base/break-linker-symname.exp b/gdb/testsuite/gdb.base/break-linker-symname.exp
> new file mode 100644
> index 0000000..2a86f37
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/break-linker-symname.exp
> @@ -0,0 +1,25 @@
> +#   Copyright 2013 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/>.
> +
> +# Test that 'set breakpoint always-inserted 1' is not a brick
> +# Also verifies that breakpoint enabling/disabling works properly
> +# with duplicated breakpoints.
> +
> +if { [prepare_for_testing break-linker-symname.exp break-linker-symname break-linker-symname.c] } {
> +    return -1
> +}

You can rewrite this part as:

-----
  standard_testfile

  if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
    return -1
  }
-----

Thanks,

-- 
Sergio

Re: [PATCH] Regression setting breakpoint

[Joel Brobecker]

> Not really a code review, but the ChangeLog entries should be of the
> form:
> 
> 2014-01-16  Your Name  <you@email.com>
> 
> 	* file.c (function): What changed.

FWIW, I have been using:

| gdb/ChangeLog:
|
|         * file.c (function): What changed.
|
| gdb/testsuite/ChangeLog:
|
|         * gdb.base/file.exp: Bla bla bla.

Two reasons: It makes it obvious which ChangeLog the entries refer to,
and it avoids the need to put a date in the revision log, which may
become obe due to the time it takes to get the patch in after it is
written.

The format with dates are accepted as well, of course. And I have
seen people add the directory usually on the line just ahead of
the CL entry. Eg:

| gdb/:
| 2014-01-16  Your Name  <you@example.com>

-- 
Joel

Re: [PATCH] Regression setting breakpoint

[Michael Eager]

Ping.

Any comments?

On 12/11/13 12:38, Michael Eager wrote:
> There is a regression when setting a breakpoint which was introduced
> in gdb-7.3 by a fix in this patch:
> https://sourceware.org/ml/gdb-patches/2010-03/msg00220.html
>
> The regression shows up in C when the linkage name is different
> from the internal symbol name.  For example,
>
>     void goo (int b) asm("_goo");
>     void goo (int b) {}
>
> will create a function with the linkage name of "_goo" and the DWARF
> will describe the source name of "goo".  There is both a DW_AT_name and
> DW_AT_linkage_name in the DIE for this function.  Some compilers
> (e.g., ICC) have options allowing linkage names to have characters
> prepended to the source name so that the linkage name is different
> from the source name and can not be computed from it.
>
> Prior to this patch, a breakpoint at "_goo" would find the linkage
> symbol, and one at "goo" would find the source symbol.  After the
> patch, the source name is replace with the linkage name.
>
> The attached patch fixes this by making die_needs_namespace()
> return 0 for any C language program.  There's also a test case.
>
> I'm not sure that this is the right fix for the problem, although
> I'm pretty sure that it should not break anything.  There is code
> in dwarf2_compute_name() which only calls die_needs_namespace()
> for C++, Java, or Fortran programs, which could be moved into
> die_needs_namespace().  Possibly the fix should be in dwarf2_physname()
> where the call to die_needs_namespace() is done before the check for
> DW_AT_linkage_name is done.
>
> Comment:  Much of the code in dwarf2_compute_name(), dwarf2_physname(),
> and related functions seems complex, convoluted, and confusing (the
> big Three C's).  The source name for a symbol is not the same as
> the fully qualified name and it's not the same as the linkage name.
> That a language supports FQNs with namespaces does not seem to me
> to say that this should have anything to do with what the linkage (aka
> physical) name for the symbol is.
>


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077