[RFA] wrong language used when re-setting breakpoint

[RFA] wrong language used when re-setting breakpoint

[Joel Brobecker]

The debugger sometimes fails to re-set a breakpoint as follow,
causing it to become disabled:

    (gdb) b nested_sub
    Breakpoint 1 at 0x401cec: file foo.adb, line 7.
    (gdb) b do_nothing
    Breakpoint 2 at 0x401cdc: file pck.adb, line 4.
    (gdb) run
    Starting program: /[...]/foo
    Error in re-setting breakpoint 1: Function "nested_sub" not defined.
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    Breakpoint 2, pck.do_nothing () at pck.adb:4
    4             null;

This only happens on machines where the debug-file-directory is
a valid directory name.

The reason behind the error is that the linespec code that re-sets
the breakpoints uses the current_language global when iterating
over a symtab's symbols. However, the that global gets switched from
Ada to C during the startup phase, probably as a side-effect of stopping
in some system code for which debugging info is available. The fix
is to make sure that we use the correct language.

gdb/ChangeLog:

        * linespec.c (iterate_over_all_matching_symtabs): Use the correct
        language when iterating over symbols.

gdb/testsuite/ChangeLog:

        * gdb.ada/bp_reset: New testcase.

Tested on x86_64-linux, no regression.
OK to commit?

Thank you,
-- 
Joel


---
 gdb/linespec.c                         |    7 +++---
 gdb/testsuite/gdb.ada/bp_reset.exp     |   37 ++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_reset/foo.adb |   27 +++++++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_reset/io.adb  |   21 ++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_reset/io.ads  |   18 ++++++++++++++++
 gdb/testsuite/gdb.ada/bp_reset/pck.adb |   21 ++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_reset/pck.ads |   18 ++++++++++++++++
 7 files changed, 146 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/bp_reset.exp
 create mode 100644 gdb/testsuite/gdb.ada/bp_reset/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/bp_reset/io.adb
 create mode 100644 gdb/testsuite/gdb.ada/bp_reset/io.ads
 create mode 100644 gdb/testsuite/gdb.ada/bp_reset/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/bp_reset/pck.ads

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 86239c9..f7ff54e 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1010,7 +1010,8 @@ iterate_over_all_matching_symtabs (struct linespec_state *state,
 	  struct block *block;
 
 	  block = BLOCKVECTOR_BLOCK (BLOCKVECTOR (symtab), STATIC_BLOCK);
-	  LA_ITERATE_OVER_SYMBOLS (block, name, domain, callback, data);
+	  state->language->la_iterate_over_symbols (block, name, domain,
+						    callback, data);
 
 	  if (include_inline)
 	    {
@@ -1021,8 +1022,8 @@ iterate_over_all_matching_symtabs (struct linespec_state *state,
 		   i < BLOCKVECTOR_NBLOCKS (BLOCKVECTOR (symtab)); i++)
 		{
 		  block = BLOCKVECTOR_BLOCK (BLOCKVECTOR (symtab), i);
-		  LA_ITERATE_OVER_SYMBOLS (block, name, domain,
-					   iterate_inline_only, &cad);
+		  state->language->la_iterate_over_symbols
+		    (block, name, domain, iterate_inline_only, &cad);
 		}
 	    }
 	}
