public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [RFA] inadvertent language switch during breakpoint_re_set_one
@ 2018-05-10 18:42 Joel Brobecker
  2018-05-10 18:58 ` Sergio Durigan Junior
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Joel Brobecker @ 2018-05-10 18:42 UTC (permalink / raw)
  To: gdb-patches

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,
-- 
Joel

---
 gdb/breakpoint.c                        | 13 ++++++++++++
 gdb/language.h                          | 26 ++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_fun_addr.exp   | 35 +++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_fun_addr/a.adb | 19 ++++++++++++++++++
 4 files changed, 93 insertions(+)
 create mode 100644 gdb/testsuite/gdb.ada/bp_fun_addr.exp
 create mode 100644 gdb/testsuite/gdb.ada/bp_fun_addr/a.adb

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d1955ec..503bbe0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -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
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;
+
+private:
+
+  enum language_mode m_lang_mode;
+};
+
 #endif /* defined (LANGUAGE_H) */
diff --git a/gdb/testsuite/gdb.ada/bp_fun_addr.exp b/gdb/testsuite/gdb.ada/bp_fun_addr.exp
new file mode 100644
index 0000000..aa1261b
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_fun_addr.exp
@@ -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"
+
diff --git a/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb b/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
new file mode 100644
index 0000000..00e2e86
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
@@ -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;
-- 
2.1.4

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 18:42 [RFA] inadvertent language switch during breakpoint_re_set_one Joel Brobecker
@ 2018-05-10 18:58 ` Sergio Durigan Junior
  2018-05-10 19:26   ` Joel Brobecker
  2018-05-10 19:08 ` Keith Seitz
  2018-05-10 19:29 ` Tom Tromey
  2 siblings, 1 reply; 11+ messages in thread
From: Sergio Durigan Junior @ 2018-05-10 18:58 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 18:42 [RFA] inadvertent language switch during breakpoint_re_set_one Joel Brobecker
  2018-05-10 18:58 ` Sergio Durigan Junior
@ 2018-05-10 19:08 ` Keith Seitz
  2018-05-10 19:21   ` Joel Brobecker
  2018-05-10 19:29 ` Tom Tromey
  2 siblings, 1 reply; 11+ messages in thread
From: Keith Seitz @ 2018-05-10 19:08 UTC (permalink / raw)
  To: Joel Brobecker, gdb-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 19:08 ` Keith Seitz
@ 2018-05-10 19:21   ` Joel Brobecker
  2018-05-10 19:35     ` Keith Seitz
  0 siblings, 1 reply; 11+ messages in thread
From: Joel Brobecker @ 2018-05-10 19:21 UTC (permalink / raw)
  To: Keith Seitz; +Cc: gdb-patches

> 		=== 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.

-- 
Joel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 18:58 ` Sergio Durigan Junior
@ 2018-05-10 19:26   ` Joel Brobecker
  0 siblings, 0 replies; 11+ messages in thread
From: Joel Brobecker @ 2018-05-10 19:26 UTC (permalink / raw)
  To: Sergio Durigan Junior; +Cc: gdb-patches

> > +  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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 18:42 [RFA] inadvertent language switch during breakpoint_re_set_one Joel Brobecker
  2018-05-10 18:58 ` Sergio Durigan Junior
  2018-05-10 19:08 ` Keith Seitz
@ 2018-05-10 19:29 ` Tom Tromey
  2018-05-10 20:59   ` Joel Brobecker
  2 siblings, 1 reply; 11+ messages in thread
From: Tom Tromey @ 2018-05-10 19:29 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

