[RFA] Ignore data minimal symbols for breakpoint linespecs

[RFA] Ignore data minimal symbols for breakpoint linespecs

[Joel Brobecker]

Hello,

This patch fixes regressions that showed up gdb.ada/task_bp.exp.

    FAIL: gdb.ada/task_bp.exp: break pck.dummy_task - from psymtab
    FAIL: gdb.ada/task_bp.exp: break pck.dummy_task - from full symtab

Basically, trying to insert a breakpoint on a task using its name
as the linespec yields two locations:

    (gdb) break pck.dummy_task
    Breakpoint 1 at 0x40321f: pck.dummy_task. (2 locations)

One of those locations is invalid, and comes from minimal symtabs:

    (gdb) info break
    Num     Type           Disp Enb Address            What
    1       breakpoint     keep y   <MULTIPLE>
    1.1                         y     0x000000000040321f in pck.dummy_task
                                                       at pck.adb:4
    1.2                         y     0x0000000000639da0 <pck__dummy_task>

The second location is unexpected. The debugger correctly rejected
pck__dummy_task from the debug info (this is variable Pck.Dummy_Task),
but still fished it out of the minimal symbol table.  The reason why
it should be rejected in this case is because this is a data symbol,
not a text one. So inserting a breakpoint at this address is unexpected,
and also has the effect of modifying this variable's value, thus modifying
the inferior's behavior.

For easier testing, I reproduced the problem with C files (testcase).
The reproducer also demonstrates how the program behavior is affected
(the global variable value gets corrupted).

The fix consists in filtering out those data symbols, unless we're
trying to print the line info of a given linespec. Without the fix,
the testcase fails as follow:

    FAIL: gdb.base/dmsym.exp: break pck__foo__bar__minsym
    FAIL: gdb.base/dmsym.exp: print val

gdb/ChangeLog:

        * linespec.c (struct collect_minsyms) [list_mode]: New field.
        (add_minsym): Ignore data symbols if not in list mode.
        (search_minsyms_for_name): Set local.list_mode.

gdb/testsuite/ChangeLog:

        * gdb.base/dmsym.c, gdb.base/dmsym_main.c, gdb.base/dmsym.exp:
        New files.

Tested on x86_64-linux. OK to commit?
Thanks,
-- 
Joel

---
 gdb/linespec.c                      |   17 +++++++
 gdb/testsuite/gdb.base/dmsym.c      |   25 ++++++++++
 gdb/testsuite/gdb.base/dmsym.exp    |   87 +++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/dmsym_main.c |   36 ++++++++++++++
 4 files changed, 165 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dmsym.c
 create mode 100644 gdb/testsuite/gdb.base/dmsym.exp
 create mode 100644 gdb/testsuite/gdb.base/dmsym_main.c

diff --git a/gdb/linespec.c b/gdb/linespec.c
index e49292e..9d753e5 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2749,6 +2749,9 @@ struct collect_minsyms
   /* The funfirstline setting from the initial call.  */
   int funfirstline;
 
+  /* The list_mode setting from the initial call.  */
+  int list_mode;
+
   /* The resulting symbols.  */
   VEC (minsym_and_objfile_d) *msyms;
 };
@@ -2799,6 +2802,19 @@ add_minsym (struct minimal_symbol *minsym, void *d)
   struct collect_minsyms *info = d;
   minsym_and_objfile_d mo;
 
+  /* Exclude data symbols when looking for breakpoint locations.   */
+  if (!info->list_mode)
+    switch (minsym->type)
+      {
+	case mst_slot_got_plt:
+	case mst_data:
+	case mst_bss:
+	case mst_abs:
+	case mst_file_data:
+	case mst_file_bss:
+	return;
+      }
+
   mo.minsym = minsym;
   mo.objfile = info->objfile;
   VEC_safe_push (minsym_and_objfile_d, info->msyms, &mo);