diff --git a/gdb/testsuite/gdb.ada/bp_reset.exp b/gdb/testsuite/gdb.ada/bp_reset.exp
new file mode 100644
index 0000000..aff3341
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_reset.exp
@@ -0,0 +1,37 @@
+# Copyright 2012 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/>.
+
+load_lib "ada.exp"
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+gdb_test "break nested_sub" \
+         "Breakpoint $decimal at $hex: file .*foo.adb, line $decimal\\."
+
+gdb_test "break do_nothing" \
+         "Breakpoint $decimal at $hex: file .*pck.adb, line $decimal\\."
+
+# Run the program. Make sure the program runs until it hits
+# the first breakpoint inside nested_sub.
+
+gdb_run_cmd
+gdb_test "" "Breakpoint $decimal, foo\\.nested_sub \\(\\).*"
+
diff --git a/gdb/testsuite/gdb.ada/bp_reset/foo.adb b/gdb/testsuite/gdb.ada/bp_reset/foo.adb
new file mode 100644
index 0000000..27298f4
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_reset/foo.adb
@@ -0,0 +1,27 @@
+--  Copyright 2012 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/>.
+
+with Pck; use Pck;
+with IO; use IO;
+
+procedure Foo is
+   procedure Nested_Sub is
+   begin
+      Put_Line ("Some string");
+   end Nested_Sub;
+begin
+   Nested_Sub;
+   Do_Nothing;
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/bp_reset/io.adb b/gdb/testsuite/gdb.ada/bp_reset/io.adb
new file mode 100644
index 0000000..ea7306e
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_reset/io.adb
@@ -0,0 +1,21 @@
+--  Copyright 2012 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/>.
+
+package body IO is
+   procedure Put_Line (S : String) is
+   begin
+      null;
+   end Put_Line;
+end IO;
diff --git a/gdb/testsuite/gdb.ada/bp_reset/io.ads b/gdb/testsuite/gdb.ada/bp_reset/io.ads
new file mode 100644
index 0000000..65772db
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_reset/io.ads
@@ -0,0 +1,18 @@
+--  Copyright 2012 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/>.
+
+package IO is
+   procedure Put_Line (S : String);
+end IO;
diff --git a/gdb/testsuite/gdb.ada/bp_reset/pck.adb b/gdb/testsuite/gdb.ada/bp_reset/pck.adb
new file mode 100644
index 0000000..ea1d233
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_reset/pck.adb
@@ -0,0 +1,21 @@
+--  Copyright 2012 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/>.
+
+package body Pck is
+   procedure Do_Nothing is
+   begin
+      null;
+   end Do_Nothing;
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/bp_reset/pck.ads b/gdb/testsuite/gdb.ada/bp_reset/pck.ads
new file mode 100644
index 0000000..c8f9ec9
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_reset/pck.ads
@@ -0,0 +1,18 @@
+--  Copyright 2012 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/>.
+
+package Pck is
+   procedure Do_Nothing;
+end Pck;
-- 
1.7.9.5

Re: [RFA] wrong language used when re-setting breakpoint

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> The reason behind the error is that the linespec code that re-sets
Joel> the breakpoints uses the current_language global when iterating
Joel> over a symtab's symbols. However, the that global gets switched from
Joel> Ada to C during the startup phase, probably as a side-effect of stopping
Joel> in some system code for which debugging info is available. The fix
Joel> is to make sure that we use the correct language.

I think the breakpoint re-setting code should be temporarily setting
current_language.  breakpoint_re_set does this.

I see that linespec now is trying to treat the current language as a
parameter, not a global.  I think that's a good idea, but I wonder
whether it is done consistently enough.  For instance, does linespec
really not call anything that might implicitly use the global?  (I
expect at least expression parsing...).  And offhand I see a use of
current_language in get_search_block.

I don't have a problem with your patch.  I think it is an improvement.
But I think it is important to understand why the current mechanism
isn't working, since I think a failure here may have other bad effects
as well.

Tom

Re: [RFA] wrong language used when re-setting breakpoint

[Joel Brobecker]

> I think the breakpoint re-setting code should be temporarily setting
> current_language.  breakpoint_re_set does this.

I kind of agree, but ... at the same time, this type of changes then
makes it harder for us to make progress in getting rid of the
current_language global, no?

Also, I think that relying on the global being set for a defined period
of time is a little risky. It seems too easy for the current_language
to change right from under us as a side-effect of some sub-action.

> I don't have a problem with your patch.  I think it is an improvement.
> But I think it is important to understand why the current mechanism
> isn't working, since I think a failure here may have other bad effects
> as well.

OK - I will commit the patch, then. But I think we'd be better off
trying to find the other bad effects, and see if we can use a parameter
to fix them, rather than making sure the global is set properly.
The problem with my suggested approach, however, is that we have
to wait for bugs that reveal those bad effects.

Thanks,
-- 
Joel

Re: [RFA] wrong language used when re-setting breakpoint

[Tom Tromey]

Joel> I kind of agree, but ... at the same time, this type of changes then
Joel> makes it harder for us to make progress in getting rid of the
Joel> current_language global, no?

Nope, sorry, what I'm suggesting is diagnosing and fixing this problem
first, then putting your patch in subsequently.

