[RFA] Fix frame argument printing when using auto language mode
Commit Message
When debugging a mixed Ada/C program using this scenario:
- set print frame-arguements all
- an Ada function named pck.call_me calls a C function named break_me
- you put a breakpoint in break_me and the program reaches this
breakpoint.
Now display the backtrace:
(gdb) bt
#0 break_me () at [...]
#1 0x000000000040243e in pck.call_me (
s={P_ARRAY = 0x7fffffffe21c, P_BOUNDS = 0x41e6e8}) at [...]
whereas we should expect:
(gdb) bt
#0 break_me () at [...]
#1 0x000000000040243e in pck.call_me (s="test") at [...]
The problem is that GDB prints the S parameter in the pck.call_me Ada
function using the current language, so the C one, because the program
is stopped in a C function, whereas it should use the pck.call_me frame
one. This behavior is ok when user manually changes the language but it's
not the right one when language is auto.
This patch fixes this problem so now when using auto language, GDB ensures
that each frame arguments are printed using the language of the frame,
even if the program is stopped in a frame using a different language.
If the user explicitly sets a language (using "set language ...") then
no change here, all the frame arguments are printed using this language.
gdb/ChangeLog:
* stack.c (print_frame_arg): Set/restore current language to each
frame's language when using auto mode.
gdb/testsuite/ChangeLog:
* gdb.ada/frame_arg_lang.exp: New testcase.
* gdb.ada/frame_arg_lang/bla.adb: New file.
* gdb.ada/frame_arg_lang/pck.ads: New file.
* gdb.ada/frame_arg_lang/pck.adb: New file.
* gdb.ada/frame_arg_lang/foo.c: New file.
Tested on x86_64-linux using AdaCore's testsuite.
Change-Id: Ic00e30933905dc72f9c8d2f2fba9ccabf7d02983
---
gdb/stack.c | 24 +++++++--
gdb/testsuite/gdb.ada/frame_arg_lang.exp | 74 ++++++++++++++++++++++++++++
gdb/testsuite/gdb.ada/frame_arg_lang/bla.adb | 22 +++++++++
gdb/testsuite/gdb.ada/frame_arg_lang/foo.c | 22 +++++++++
gdb/testsuite/gdb.ada/frame_arg_lang/pck.adb | 24 +++++++++
gdb/testsuite/gdb.ada/frame_arg_lang/pck.ads | 18 +++++++
6 files changed, 181 insertions(+), 3 deletions(-)
create mode 100644 gdb/testsuite/gdb.ada/frame_arg_lang.exp
create mode 100644 gdb/testsuite/gdb.ada/frame_arg_lang/bla.adb
create mode 100644 gdb/testsuite/gdb.ada/frame_arg_lang/foo.c
create mode 100644 gdb/testsuite/gdb.ada/frame_arg_lang/pck.adb
create mode 100644 gdb/testsuite/gdb.ada/frame_arg_lang/pck.ads
Comments
On Mon, Feb 19, 2018 at 12:52 PM, Xavier Roirand <roirand@adacore.com> wrote:
>
> The problem is that GDB prints the S parameter in the pck.call_me Ada
> function using the current language, so the C one, because the program
> is stopped in a C function, whereas it should use the pck.call_me frame
> one. This behavior is ok when user manually changes the language but it's
> not the right one when language is auto.
Agreed, GDB should use per-frame language instead of global current
language. However, instead of switching global variable current_language,
why don't we pass the per-frame language down to la_val_print? In
ada-valprint.c:ada_val_print,
ada_val_print_1 (type, embedded_offset, address,
stream, recurse, val, options,
current_language);
^^^^^^^^^^^^^^^^^
why don't pass language for ada here?
Le 2/20/18 à 4:15 PM, Yao Qi a écrit :
> On Mon, Feb 19, 2018 at 12:52 PM, Xavier Roirand <roirand@adacore.com> wrote:
>>
>> The problem is that GDB prints the S parameter in the pck.call_me Ada
>> function using the current language, so the C one, because the program
>> is stopped in a C function, whereas it should use the pck.call_me frame
>> one. This behavior is ok when user manually changes the language but it's
>> not the right one when language is auto.
>
> Agreed, GDB should use per-frame language instead of global current
> language. However, instead of switching global variable current_language,
> why don't we pass the per-frame language down to la_val_print? In
> ada-valprint.c:ada_val_print,
>
> ada_val_print_1 (type, embedded_offset, address,
> stream, recurse, val, options,
> current_language);
> ^^^^^^^^^^^^^^^^^
>
> why don't pass language for ada here?
>
Hello Yao,
It was my first idea, but it would imply a consequent amount of work
because when ada_val_print_1 is called, the stack is the following one:
#0 ada_val_print_1
#1 ada_val_print
#2 val_print (got the language of the frame arg)
So if we want to pass the per-frame language down to ada_val_print_1
then in the generic val_print function, we have to change:
language->la_val_print (type, embedded_offset, address,
stream, recurse, val,
&local_opts);
to something like:
language->la_val_print (type, embedded_offset, address,
stream, recurse, val,
&local_opts, language);
^^^^^^^^
Fix me if I'm wrong but it means that <language>_val_print functions for
all the supported GDB languages have to be modified in order to support
the language parameter.
Regards.
On Tue, Feb 20, 2018 at 4:28 PM, Xavier Roirand <roirand@adacore.com> wrote:
>
> It was my first idea, but it would imply a consequent amount of work because
> when ada_val_print_1 is called, the stack is the following one:
>
> #0 ada_val_print_1
> #1 ada_val_print
> #2 val_print (got the language of the frame arg)
>
> So if we want to pass the per-frame language down to ada_val_print_1 then in
> the generic val_print function, we have to change:
>
> language->la_val_print (type, embedded_offset, address,
> stream, recurse, val,
> &local_opts);
> to something like:
>
> language->la_val_print (type, embedded_offset, address,
> stream, recurse, val,
> &local_opts, language);
> ^^^^^^^^
>
We don't have to do that. ada_val_print_1 is called in ada_val_print,
ada_val_print_1 (type, embedded_offset, address,
stream, recurse, val, options,
current_language);
and ada_val_print is language->la_val_print in effect, so "language"
is ada_language_defn, and we can rewrite the code above like this,
ada_val_print_1 (type, embedded_offset, address,
stream, recurse, val, options,
&ada_language_defn);
This change may break existing behaviour. Printing for ada (or other
languages) shouldn't depend on current_language, IMO.
> Fix me if I'm wrong but it means that <language>_val_print functions for all
> the supported GDB languages have to be modified in order to support the
> language parameter.
>
Hello,
This is a doable fix, I don't see any issue with it and hope that it
will not add any side effect. I'll propose an RFA soon. Thx
Regards
Le 2/20/18 à 5:48 PM, Yao Qi a écrit :
> On Tue, Feb 20, 2018 at 4:28 PM, Xavier Roirand <roirand@adacore.com> wrote:
>>
>> It was my first idea, but it would imply a consequent amount of work because
>> when ada_val_print_1 is called, the stack is the following one:
>>
>> #0 ada_val_print_1
>> #1 ada_val_print
>> #2 val_print (got the language of the frame arg)
>>
>> So if we want to pass the per-frame language down to ada_val_print_1 then in
>> the generic val_print function, we have to change:
>>
>> language->la_val_print (type, embedded_offset, address,
>> stream, recurse, val,
>> &local_opts);
>> to something like:
>>
>> language->la_val_print (type, embedded_offset, address,
>> stream, recurse, val,
>> &local_opts, language);
>> ^^^^^^^^
>>
>
> We don't have to do that. ada_val_print_1 is called in ada_val_print,
>
> ada_val_print_1 (type, embedded_offset, address,
> stream, recurse, val, options,
> current_language);
>
> and ada_val_print is language->la_val_print in effect, so "language"
> is ada_language_defn, and we can rewrite the code above like this,
>
> ada_val_print_1 (type, embedded_offset, address,
> stream, recurse, val, options,
> &ada_language_defn);
>
> This change may break existing behaviour. Printing for ada (or other
> languages) shouldn't depend on current_language, IMO.
>
>> Fix me if I'm wrong but it means that <language>_val_print functions for all
>> the supported GDB languages have to be modified in order to support the
>> language parameter.
>>
>
Hi Yao,
> We don't have to do that. ada_val_print_1 is called in ada_val_print,
>
> ada_val_print_1 (type, embedded_offset, address,
> stream, recurse, val, options,
> current_language);
>
> and ada_val_print is language->la_val_print in effect, so "language"
> is ada_language_defn, and we can rewrite the code above like this,
>
> ada_val_print_1 (type, embedded_offset, address,
> stream, recurse, val, options,
> &ada_language_defn);
>
> This change may break existing behaviour. Printing for ada (or other
> languages) shouldn't depend on current_language, IMO.
Thanks for the extra pair of eyes, and the pertinent suggestions!
FWIW, I agree with you, your fix is much better.
@@ -268,6 +268,7 @@ print_frame_arg (const struct frame_arg *arg)
{
const struct language_defn *language;
struct value_print_options opts;
+ const struct language_defn *saved_current_language = NULL;
/* Avoid value_print because it will deref ref parameters. We
just want to print their addresses. Print ??? for args whose
@@ -277,10 +278,24 @@ print_frame_arg (const struct frame_arg *arg)
annotate_arg_value (value_type (arg->val));
- /* Use the appropriate language to display our symbol, unless the
- user forced the language to a specific language. */
+ /* Use the appropriate language to display our symbol,
+ unless the user forced the language to a specific
+ language.
+ There's a problem when using language_mode_auto since
+ some language specific functions do not print properly
+ the frame argument because they do not have/use the
+ LANGUAGE parameter given to common_val_print but use
+ global current_language variable instead.
+ To solve this, we switch the current language to the
+ language of the frame argument for each argument then
+ we finally restore the current language. */
+
if (language_mode == language_mode_auto)
- language = language_def (SYMBOL_LANGUAGE (arg->sym));
+ {
+ language = language_def (SYMBOL_LANGUAGE (arg->sym));
+ saved_current_language = current_language;
+ current_language = language;
+ }
else
language = current_language;
@@ -292,6 +307,9 @@ print_frame_arg (const struct frame_arg *arg)
opts.summary = !strcmp (print_frame_arguments, "scalars");
common_val_print (arg->val, &stb, 2, &opts, language);
+
+ if (saved_current_language != NULL)
+ current_language = saved_current_language;
}
CATCH (except, RETURN_MASK_ERROR)
{
new file mode 100644
@@ -0,0 +1,74 @@
+# Copyright 2018 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 bla
+set cfile "${testdir}/foo"
+set csrcfile ${srcdir}/${subdir}/${cfile}.c
+set cobject [standard_output_file ${cfile}.o]
+
+gdb_compile "${csrcfile}" "${cobject}" object [list debug]
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug additional_flags=-largs additional_flags=${cobject} additional_flags=-margs]] != "" } {
+ return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/foo.c]
+runto "foo.c:$bp_location"
+
+gdb_test_no_output "set print frame-arguments all"
+
+# Here is the scenario:
+# - Once stopped in a C function, with language_mode set to auto, print
+# backtrace, we should see the Ada frame arguments printed using Ada
+# syntax.
+# - Set language to C, then check that printing backtrace shows the Ada
+# frame arguments using C syntax.
+# - Set language back to auto, check language mode value, then print
+# backtrace, we should see Ada frame arguments printed using Ada C
+# syntax.
+
+gdb_test "show lang" \
+ "The current source language is \"auto; currently c\"." \
+ "show language #1"
+
+gdb_test "bt" \
+ "#1 $hex in pck\\.call_me \\(s=\"test\"\\).*" \
+ "backtrace (auto: c)"
+
+gdb_test_no_output "set language c" \
+ "Set current source language to \"manual; currently c\"."
+
+gdb_test "show lang" \
+ "The current source language is \"c\"." \
+ "show language #2"
+
+gdb_test "bt" \
+ "#1 $hex in pck\\.call_me \\(s={P_ARRAY = $hex, P_BOUNDS = $hex}\\).*" \
+ "backtrace (manual: c)"
+
+gdb_test_no_output "set language auto" \
+ "Set current source language to \"auto; currently c\"."
+
+gdb_test "show lang" \
+ "The current source language is \"auto; currently c\"." \
+ "show language #3"
+
+gdb_test "bt" \
+ "#1 $hex in pck\\.call_me \\(s=\"test\"\\).*" \
+ "backtrace (auto: ada)"
+
new file mode 100644
@@ -0,0 +1,22 @@
+-- Copyright 2018 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;
+
+procedure Bla is
+ S : String := "test";
+begin
+ Call_Me (S);
+end Bla;
new file mode 100644
@@ -0,0 +1,22 @@
+/* Copyright 2018 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/>. */
+
+int
+break_me (void)
+{
+ return 0; /* STOP */
+}
new file mode 100644
@@ -0,0 +1,24 @@
+-- Copyright 2018 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 C_Break_Me;
+ pragma Import (C, C_Break_Me, "break_me");
+
+ procedure Call_Me (S: in out String) is
+ begin
+ C_Break_Me;
+ end Call_Me;
+end Pck;
new file mode 100644
@@ -0,0 +1,18 @@
+-- Copyright 2018 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 Call_Me (S: in out String);
+end Pck;