@@ -2829,6 +2845,7 @@ search_minsyms_for_name (struct collect_info *info, const char *name,
 
     memset (&local, 0, sizeof (local));
     local.funfirstline = info->state->funfirstline;
+    local.list_mode = info->state->list_mode;
 
     cleanup = make_cleanup (VEC_cleanup (minsym_and_objfile_d),
 			    &local.msyms);
diff --git a/gdb/testsuite/gdb.base/dmsym.c b/gdb/testsuite/gdb.base/dmsym.c
new file mode 100644
index 0000000..889e800
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dmsym.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 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/>.  */
+
+int pck__foo__bar__minsym = 123;
+
+int
+get_pck__foo__bar__minsym (void)
+{
+  pck__foo__bar__minsym++;
+  return pck__foo__bar__minsym;
+}
diff --git a/gdb/testsuite/gdb.base/dmsym.exp b/gdb/testsuite/gdb.base/dmsym.exp
new file mode 100644
index 0000000..2c27f25
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dmsym.exp
@@ -0,0 +1,87 @@
+# Copyright (C) 2011 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/>.
+
+set testfile dmsym_main
+
+# Build dmsym_main using two C files:
+#   - dmsym.c, which needs to be built without debug info;
+#   - dmsym_main.c, which needs to be build with debug info.
+# This is why we use gdb_compile instead of relying on the usual
+# call to prepare_for_testing.
+
+if {[gdb_compile "${srcdir}/${subdir}/dmsym.c" \
+                 ${objdir}/${subdir}/dmsym.o \
+                 object {}] != ""} {
+  untested dmsym.exp
+  return -1
+}
+
+if {[gdb_compile \
+      [list ${srcdir}/${subdir}/dmsym_main.c ${objdir}/${subdir}/dmsym.o] \
+      ${objdir}/${subdir}/${testfile} \
+      executable {debug}] != ""} {
+    untested dmsym.exp
+    return -1
+}
+
+clean_restart ${testfile}
+
+# Some convenient regular expressions...
+set num "\[0-9\]+"
+set addr "0x\[0-9a-zA-Z\]+"
+
+# Although the test program is written in C, the original problem
+# occurs only when the language is Ada. The use of a C program is
+# only a convenience to be able to exercise the original problem
+# without requiring an Ada compiler. In the meantime, temporarily
+# force the language to Ada.
+
+gdb_test_no_output "set lang ada"
+
+# Verify that setting a breakpoint on `pck__foo__bar__minsym' only
+# results in one location found (function pck__foo__bar__minsym__2).
+# A mistake would be to also insert a breakpoint where
+# pck__foo__bar__minsym is defined.  Despite the fact that there is
+# no debugging info available, this is a data symbol and thus should
+# not be used for breakpoint purposes.
+
+gdb_test "break pck__foo__bar__minsym" \
+         "Breakpoint $num at $addr.: file .*dmsym_main\\.c, line $num\\."
+
+# However, verify that the `info line' command, on the other hand,
+# finds both locations.
+
+gdb_test "info line pck__foo__bar__minsym" \
+         "Line $num of \".*dmsym_main\\.c\" .*\r\nNo line number information available for address $addr <pck__foo__bar__minsym>"
+
+gdb_test_no_output "set lang auto"
+
+# Now, run the program until we get past the call to
+# pck__foo__bar__minsym__2. Except when using hardware breakpoints,
+# inferior behavior is going to be affected if a breakpoint was
+# incorrectly inserted at pck__foo__bar__minsym.
+
+gdb_breakpoint dmsym_main.c:[gdb_get_line_number "BREAK" dmsym_main.c]
+
+gdb_run_cmd
+gdb_test "" \
+         "Breakpoint $num, pck__foo__bar__minsym__2 \\(\\) at.*" \
+         "Run until breakpoint at BREAK"
+
+gdb_test "continue" \
+         "Breakpoint $num, main \\(\\) at.*"
+
+gdb_test "print val" \
+         " = 124"
diff --git a/gdb/testsuite/gdb.base/dmsym_main.c b/gdb/testsuite/gdb.base/dmsym_main.c
new file mode 100644
index 0000000..02cfcb7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dmsym_main.c
@@ -0,0 +1,36 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011 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 get_pck__foo__bar__minsym (void);
+
+int
+pck__foo__bar__minsym__2 (void)
+{
+  return get_pck__foo__bar__minsym ();
+}
+
+int
+main (void)
+{
+  int val = pck__foo__bar__minsym__2 ();
+
+  if (val != 124) /* BREAK */
+    return 1;
+  return 0;
+}
+
+
-- 
1.7.1

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Joel Brobecker]

> This patch fixes regressions that showed up gdb.ada/task_bp.exp.
> 
>     FAIL: gdb.ada/task_bp.exp: break pck.dummy_task - from psymtab
>     FAIL: gdb.ada/task_bp.exp: break pck.dummy_task - from full symtab