Joel> Also, I think that relying on the global being set for a defined period
Joel> of time is a little risky. It seems too easy for the current_language
Joel> to change right from under us as a side-effect of some sub-action.

Yes, I agree, and I think that's why the direction linespec is headed is
a good one.  But, temporarily setting current_language is how gdb has
worked for a long time, and if that broke, it probably means a
regression or bug elsewhere.  And, linespec isn't actually fully
parameterized like this anyway.

Tom

Re: [RFA] wrong language used when re-setting breakpoint

[Tom Tromey]

Joel> OK - I will commit the patch, then. 

I was going to look into the issue, but I reverted the linespec.c change
locally but bp_reset.exp still passes.  I do have a valid
debug-file-directory.  Is there something else I need to do to see the
failure?

Tom

Re: [RFA] wrong language used when re-setting breakpoint

[Joel Brobecker]

> I was going to look into the issue, but I reverted the linespec.c change
> locally but bp_reset.exp still passes.  I do have a valid
> debug-file-directory.  Is there something else I need to do to see the
> failure?

I think it's very sensitive to which modules you have in your
debug-file-directory. The machine on which I reproduced the testcase
has a ton of stuff in there, and I didn't spend the time to figure out
which one it is that causes the problem.

That being said, when doing the same, I discovered that the testcase
indeed passes without my patch. I did the investigation and patching
using an older version of GDB, and simply merged it when it came to
submitting it. I didn't think of running the testcase without my patch
with the HEAD!

A recent change must have disturbed a little the balance. I'll try
looking into it a little deeper. Do you think I should revert my patch?

-- 
Joel

Re: [RFA] wrong language used when re-setting breakpoint

[Tom Tromey]

Joel> A recent change must have disturbed a little the balance. I'll try
Joel> looking into it a little deeper. Do you think I should revert my patch?

My concern was really that your patch might be obscuring some other bug.
But, it sounds like this other bug no longer exists.  In that case I
think your patch is a cleanup and should stay in.

Tom

Re: [RFA] wrong language used when re-setting breakpoint

[Joel Brobecker]

> My concern was really that your patch might be obscuring some other bug.
> But, it sounds like this other bug no longer exists.  In that case I
> think your patch is a cleanup and should stay in.

Makes sense. Thank you.

In the meantime, I have found the source of the language switch, and
I know why you're not seeing it.  There is a function called
ada-lang.c:add_symbols_from_enclosing_procs which is empty in the
official tree, but starts with the following code in our tree:

   frame = deprecated_safe_get_selected_frame ();

(not sure why this code hasn't been contributed yet, I will look into
that this winter after Xmas).

This function gets called during the breakpoint re-set phase of
the "nested_sub" breakpoint, via the ada_lookup_symbol_list function.
The exact sequence is:
    ada_lookup_symbol_list
    -> ada_add_local_symbols
    -> add_symbols_from_enclosing_procs
    -> deprecated_safe_get_selected_frame
    [...]
    -> select_frame
    -> set_language

The frame in question is in the loader (/usr/lib/debug/lib/ld-2.11.1.so).

You can probably reproduce the same problem by adding the call in
your version of add_symbols_from_enclosing_procs. Perhaps we (AdaCore)
should also do something to avoid the language-switch side-effect,
but I am hesitant to do anything, because I'm thinking I might affect
something else later on.

-- 
Joel

Re: [RFA] wrong language used when re-setting breakpoint

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> In the meantime, I have found the source of the language switch, and
Joel> I know why you're not seeing it.  There is a function called
Joel> ada-lang.c:add_symbols_from_enclosing_procs which is empty in the
Joel> official tree, but starts with the following code in our tree:
Joel>    frame = deprecated_safe_get_selected_frame ();

Ah, I see.

Joel> Perhaps we (AdaCore) should also do something to avoid the
Joel> language-switch side-effect, but I am hesitant to do anything,
Joel> because I'm thinking I might affect something else later on.

I don't really know the Ada code well enough to comment.
The current code you have sounds tricky, maybe too tricky.

I wouldn't mind pushing linespec to be more fully parameterized.
However, this is maybe non-trivial if it calls out into other parts of
gdb.

I do see that linespec itself takes care to restore current_language in
a similar scenario; see get_search_block.  But I wonder whether this can
have the effect of accidentally dropping a language switch.

Tom