From: Joel Brobecker Date: Fri, 3 Mar 2017 16:31:13 +0000 (+0100) Subject: local variable watchpoint not deleted after leaving scope X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=286acbb5c25c5895832d3dd3be3398d3f2d64fad;p=binutils-gdb.git local variable watchpoint not deleted after leaving scope When debugging an Ada program, and inserting a watchpoint tracking a local variable, the watchpoint doesn't get automatically deleted upon leaving that variable's scope. This watchpoint then starts creating problems later on, when trying to resume the program's execution from a location outside of the watchpoint's scope: (gdb) c Continuing. Breakpoint 2, foo_p708_025 () at foo_p708_025.adb:7 7 Do_Nothing (Val); (gdb) n No frame is currently executing in block pck.get_val. Command aborted. (gdb) c Continuing. No frame is currently executing in block pck.get_val. Command aborted. The expected output is the following: - The program's execution after the first continue should stop as soon as we reach the end of the watchpoint's scope, and the debugger should be deleting it. - Then we can continue until reaching breakpoint 2 above; - After which we should be able to do next/continue as usual. The reason the watchpoint is not automatically deleted at scope exit is because the watchpoint is not marked as being scope-specific (b->exp_valid_block is equal NULL), and this is because the symbol lookup for our local variable failed to set the innermost_block global variable during the lookup. More precisely, if we look at watch_command_1, we do the following: innermost_block = NULL; [...] exp = parse_exp_1 (&arg, 0, 0, 0); [...] exp_valid_block = innermost_block; Currently, innermost_block stays NULL after the call to parse_exp_1. Digging further, this innermost_block is typically set during symbol lookup when the symbol is considered to have a frame-relative address. For instance, in c-exp.y, we see some code like the following: if (symbol_read_needs_frame (sym.symbol)) { if (innermost_block == 0 || contained_in (sym.block, innermost_block)) innermost_block = sym.block; } We actually have the exact same mechanism in ada-exp.y, except that it vhas accidently been turned off. See write_var_from_sym, where we start with: if (orig_left_context == NULL && symbol_read_needs_frame (sym)) { if (innermost_block == 0 || contained_in (block, innermost_block)) innermost_block = block; } In this case, orig_left_context is a parameter, and looking at the point of call in write_var_or_type, we see: if (nsyms == 1) { write_var_from_sym (par_state, block, syms[0].block, syms[0].symbol); In the call above, the paramater we are interested in is "block", which is a parameter for write_var_or_type as well, except we explicitly override its value at the beginning when found to be NULL: if (block == NULL) block = expression_context_block; So the block we pass to write_var_from_sym is not NULL, and we therefore don't set innermost_block, which leads to the watchpoint no longer being marked as scope-specific. The handling of orig_left_context in write_var_from_sym was there to handle the case where a user writes an expression where the symbol is qualified with a scope (Eg: "function::variable"). But it appears that handling this is specifically here is no longer necessary, so this patch simply removes that parameter and the associated check, and then updates all the points of calls. Interestingly, this also affects GDB/MI, and in particular varobjs, because local variables are now properly reported as having a block, which causes the associated varob to have a "thread-id" field. This patch also adjusts a couple of Ada/gdb-mi tests. gdb/ChangeLog: * ada-exp.y (write_var_from_sym): Remove parameter "orig_left_context". Update all callers. gdb/testsuite/ChangeLog: * gdb.ada/scoped_watch: New testcase. * gdb.ada/watch_arg.exp: Adjust expected behavior to the behavior which is actually correct. * gdb.ada/mi_interface.exp: Add missing thread-id in expected varobj. * gdb.ada/mi_var_array.exp: Add missing thread-id in expected varobj. --- diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 2be36de6bbe..cc38854e442 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2017-11-08 Joel Brobecker + + * ada-exp.y (write_var_from_sym): Remove parameter + "orig_left_context". Update all callers. + 2017-11-08 Simon Marchi * tracepoint.h (class collection_list) : Return diff --git a/gdb/ada-exp.y b/gdb/ada-exp.y index 3014a311075..004d58b00e0 100644 --- a/gdb/ada-exp.y +++ b/gdb/ada-exp.y @@ -749,14 +749,14 @@ yyerror (const char *msg) } /* Emit expression to access an instance of SYM, in block BLOCK (if - * non-NULL), and with :: qualification ORIG_LEFT_CONTEXT. */ + non-NULL). */ + static void write_var_from_sym (struct parser_state *par_state, - const struct block *orig_left_context, const struct block *block, struct symbol *sym) { - if (orig_left_context == NULL && symbol_read_needs_frame (sym)) + if (symbol_read_needs_frame (sym)) { if (innermost_block == 0 || contained_in (block, innermost_block)) @@ -837,8 +837,7 @@ write_object_renaming (struct parser_state *par_state, &inner_renaming_expr)) { case ADA_NOT_RENAMING: - write_var_from_sym (par_state, orig_left_context, sym_info.block, - sym_info.symbol); + write_var_from_sym (par_state, sym_info.block, sym_info.symbol); break; case ADA_OBJECT_RENAMING: write_object_renaming (par_state, sym_info.block, @@ -899,7 +898,7 @@ write_object_renaming (struct parser_state *par_state, else if (SYMBOL_CLASS (index_sym_info.symbol) == LOC_TYPEDEF) /* Index is an old-style renaming symbol. */ index_sym_info.block = orig_left_context; - write_var_from_sym (par_state, NULL, index_sym_info.block, + write_var_from_sym (par_state, index_sym_info.block, index_sym_info.symbol); } if (slice_state == SIMPLE_INDEX) @@ -1310,8 +1309,7 @@ write_var_or_type (struct parser_state *par_state, if (nsyms == 1) { - write_var_from_sym (par_state, block, syms[0].block, - syms[0].symbol); + write_var_from_sym (par_state, syms[0].block, syms[0].symbol); write_selectors (par_state, encoded_name + tail_index); return NULL; } @@ -1384,7 +1382,7 @@ write_name_assoc (struct parser_state *par_state, struct stoken name) if (nsyms != 1 || SYMBOL_CLASS (syms[0].symbol) == LOC_TYPEDEF) write_exp_op_with_string (par_state, OP_NAME, name); else - write_var_from_sym (par_state, NULL, syms[0].block, syms[0].symbol); + write_var_from_sym (par_state, syms[0].block, syms[0].symbol); } else if (write_var_or_type (par_state, NULL, name) != NULL) diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 63492599177..15c0caf8532 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,11 @@ +2017-11-08 Joel Brobecker + + * gdb.ada/scoped_watch: New testcase. + * gdb.ada/watch_arg.exp: Adjust expected behavior to the behavior + which is actually correct. + * gdb.ada/mi_interface.exp: Add missing thread-id in expected varobj. + * gdb.ada/mi_var_array.exp: Add missing thread-id in expected varobj. + 2017-11-08 Pedro Alves * gdb.gdb/complaints.exp (test_initial_complaints) diff --git a/gdb/testsuite/gdb.ada/mi_interface.exp b/gdb/testsuite/gdb.ada/mi_interface.exp index 65d516a9cac..ecbe2f3541f 100644 --- a/gdb/testsuite/gdb.ada/mi_interface.exp +++ b/gdb/testsuite/gdb.ada/mi_interface.exp @@ -44,9 +44,9 @@ mi_continue_to_line \ "stop at start of main Ada procedure" mi_gdb_test "-var-create ggg1 * ggg1" \ - "\\^done,name=\"ggg1\",numchild=\"1\",value=\"{...}\",type=\" pck.gadatatype\",has_more=\"0\"" \ + "\\^done,name=\"ggg1\",numchild=\"1\",value=\"{...}\",type=\" pck.gadatatype\",thread-id=\"$decimal\",has_more=\"0\"" \ "create ggg1 varobj" mi_gdb_test "-var-list-children 1 ggg1" \ - "\\^done,numchild=\"1\",children=\\\[child={name=\"ggg1.i\",exp=\"i\",numchild=\"0\",value=\"42\",type=\"integer\"}\\\],has_more=\"0\"" \ + "\\^done,numchild=\"1\",children=\\\[child={name=\"ggg1.i\",exp=\"i\",numchild=\"0\",value=\"42\",type=\"integer\",thread-id=\"$decimal\"}\\\],has_more=\"0\"" \ "list ggg1's children" diff --git a/gdb/testsuite/gdb.ada/mi_var_array.exp b/gdb/testsuite/gdb.ada/mi_var_array.exp index aab9109766d..056ee884e81 100644 --- a/gdb/testsuite/gdb.ada/mi_var_array.exp +++ b/gdb/testsuite/gdb.ada/mi_var_array.exp @@ -48,5 +48,5 @@ mi_gdb_test "-var-create vta * vta" \ "create bt varobj" mi_gdb_test "-var-list-children vta" \ - "\\^done,numchild=\"2\",children=\\\[child={name=\"vta.n\",exp=\"n\",numchild=\"0\",type=\"bar\\.int\"},child={name=\"vta.f\",exp=\"f\",numchild=\"0\",type=\"array \\(1 .. n\\) of character\"}\\\],.*" \ + "\\^done,numchild=\"2\",children=\\\[child={name=\"vta.n\",exp=\"n\",numchild=\"0\",type=\"bar\\.int\",thread-id=\"$decimal\"},child={name=\"vta.f\",exp=\"f\",numchild=\"0\",type=\"array \\(1 .. n\\) of character\",thread-id=\"$decimal\"}\\\],.*" \ "list vta's children" diff --git a/gdb/testsuite/gdb.ada/scoped_watch.exp b/gdb/testsuite/gdb.ada/scoped_watch.exp new file mode 100644 index 00000000000..51f6c41fdab --- /dev/null +++ b/gdb/testsuite/gdb.ada/scoped_watch.exp @@ -0,0 +1,83 @@ +# Copyright 2017 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 . + +load_lib "ada.exp" + +if { [skip_ada_tests] } { return -1 } + +standard_ada_testfile foo_p708_025 + +if {[gdb_compile_ada "${srcfile}" "${binfile}" executable {debug}] != ""} { + return -1 +} + +clean_restart ${testfile} + +set bp_location [gdb_get_line_number "START" ${testdir}/pck.adb] +runto "pck.adb:$bp_location" + +# Insert a watchpoint on local variable "result" + +gdb_test "watch result" \ + ".*atchpoint \[0-9\]+: result" + +# Insert a breakpoint we'll reach after returning from the current +# function. + +set bp_location [gdb_get_line_number "Do_Nothing" ${testdir}/foo_p708_025.adb] +gdb_test "break foo_p708_025.adb:$bp_location" \ + "Breakpoint \[0-9\]+ at.*: file .*foo_p708_025.adb, line \[0-9\]+." + +# This breakpoint will be there to stop us after we test what happens +# during a continue (see below...) + +gdb_test "break pck.increment" \ + "Breakpoint \[0-9\]+ at.*: file .*pck.adb, line \[0-9\]+." + +# Continue until we reach our watchpoint. It isn't strictly necessary +# for our purpose that the watchpoint actually triggers, but this shows +# that the watchpoint exists and is active. +gdb_test "cont" \ + ".*atchpoint \[0-9\]+: result.*Old value = 8.*New value = 64.*" \ + "continuing to watchpoint hit" + +# Continue again. We should be stopped at the (internal) breakpoint +# that we setup to delete the watchpoint as soon as the program leaves +# the current scope. + +gdb_test \ + "cont" \ + ".*atchpoint \[0-9\]+ deleted because the program has left the block.*" \ + "continuing until watchpoint automatic deletion" + +# Continue one more time. We should be reaching one of the breakpoints +# (on the call to Do_Nothing) we set earlier. + +gdb_test "cont" \ + "Breakpoint \[0-9\]+.*Do_Nothing.*" \ + "continuing to breakpoint on call to Do_Nothing" + +# Do a next, to verify that it works... + +gdb_test "next" \ + ".* Call_Me;" \ + "next to call to Call_Me" + +# And finally, one more continue. + + +gdb_test "cont" \ + "Breakpoint \[0-9\]+.*pck\\.increment.*" \ + "continuing to breakpoint in pck.increment" diff --git a/gdb/testsuite/gdb.ada/scoped_watch/foo_p708_025.adb b/gdb/testsuite/gdb.ada/scoped_watch/foo_p708_025.adb new file mode 100644 index 00000000000..d7d1cdfd47e --- /dev/null +++ b/gdb/testsuite/gdb.ada/scoped_watch/foo_p708_025.adb @@ -0,0 +1,25 @@ +-- Copyright 2017 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 . + +with Pck; use Pck; + +procedure Foo_P708_025 is + Val : Integer; +begin + Val := Get_Val (8, False); + Do_Nothing (Val); + Call_Me; + Increment (Val); +end Foo_P708_025; diff --git a/gdb/testsuite/gdb.ada/scoped_watch/pck.adb b/gdb/testsuite/gdb.ada/scoped_watch/pck.adb new file mode 100644 index 00000000000..075bc618c99 --- /dev/null +++ b/gdb/testsuite/gdb.ada/scoped_watch/pck.adb @@ -0,0 +1,44 @@ +-- Copyright 2017 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 . + +package body Pck is + function Get_Val (Seed: Integer; Off_By_One: Boolean) return Integer is + Result : Integer := Seed; + begin + if Off_By_One then -- START + Result := Result - 1; + end if; + Result := Result * 8; + if Off_By_One then + Result := Result + 1; + end if; + return Result; + end Get_Val; + + procedure Do_Nothing (Val: in out Integer) is + begin + null; + end Do_Nothing; + + procedure Call_Me is + begin + null; + end Call_Me; + + procedure Increment (Val : in out Integer) is + begin + Val := Val + 1; + end Increment; +end Pck; diff --git a/gdb/testsuite/gdb.ada/scoped_watch/pck.ads b/gdb/testsuite/gdb.ada/scoped_watch/pck.ads new file mode 100644 index 00000000000..f58558205cf --- /dev/null +++ b/gdb/testsuite/gdb.ada/scoped_watch/pck.ads @@ -0,0 +1,21 @@ +-- Copyright 2017 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 . + +package Pck is + function Get_Val (Seed: Integer; Off_By_One: Boolean) return Integer; + procedure Do_Nothing (Val: in out Integer); + procedure Call_Me; + procedure Increment (Val : in out Integer); +end Pck; diff --git a/gdb/testsuite/gdb.ada/watch_arg.exp b/gdb/testsuite/gdb.ada/watch_arg.exp index 047ed3415cf..f50bbb95455 100644 --- a/gdb/testsuite/gdb.ada/watch_arg.exp +++ b/gdb/testsuite/gdb.ada/watch_arg.exp @@ -40,8 +40,17 @@ gdb_test "break watch.adb:$bp_location" \ "Breakpoint \[0-9\]+ at.*: file .*watch.adb, line \[0-9\]+." \ "insert second breakpoint in watch.adb" -# Then continue to that breakpoint, and verify that the watchpoint -# did not interfere with that. +# Continue again. We should be stopped at the (internal) breakpoint +# that we setup to delete the watchpoint as soon as the program leaves +# the watchpoint scope. + +gdb_test \ + "cont" \ + ".*atchpoint \[0-9\]+ deleted because the program has left the block.*" \ + "continuing until watchpoint automatic deletion" + +# Continue one more time, and verify that we land at the second breakpoint +# we inserted. gdb_test "cont" \ "Breakpoint \[0-9\]+, watch \\(\\).*" \