>>>>> "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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 19:21   ` Joel Brobecker
@ 2018-05-10 19:35     ` Keith Seitz
  0 siblings, 0 replies; 11+ messages in thread
From: Keith Seitz @ 2018-05-10 19:35 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 19:29 ` Tom Tromey
@ 2018-05-10 20:59   ` Joel Brobecker
  2018-05-31  0:02     ` Joel Brobecker
  2018-06-01 12:42     ` Pedro Alves
  0 siblings, 2 replies; 11+ messages in thread
From: Joel Brobecker @ 2018-05-10 20:59 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

> For a plain scalar you can just use scoped_restore and make_scoped_restore.

Nice! Thanks Tom.

New patch attached.

gdb/ChangeLog:

        * 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.

-- 
Joel

[-- Attachment #2: 0001-inadvertent-language-switch-during-breakpoint_re_set.patch --]
[-- Type: text/x-diff, Size: 6119 bytes --]

From 90bb7d41b87c259d09faddae174887defd87cbc9 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 10 May 2018 13:17:52 -0500
Subject: [PATCH] inadvertent language switch during breakpoint_re_set_one

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.

gdb/ChangeLog:

        * 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.
---
 gdb/breakpoint.c                        | 13 ++++++++++++
 gdb/testsuite/gdb.ada/bp_fun_addr.exp   | 35 +++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_fun_addr/a.adb | 19 ++++++++++++++++++
 3 files changed, 67 insertions(+)
 create mode 100644 gdb/testsuite/gdb.ada/bp_fun_addr.exp
 create mode 100644 gdb/testsuite/gdb.ada/bp_fun_addr/a.adb

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d1955ec..6dfa2d9 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -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 save_language_mode = make_scoped_restore (&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
diff --git a/gdb/testsuite/gdb.ada/bp_fun_addr.exp b/gdb/testsuite/gdb.ada/bp_fun_addr.exp
new file mode 100644
index 0000000..aa1261b
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_fun_addr.exp
@@ -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"
+
diff --git a/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb b/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
new file mode 100644
index 0000000..00e2e86
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
@@ -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;
-- 
2.1.4


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 20:59   ` Joel Brobecker
@ 2018-05-31  0:02     ` Joel Brobecker
  2018-06-01 12:42     ` Pedro Alves
  1 sibling, 0 replies; 11+ messages in thread
From: Joel Brobecker @ 2018-05-31  0:02 UTC (permalink / raw)
  To: gdb-patches

> gdb/ChangeLog:
> 
>         * 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.

Friendly 2 weeks ping on this patch :).

Thanks!

> >From 90bb7d41b87c259d09faddae174887defd87cbc9 Mon Sep 17 00:00:00 2001
> From: Joel Brobecker <brobecker@adacore.com>
> Date: Thu, 10 May 2018 13:17:52 -0500
> Subject: [PATCH] inadvertent language switch during breakpoint_re_set_one
> 
> 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.
> 
> gdb/ChangeLog:
> 
>         * 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.
> ---
>  gdb/breakpoint.c                        | 13 ++++++++++++
>  gdb/testsuite/gdb.ada/bp_fun_addr.exp   | 35 +++++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.ada/bp_fun_addr/a.adb | 19 ++++++++++++++++++
>  3 files changed, 67 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.ada/bp_fun_addr.exp
>  create mode 100644 gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index d1955ec..6dfa2d9 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -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 save_language_mode = make_scoped_restore (&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
> diff --git a/gdb/testsuite/gdb.ada/bp_fun_addr.exp b/gdb/testsuite/gdb.ada/bp_fun_addr.exp
> new file mode 100644
> index 0000000..aa1261b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ada/bp_fun_addr.exp
> @@ -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"
> +
> diff --git a/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb b/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
> new file mode 100644
> index 0000000..00e2e86
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
> @@ -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;
> -- 
> 2.1.4
> 


-- 
Joel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-05-10 20:59   ` Joel Brobecker
  2018-05-31  0:02     ` Joel Brobecker
@ 2018-06-01 12:42     ` Pedro Alves
  2018-06-01 16:39       ` Joel Brobecker
  1 sibling, 1 reply; 11+ messages in thread
From: Pedro Alves @ 2018-06-01 12:42 UTC (permalink / raw)
  To: Joel Brobecker, Tom Tromey; +Cc: gdb-patches

LGTM.  Could nits below.

On 05/10/2018 09:40 PM, Joel Brobecker wrote:

> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -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

Double "We we".

> +gdb_run_cmd
> +gdb_test "" \
> +         "Breakpoint $decimal, a \\(\\).*" \
> +         "Run until breakpoint at a'address"

Lowercase "run".

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFA] inadvertent language switch during breakpoint_re_set_one
  2018-06-01 12:42     ` Pedro Alves
@ 2018-06-01 16:39       ` Joel Brobecker
  0 siblings, 0 replies; 11+ messages in thread
