[RFC/commit] parse breakpoint condition according to breakpoint location

[RFC/commit] parse breakpoint condition according to breakpoint location

[Joel Brobecker]

Hello,

This is a patch that was actually approved back in 2003:

    http://www.sourceware.org/ml/gdb-patches/2003-09/msg00195.html

... but I never got to commmit it because I discovered right after
that it exposed a flaw in the current design which triggered a problem
elsewhere. An improved version of the patch was then sent:

    http://www.sourceware.org/ml/gdb-patches/2003-09/msg00225.html

... But at the same time, the discussion digressed over the use of
a global. I proposed a plan for dealing with this, but it's a huge
change (adding a language parameter to the parse_* routines), and
I don't think I got feedback on how I was planning to proceed.

Summary of the problem: 

We're debugging a multi-language application, and just stopped inside
some C code. For instance:

    (gdb) b c_function
    Breakpoint 1 at 0x4029c8: file foo.c, line 6.
    (gdb) run
    Starting program: /[...]/a

    Breakpoint 1, c_function () at foo.c:6
    6         callme ();
    (gdb) show lang
    The current source language is "auto; currently c".

The current language is C, but we want to insert a condition breakpoint
inside some Ada Code. But the problem is that the breakpoint condition
gets evaluated using the current language, and this usually triggers
an error.  For instance:

    (gdb) b mixed.adb:15 if light = green
    No symbol "green" in current context.

So the decision was, if the language is auto, to use the language
corresponding to the breakpoint location, not the current frame.
This is what this patch implements.

gdb/
2010-05-09  Joel Brobecker  <brobecker@adacore.com>

        * parse.c (parse_exp_in_context): When block is not NULL, use
        its associated language to parse the expression instead of
        the current_language.

gdb/testsuite/
2010-05-09  Joel Brobecker  <brobecker@adacore.com>

        * gdb.ada/cond_lang: New testcase.

Tested on x86_64-linux. No regression.  This has also been in AdaCore's
tree since 2003, and we haven't had problems with it.

For the longer term, this approach is far from perfect, since it depends
on the current_language global, which may change over time.  For instance,
if the user switches from language auto to language c between the moment
he creates the breakpoint and the moment he runs the program, the condition
will get re-evaluated using C the next time around.  But I think that
this is already quite a good useability improvement and the only part of
the logic that could move elsewhere is the part that computes the language
to use, and that's easily moved out.   So it's not making further refinments
any harder in the future.

Hence the suggestion to include this patch as is.  I can work on cleaning
things up as a separate patch. I will send a followup mail on that. In
the meantime, any objection to this change?

Thanks,
-- 
Joel

---
 gdb/parse.c                               |   35 +++++++++++++++--
 gdb/testsuite/gdb.ada/cond_lang.exp       |   58 +++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/cond_lang/a.adb     |   21 ++++++++++
 gdb/testsuite/gdb.ada/cond_lang/foo.c     |   25 ++++++++++++
 gdb/testsuite/gdb.ada/cond_lang/mixed.adb |   49 ++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/cond_lang/mixed.ads |   20 ++++++++++
 gdb/testsuite/gdb.ada/cond_lang/pck.adb   |   21 ++++++++++
 gdb/testsuite/gdb.ada/cond_lang/pck.ads   |   20 ++++++++++
 8 files changed, 245 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/cond_lang.exp
 create mode 100644 gdb/testsuite/gdb.ada/cond_lang/a.adb
 create mode 100644 gdb/testsuite/gdb.ada/cond_lang/foo.c
 create mode 100644 gdb/testsuite/gdb.ada/cond_lang/mixed.adb
 create mode 100644 gdb/testsuite/gdb.ada/cond_lang/mixed.ads
 create mode 100644 gdb/testsuite/gdb.ada/cond_lang/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/cond_lang/pck.ads