This patch also happen to fix the gdb.base/fixsection.exp regression
reported by Jan :-). (http://www.sourceware.org/ml/gdb-patches/2011-12/msg00560.html)

    -PASS: gdb.base/fixsection.exp: breakpoint at static_fun
    +FAIL: gdb.base/fixsection.exp: breakpoint at static_fun

It's exactly the same problem: You have a data symbol named
static_fun inside the minimal symbol table of fixsectshr.sl. After
having found all symbols from the debugging info, GDB searched
the minimal symbol tables and found that data symbol too.

    (gdb) b static_fun
    Breakpoint 2 at 0x4005fc: static_fun. (2 locations)
    (gdb) info break
    Num   Type        Disp Enb Address            What
    2     breakpoint  keep y   <MULTIPLE>
    2.1                    y     0x00000000004005fc in static_fun
                                                  at /[...]/fixsection.c:26
    2.2                    y     0x00007ffff7dde798 <static_fun>
    (gdb) info symbol 0x00007ffff7dde798
    static_fun in section .bss of /[...]/fixsectshr.sl

> gdb/ChangeLog:
> 
>         * linespec.c (struct collect_minsyms) [list_mode]: New field.
>         (add_minsym): Ignore data symbols if not in list mode.
>         (search_minsyms_for_name): Set local.list_mode.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.base/dmsym.c, gdb.base/dmsym_main.c, gdb.base/dmsym.exp:
>         New files.

-- 
Joel

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Jan Kratochvil]

On Fri, 23 Dec 2011 12:01:26 +0100, Joel Brobecker wrote:
> > This patch fixes regressions that showed up gdb.ada/task_bp.exp.
> > 
> >     FAIL: gdb.ada/task_bp.exp: break pck.dummy_task - from psymtab
> >     FAIL: gdb.ada/task_bp.exp: break pck.dummy_task - from full symtab
> 
> This patch also happen to fix the gdb.base/fixsection.exp regression
> reported by Jan :-). (http://www.sourceware.org/ml/gdb-patches/2011-12/msg00560.html)

I found your patch OK BTW.


Thanks,
Jan

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * linespec.c (struct collect_minsyms) [list_mode]: New field.
>         (add_minsym): Ignore data symbols if not in list mode.
>         (search_minsyms_for_name): Set local.list_mode.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.base/dmsym.c, gdb.base/dmsym_main.c, gdb.base/dmsym.exp:
>         New files.

I would normally wait to give the original author (Tom in this case)
some time to comment, but given that it fixes a couple of regressions
as well, and the fact that Jan is OK, I went ahead and checked it in.
If Tom has some comments, I don't mind fixing or reverting at all.

This is HEAD only. I think we'll want that for the branch, but I'll
wait for Tom's feedback, if any.

-- 
Joel

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Edjunior Barbosa Machado]

On 12/27/2011 02:11 AM, Joel Brobecker wrote:

>> gdb/ChangeLog:
>>
>>         * linespec.c (struct collect_minsyms) [list_mode]: New field.
>>         (add_minsym): Ignore data symbols if not in list mode.
>>         (search_minsyms_for_name): Set local.list_mode.
>>
>> gdb/testsuite/ChangeLog:
>>
>>         * gdb.base/dmsym.c, gdb.base/dmsym_main.c, gdb.base/dmsym.exp:
>>         New files.
> 
> I would normally wait to give the original author (Tom in this case)
> some time to comment, but given that it fixes a couple of regressions
> as well, and the fact that Jan is OK, I went ahead and checked it in.
> If Tom has some comments, I don't mind fixing or reverting at all.
> 
> This is HEAD only. I think we'll want that for the branch, but I'll
> wait for Tom's feedback, if any.
> 


Hi,
I noticed this patch unfortunately caused around 300 regressions in the testsuite running on ppc64, with failures in several testcases such as:

gdb.asm/asm-source.exp
gdb.base/solib-weak.exp
gdb.base/break-interp.exp
gdb.cp/cmpd-minsyms.exp
gdb.cp/exception.exp
gdb.cp/meth-typedefs.exp
gdb.cp/ovsrch.exp

and others.

Please let me know if you need further info or the testsuite results.

Thanks,
-- 
Edjunior

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Joel Brobecker]

> I noticed this patch unfortunately caused around 300 regressions in
> the testsuite running on ppc64, with failures in several testcases
> such as:

Unfortunately, this is not a platform that I have access to, so
it's probably going to be hard for me to determine what the problem
is.

Can you send me the logs of the tests that fail? Can you also
include a log of the same tests, but before the patch is applied.
(please, if possible, only include the testcases that regress)

Perhaps we'll get lucky and the output will give us a clue.
Otherwise, I'll probably need a hand from you.

-- 
Joel

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Mark Kettenis]

> Date: Tue, 27 Dec 2011 20:48:37 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > I noticed this patch unfortunately caused around 300 regressions in
> > the testsuite running on ppc64, with failures in several testcases
> > such as:
> 
> Unfortunately, this is not a platform that I have access to, so
> it's probably going to be hard for me to determine what the problem
> is.
> 
> Can you send me the logs of the tests that fail? Can you also
> include a log of the same tests, but before the patch is applied.
> (please, if possible, only include the testcases that regress)
> 
> Perhaps we'll get lucky and the output will give us a clue.
> Otherwise, I'll probably need a hand from you.