From: Joel Brobecker @ 2018-06-01 16:39 UTC (permalink / raw)
  To: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

> > --- a/gdb/breakpoint.c
> > +++ b/gdb/breakpoint.c
> > @@ -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
> 
> Double "We we".
> 
> > +gdb_run_cmd
> > +gdb_test "" \
> > +         "Breakpoint $decimal, a \\(\\).*" \
> > +         "Run until breakpoint at a'address"
> 
> Lowercase "run".

Thanks, Pedro. Attached is the commit I just pushed (to master).

-- 
Joel

[-- Attachment #2: 0001-inadvertent-language-switch-during-breakpoint_re_set.patch --]
[-- Type: text/x-diff, Size: 7046 bytes --]

From 8e817061976910fd1ac7bb8f689dbd96123ea593 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Fri, 1 Jun 2018 09:36:05 -0700
Subject: [PATCH] inadvertent language switch during breakpoint_re_set_one

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.

gdb/ChangeLog:

        * 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.
---
 gdb/ChangeLog                           |  5 +++++
 gdb/breakpoint.c                        | 13 ++++++++++++
 gdb/testsuite/ChangeLog                 |  4 ++++
 gdb/testsuite/gdb.ada/bp_fun_addr.exp   | 35 +++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_fun_addr/a.adb | 19 ++++++++++++++++++
 5 files changed, 76 insertions(+)
 create mode 100644 gdb/testsuite/gdb.ada/bp_fun_addr.exp
 create mode 100644 gdb/testsuite/gdb.ada/bp_fun_addr/a.adb

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 01f4c009b2..082a435de9 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-06-01  Joel Brobecker  <brobecker@adacore.com>
+
+	* breakpoint.c (breakpoint_re_set): Temporarily force language_mode
+	to language_mode_manual while calling breakpoint_re_set_one.
+
 2018-06-01  Tom Tromey  <tom@tromey.com>
 
 	* valops.c (value_cast_structs, destructor_name_p): Update.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f3101af96f..00c538e989 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13868,6 +13868,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 restore it once all breakpoints
+       have been reset.  */
+    scoped_restore save_language_mode = make_scoped_restore (&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
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 9fb10b5a23..8cef172a26 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2018-06-01  Joel Brobecker  <brobecker@adacore.com>
+
+	* gdb.ada/bp_fun_addr: New testcase.
+
 2018-06-01  Tom Tromey  <tom@tromey.com>
 
 	* gdb.xml/tdesc-regs.exp (load_description): Update expected
diff --git a/gdb/testsuite/gdb.ada/bp_fun_addr.exp b/gdb/testsuite/gdb.ada/bp_fun_addr.exp
new file mode 100644
index 0000000000..b037d43538
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_fun_addr.exp
@@ -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"
+
diff --git a/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb b/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
new file mode 100644
index 0000000000..00e2e86355
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_fun_addr/a.adb
@@ -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;
-- 
2.11.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-06-01 16:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-10 18:42 [RFA] inadvertent language switch during breakpoint_re_set_one Joel Brobecker
2018-05-10 18:58 ` Sergio Durigan Junior
2018-05-10 19:26   ` Joel Brobecker
2018-05-10 19:08 ` Keith Seitz
2018-05-10 19:21   ` Joel Brobecker
2018-05-10 19:35     ` Keith Seitz
2018-05-10 19:29 ` Tom Tromey
2018-05-10 20:59   ` Joel Brobecker
2018-05-31  0:02     ` Joel Brobecker
2018-06-01 12:42     ` Pedro Alves
2018-06-01 16:39       ` Joel Brobecker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).