[RFA] inadvertent language switch during breakpoint_re_set_one
Commit Message
Hello,
Trying to insert a breakpoint using *FUNC'address with an Ada program
and then running the program until reaching that breakpoint currently
yields the following behavior:
(gdb) break *a'address
Breakpoint 1 at 0x40240c: file a.adb, line 1.
(gdb) run
[1] + 27222 suspended (tty output) /[...]/gdb -q simple_main
Unsuspending GDB then shows it was suspended trying to report
the following error:
Starting program: /home/takamaka.a/brobecke/ex/simple/a
Error in re-setting breakpoint 1: Unmatched single quote.
Error in re-setting breakpoint 1: Unmatched single quote.
Error in re-setting breakpoint 1: Unmatched single quote.
[Inferior 1 (process 32470) exited normally]
The "a'address" is Ada speak for function A's address ("A" by
itself means the result of calling A with no arguments). The
transcript above shows that we're having problems trying to
parse the breakpoint location while re-setting it. As a result,
we also fail to stop at the breakpoint.
Normally, breakpoint locations are evaluated with the current_language
being set to the language of the breakpoint. But, unfortunately for us,
what happened in this case is that parse_exp_in_context_1 calls
get_selected_block which eventually leads to a call to select_frame
because the current_frame hadn't been set yet. select_frame then
finds that our language_mode is auto, and therefore changes the
current_language to match the language of the frame we just selected.
In our case, the language chosen was 'c', which of course is not
able to parse an Ada expression, hence the error.
This patch prevents this by forcing the language_mode to manual
before we call breakpoint_re_set_one. For that, we introduce
a new class to restore the language mode upon exit.
gdb/ChangeLog:
* language.h (scoped_restore_current_language_mode): New class.
* breakpoint.c (breakpoint_re_set): Temporarily force language_mode
to language_mode_manual while calling breakpoint_re_set_one.
gdb/testsuite/ChangeLog:
* gdb.ada/bp_fun_addr: New testcase.
Tested on x86_64-linux.
OK to push?
Thanks,
Comments
On Thursday, May 10 2018, Joel Brobecker wrote:
> Hello,
Hey Joel,
[...]
> diff --git a/gdb/language.h b/gdb/language.h
> index 029de4a..3e2b90a 100644
> --- a/gdb/language.h
> +++ b/gdb/language.h
> @@ -705,4 +705,30 @@ private:
> enum language m_lang;
> };
>
> +/* Save the current language mode and restore it upon destruction. */
> +
> +class scoped_restore_current_language_mode
> +{
> +public:
> +
> + explicit scoped_restore_current_language_mode ()
> + : m_lang_mode (language_mode)
> + {
> + }
> +
> + ~scoped_restore_current_language_mode ()
> + {
> + language_mode = m_lang_mode;
> + }
> +
> + scoped_restore_current_language_mode
> + (const scoped_restore_current_language_mode &) = delete;
> + scoped_restore_current_language_mode &operator=
> + (const scoped_restore_current_language_mode &) = delete;
I think you can use DISABLE_COPY_AND_ASSIGN here.
Cheers,
Hi, Joel,
Just a quick question:
On 05/10/2018 11:19 AM, Joel Brobecker wrote:
> gdb/testsuite/ChangeLog:
>
> * gdb.ada/bp_fun_addr: New testcase.
Is this test supposed to trigger the failure? It passes for me:
$ make check TESTS=gdb.ada/bp_fun_addr.exp
[snip]
=== gdb tests ===
Schedule of variations:
unix
Running target unix
Using /usr/share/dejagnu/baseboards/unix.exp as board description file for target.
Using /usr/share/dejagnu/config/unix.exp as generic interface file for target.
Using /home/keiths/work/gdb/branches/virgin/linux/gdb/testsuite/../../../src/gdb/testsuite/config/unix.exp as tool-and-target-specific interface file.
Running /home/keiths/work/gdb/branches/virgin/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.ada/bp_fun_addr.exp ...
=== gdb Summary ===
# of expected passes 3
Is my environment different from yours, i.e., is this something that is exposed on your internal tree?
Keith
> === gdb Summary ===
>
> # of expected passes 3
>
> Is my environment different from yours, i.e., is this something that
> is exposed on your internal tree?
That's really odd. I'm on top of branch master, and without this patch,
I do get an error:
| FAIL: gdb.ada/bp_fun_addr.exp: Run until breakpoint at a'address (the program exited)
|
| === gdb Summary ===
|
| # of expected passes 2
| # of unexpected failures 1
You wouldn't have rebuilt GDB with my patch without realizing it,
by any chance?
Other than that, the only difference I can think of is that I am
using GNAT Pro, rather than the GNU Ada compiler... I can send you
my executable, if it can help clarify the mystery.
> > + scoped_restore_current_language_mode
> > + (const scoped_restore_current_language_mode &) = delete;
> > + scoped_restore_current_language_mode &operator=
> > + (const scoped_restore_current_language_mode &) = delete;
>
> I think you can use DISABLE_COPY_AND_ASSIGN here.
Thanks for the tip, Sergio.
If the patch is accepted, I'll adjust to include a first patch which
ajusts class scoped_restore_current_language to use DISABLE_COPY_AND_ASSIGN,
and then adjust my patch on top of it to do the same.
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
Joel> + explicit scoped_restore_current_language_mode ()
Joel> + : m_lang_mode (language_mode)
Joel> + {
Joel> + }
Joel> +
Joel> + ~scoped_restore_current_language_mode ()
Joel> + {
Joel> + language_mode = m_lang_mode;
Joel> + }
[...]
Joel> + enum language_mode m_lang_mode;
For a plain scalar you can just use scoped_restore and make_scoped_restore.
Tom
On 05/10/2018 12:08 PM, Joel Brobecker wrote:
>> === gdb Summary ===
>>
>> # of expected passes 3
>>
>> Is my environment different from yours, i.e., is this something that
>> is exposed on your internal tree?
>
> That's really odd. I'm on top of branch master, and without this patch,
> I do get an error:
>
> | FAIL: gdb.ada/bp_fun_addr.exp: Run until breakpoint at a'address (the program exited)
> |
> | === gdb Summary ===
> |
> | # of expected passes 2
> | # of unexpected failures 1
>
> You wouldn't have rebuilt GDB with my patch without realizing it,
> by any chance?
Nope. All clean except for the test.
> Other than that, the only difference I can think of is that I am
> using GNAT Pro, rather than the GNU Ada compiler... I can send you
> my executable, if it can help clarify the mystery.
Yeah, send it my way, please. I'm just curious why this isn't working the way I thought it should. [I have no objections with your patch otherwise.]
Keith
@@ -13881,6 +13881,19 @@ breakpoint_re_set (void)
scoped_restore save_input_radix = make_scoped_restore (&input_radix);
scoped_restore_current_pspace_and_thread restore_pspace_thread;
+ /* breakpoint_re_set_one sets the current_language to the language
+ of the breakpoint it is resetting (see prepare_re_set_context)
+ before re-evaluating the breakpoint's location. This change can
+ unfortunately get undone by accident if the language_mode is set
+ to auto, and we either switch frames, or more likely in this context,
+ we select the current frame.
+
+ We prevent this by temporarily turning the language_mode to
+ language_mode_manual. We we restore it once all breakpoints
+ have been reset. */
+ scoped_restore_current_language_mode save_language_mode;
+ language_mode = language_mode_manual;
+
/* Note: we must not try to insert locations until after all
breakpoints have been re-set. Otherwise, e.g., when re-setting
breakpoint 1, we'd insert the locations of breakpoint 2, which
@@ -705,4 +705,30 @@ private:
enum language m_lang;
};
+/* Save the current language mode and restore it upon destruction. */
+
+class scoped_restore_current_language_mode
+{
+public:
+
+ explicit scoped_restore_current_language_mode ()
+ : m_lang_mode (language_mode)
+ {
+ }
+
+ ~scoped_restore_current_language_mode ()
+ {
+ language_mode = m_lang_mode;
+ }
+
+ scoped_restore_current_language_mode
+ (const scoped_restore_current_language_mode &) = delete;
+ scoped_restore_current_language_mode &operator=
+ (const scoped_restore_current_language_mode &) = delete;
+
+private:
+
+ enum language_mode m_lang_mode;
+};
+
#endif /* defined (LANGUAGE_H) */
new file mode 100644
@@ -0,0 +1,35 @@
+# Copyright 2016-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"
+
+if { [skip_ada_tests] } { return -1 }
+
+standard_ada_testfile a
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable {debug}] != ""} {
+ return -1
+}
+
+clean_restart ${testfile}
+
+gdb_test "break *a'address" \
+ "Breakpoint \[0-9\]+ at.*: file .*a.adb, line \[0-9\]+."
+
+gdb_run_cmd
+gdb_test "" \
+ "Breakpoint $decimal, a \\(\\).*" \
+ "Run until breakpoint at a'address"
+
new file mode 100644
@@ -0,0 +1,19 @@
+-- Copyright 2016-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/>.
+
+procedure A is
+begin
+ null;
+end A;