Give that the target in question is 64-bit PowerPC, my bet is that by
discarding data minimal symbols, you're also discarding function
descriptors.  That's probably not a good idea.

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Joel Brobecker]

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

> Give that the target in question is 64-bit PowerPC, my bet is that by
> discarding data minimal symbols, you're also discarding function
> descriptors.  That's probably not a good idea.

Thanks for the tip, Mark. After having looked at the logs, I think
you are right on the money. What do you think of the attached patch?

Edjunior, can you test it for me?

Thanks,
-- 
Joel

[-- Attachment #2: 0001-linespec-keep-function-descriptors-during-minimal-sy.patch --]
[-- Type: text/x-diff, Size: 1295 bytes --]

From af87a92e11cf40dc2f2fcc358cf405e3cb2905ec Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 28 Dec 2011 09:35:35 +0400
Subject: [PATCH] linespec: keep function descriptors during minimal symbol search

When discarding data (minimal) symbols, we need to be careful to
not throw away the function descriptors.  This makes a different
on platforms where these descriptors are used and live in a data
section.

gdb/ChangeLog:

        * linespec.c (add_minsym): Preserve function descriptors.
---
 gdb/linespec.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 9d753e5..1266418 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2812,7 +2812,17 @@ add_minsym (struct minimal_symbol *minsym, void *d)
 	case mst_abs:
 	case mst_file_data:
 	case mst_file_bss:
-	return;
+	  {
+	    /* Make sure this minsym is not a function descriptor
+	       before we decide to discard it.  */
+	    struct gdbarch *gdbarch = info->objfile->gdbarch;
+	    CORE_ADDR addr = gdbarch_convert_from_func_ptr_addr
+			       (gdbarch, SYMBOL_VALUE_ADDRESS (minsym),
+				&current_target);
+
+	    if (addr == SYMBOL_VALUE_ADDRESS (minsym))
+	      return;
+	  }
       }
 
   mo.minsym = minsym;
-- 
1.7.1

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Edjunior Barbosa Machado]

On 12/28/2011 04:02 AM, Joel Brobecker wrote:

>> Give that the target in question is 64-bit PowerPC, my bet is that by
>> discarding data minimal symbols, you're also discarding function
>> descriptors.  That's probably not a good idea.
> 
> Thanks for the tip, Mark. After having looked at the logs, I think
> you are right on the money. What do you think of the attached patch?
> 
> Edjunior, can you test it for me?
> 
> Thanks,


Hi Joel,

excellent, I've tested it on ppc64 and it fixes all those regressions I mentioned.

Thanks a lot!
-- 
Edjunior

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Joel Brobecker]

> excellent, I've tested it on ppc64 and it fixes all those regressions
> I mentioned.

Excellent. It would be nice to have someone else's feedback before
I commit, as I am not very familiar with this sort of thing (I tend
to have a simpler view of things, as the first patch demonstrated).

-- 
Joel

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Ulrich Weigand]

Joel Brobecker wrote:
> > excellent, I've tested it on ppc64 and it fixes all those regressions
> > I mentioned.
> 
> Excellent. It would be nice to have someone else's feedback before
> I commit, as I am not very familiar with this sort of thing (I tend
> to have a simpler view of things, as the first patch demonstrated).

I was seeing the same regressions on ppc64, and the patch indeed
fixes them.  I'm wondering whether this is best place to perform
this check, though -- maybe it might be better to label function
descriptor minsyms as mst_text to start with (or possibly some
new category) ...

But in the absence of such a solution, I'd still be in favor of
committing your patch as is, given that it fixes a serious
regression and doesn't look like it can make things worse.


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> I'm wondering whether this is best place to perform
Ulrich> this check, though -- maybe it might be better to label function
Ulrich> descriptor minsyms as mst_text to start with (or possibly some
Ulrich> new category) ...

Yeah, I wonder that too.

Ulrich> But in the absence of such a solution, I'd still be in favor of
Ulrich> committing your patch as is, given that it fixes a serious
Ulrich> regression and doesn't look like it can make things worse.

I agree.

Tom

Re: [RFA] Ignore data minimal symbols for breakpoint linespecs

[Joel Brobecker]

> Ulrich> But in the absence of such a solution, I'd still be in favor of
> Ulrich> committing your patch as is, given that it fixes a serious
> Ulrich> regression and doesn't look like it can make things worse.
> 
> I agree.

OK, thanks for taking a look, guys. Both patches are now in HEAD + branch...

-- 
Joel