diff --git a/gdb/parse.c b/gdb/parse.c
index 7db6e92..e7e5145 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -1070,6 +1070,7 @@ parse_exp_in_context (char **stringptr, struct block *block, int comma,
 {
   volatile struct gdb_exception except;
   struct cleanup *old_chain;
+  const struct language_defn *lang = NULL;
   int subexp;
 
   lexptr = *stringptr;
@@ -1107,17 +1108,43 @@ parse_exp_in_context (char **stringptr, struct block *block, int comma,
 	expression_context_pc = BLOCK_START (expression_context_block);
     }
 
+  if (language_mode == language_mode_auto && block != NULL)
+    {
+      /* Find the language associated to the given context block.
+         Default to the current language if it can not be determined.
+
+         Note that using the language corresponding to the current frame
+         can sometimes give unexpected results.  For instance, this
+         routine is often called several times during the inferior
+         startup phase to re-parse breakpoint expressions after
+         a new shared library has been loaded.  The language associated
+         to the current frame at this moment is not relevant for
+         the breakpoint. Using it would therefore be silly, so it seems
+         better to rely on the current language rather than relying on
+         the current frame language to parse the expression. That's why
+         we do the following language detection only if the context block
+         has been specifically provided.  */
+      struct symbol *func = block_linkage_function (block);
+
+      if (func != NULL)
+        lang = language_def (SYMBOL_LANGUAGE (func));
+      if (lang == NULL || lang->la_language == language_unknown)
+        lang = current_language;
+    }
+  else
+    lang = current_language;
+
   expout_size = 10;
   expout_ptr = 0;
   expout = (struct expression *)
     xmalloc (sizeof (struct expression) + EXP_ELEM_TO_BYTES (expout_size));
-  expout->language_defn = current_language;
+  expout->language_defn = lang;
   expout->gdbarch = get_current_arch ();
 
   TRY_CATCH (except, RETURN_MASK_ALL)
     {
-      if (current_language->la_parser ())
-	current_language->la_error (NULL);
+      if (lang->la_parser ())
+        lang->la_error (NULL);
     }
   if (except.reason < 0)
     {
@@ -1150,7 +1177,7 @@ parse_exp_in_context (char **stringptr, struct block *block, int comma,
   if (out_subexp)
     *out_subexp = subexp;
 
-  current_language->la_post_parser (&expout, void_context_p);
+  lang->la_post_parser (&expout, void_context_p);
 
   if (expressiondebug)
     dump_prefix_expression (expout, gdb_stdlog);
diff --git a/gdb/testsuite/gdb.ada/cond_lang.exp b/gdb/testsuite/gdb.ada/cond_lang.exp
new file mode 100644
index 0000000..dcd46da
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/cond_lang.exp
@@ -0,0 +1,58 @@
+# Copyright 2010 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/>.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+load_lib "ada.exp"
+
+set testdir "cond_lang"
+set testfile "${testdir}/a"
+set cfile "${testdir}/foo"
+set adasrcfile ${srcdir}/${subdir}/${testfile}.adb
+set csrcfile ${srcdir}/${subdir}/${cfile}.c
+set cobject  ${objdir}/${subdir}/${cfile}.o
+set binfile ${objdir}/${subdir}/${testfile}
+
+file mkdir ${objdir}/${subdir}/${testdir}
+gdb_compile "${csrcfile}" "${cobject}" object [list debug]
+if {[gdb_compile_ada "${adasrcfile}" "${binfile}" executable [list debug]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+# Run to c_function an verify that the language automatically gets set to C.
+runto c_function
+gdb_test "show lang" \
+         "The current source language is \"auto; currently c\"\\."
+
+# Now, insert a breakpoint inside an Ada unit, using a condition written
+# in Ada. Even though the current language is "auto; currently c", we
+# expect the debugger to parse the expression using Ada, because the
+# current language mode is auto, and the breakpoint is inside Ada code.
+set bp_location [gdb_get_line_number "STOP" ${testdir}/mixed.adb]
+gdb_test "break mixed.adb:${bp_location} if light = green" \
+         "Breakpoint \[0-9\]* at .*: file .*/mixed.adb, line \[0-9\]*\\."
+
+# Now, continue until we hit the breakpoint.  If the condition is
+# evaluated correctly, the first hit will be ignored, and the debugger
+# will stop at the second hit only, when the "light" argument is equal
+# to green.
+gdb_test "continue" \
+         "Breakpoint \[0-9\]*, mixed\\.break_me \\(light=green\\) at .*"
+
+
diff --git a/gdb/testsuite/gdb.ada/cond_lang/a.adb b/gdb/testsuite/gdb.ada/cond_lang/a.adb
new file mode 100644
index 0000000..e1cda16
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/cond_lang/a.adb
@@ -0,0 +1,21 @@
+--  Copyright 2010 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 Mixed;
+
+procedure A is
+begin
+   Mixed.Start_Test;
+end A;
diff --git a/gdb/testsuite/gdb.ada/cond_lang/foo.c b/gdb/testsuite/gdb.ada/cond_lang/foo.c
new file mode 100644
index 0000000..fe27fc9
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/cond_lang/foo.c
@@ -0,0 +1,25 @@
+/* Copyright 2010 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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 callme (void);
+
+void
+c_function (void)
+{
+  callme ();
+}
+
diff --git a/gdb/testsuite/gdb.ada/cond_lang/mixed.adb b/gdb/testsuite/gdb.ada/cond_lang/mixed.adb
new file mode 100644
index 0000000..1d5fd31
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/cond_lang/mixed.adb
@@ -0,0 +1,49 @@
+--  Copyright 2010 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;
+
+package body Mixed is
+   --  We are importing symbols from foo.o, so make sure this object file
+   --  gets linked in.
+   Pragma Linker_Options ("foo.o");
+
+   type Color is (Red, Green, Blue);
+
+   procedure C_Function;
+   pragma Import (C, C_Function, "c_function");
+
+   procedure Callme;
+   pragma Export (C, Callme, "callme");
+
+   procedure Break_Me (Light : Color) is
+   begin
+      Put_Line ("Light: " & Color'Image (Light));  --  STOP
+   end Break_Me;
+
+   procedure Callme is
+   begin
+      Break_Me (Red);
+      Break_Me (Green);
+      Break_Me (Blue);
+   end Callme;
+
+   procedure Start_Test is
+   begin
+      --  Call C_Function, which will call Callme.
+      C_Function;
+   end Start_Test;
+
+end Mixed;
diff --git a/gdb/testsuite/gdb.ada/cond_lang/mixed.ads b/gdb/testsuite/gdb.ada/cond_lang/mixed.ads
new file mode 100644
index 0000000..b3a712a
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/cond_lang/mixed.ads
@@ -0,0 +1,20 @@
+--  Copyright 2010 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 Mixed is
+
+   procedure Start_Test;
+
+end Mixed;
diff --git a/gdb/testsuite/gdb.ada/cond_lang/pck.adb b/gdb/testsuite/gdb.ada/cond_lang/pck.adb
new file mode 100644
index 0000000..d412fec
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/cond_lang/pck.adb
@@ -0,0 +1,21 @@
+--  Copyright 2010 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 Put_Line (S : String) is
+   begin
+      null;
+   end Put_Line;
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/cond_lang/pck.ads b/gdb/testsuite/gdb.ada/cond_lang/pck.ads
new file mode 100644
index 0000000..4fbc2a2
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/cond_lang/pck.ads
@@ -0,0 +1,20 @@
+--  Copyright 2010 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 Put_Line (S : String);
+   --  Stub implementation of Put_Line to avoid a dependency on Text_IO.
+   --  Does actually nothing.
+end Pck;
-- 
1.6.3.3

Re: [RFC/commit] parse breakpoint condition according to breakpoint location

[Tom Tromey]

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

Joel> So the decision was, if the language is auto, to use the language
Joel> corresponding to the breakpoint location, not the current frame.
Joel> This is what this patch implements.

Sounds reasonable.

Joel> ... But at the same time, the discussion digressed over the use of
Joel> a global. I proposed a plan for dealing with this, but it's a huge
Joel> change (adding a language parameter to the parse_* routines), and
Joel> I don't think I got feedback on how I was planning to proceed.

For reference the plan is here:

    http://www.sourceware.org/ml/gdb-patches/2003-09/msg00333.html

I would prefer never passing NULL to parse_exp_1 -- just make the
callers explicitly pass current_language.

That is just a minor detail; overall it sounds fine to me.

current_language is a problem global.  It is referenced in a lot of
places where an argument would be preferable.  E.g., even though
expressions carry their language, code in valops.c checks the global, so
you can wind up with odd behavior (at least in theory, I have not tried
to make a reproducer).  Probably, evaluate_expression should call
set_language and restore the old language in a cleanup -- though this is
a workaround and not a clean fix.

Joel> Summary of the problem: 
Joel> For the longer term, this approach is far from perfect, since it
Joel> depends on the current_language global, which may change over
Joel> time.  For instance, if the user switches from language auto to
Joel> language c between the moment he creates the breakpoint and the
Joel> moment he runs the program, the condition will get re-evaluated
Joel> using C the next time around.

Once the condition is successfully parsed, it seems to me that any
re-parsing should be done with that same language.  Maybe this is
already accomplished by the set_language calls in breakpoint.c, I'm not
sure.

Tom

Re: [RFC/commit] parse breakpoint condition according to breakpoint location

[Joel Brobecker]

> current_language is a problem global.  It is referenced in a lot of
> places where an argument would be preferable.  E.g., even though
> expressions carry their language, code in valops.c checks the global, so
> you can wind up with odd behavior (at least in theory, I have not tried
> to make a reproducer).  Probably, evaluate_expression should call
> set_language and restore the old language in a cleanup -- though this is
> a workaround and not a clean fix.

I see that you already starting cleaning some things up in that
department :).

The idea of temporarily switching the current_language global is not
even guaranteed to work, because there is the risk that some functions
that we use actually change it again from under us.  For instance, a
while ago, I ran into a situation where a some code set the
current_language before calling a parsing routine; but that did not work
because the parsing routine called get_current_frame which had a
side-effect of changing the current_language!

> Once the condition is successfully parsed, it seems to me that any
> re-parsing should be done with that same language.

Agreed 100%.

> Maybe this is already accomplished by the set_language calls in
> breakpoint.c, I'm not sure.

I do not think so. I need to work on that followup email I was
referring to (I'm in Paris right now, spending most of my days
meeting with people). I'll do that now...

-- 
Joel

Re: [RFC/commit] parse breakpoint condition according to breakpoint location

[Joel Brobecker]

> gdb/
> 2010-05-09  Joel Brobecker  <brobecker@adacore.com>
> 
>         * parse.c (parse_exp_in_context): When block is not NULL, use
>         its associated language to parse the expression instead of
>         the current_language.
> 
> gdb/testsuite/
> 2010-05-09  Joel Brobecker  <brobecker@adacore.com>
> 
>         * gdb.ada/cond_lang: New testcase.

This patch is now in. I hope to have time to work on the parse routines
soon.

-- 
Joel