diff mbox

Revert: [obv] Code cleanup: Move print_command_1 expr variable scope

Message ID 20150327192231.GA21247@host1.jankratochvil.net
State New, archived
Headers

Commit Message

Jan Kratochvil March 27, 2015, 7:22 p.m. UTC 
  Hi Simon,

On Fri, 27 Mar 2015 20:15:13 +0100, Simon Marchi wrote:
> I think this patch is wrong. Starting with that commit (f30d5c7),
> some tests (e.g. mi-break.exp) started to fail for me, because
> of gdb segfaulting. Backtrace here: http://paste.ubuntu.com/10690836/
> 
> The address of expr is passed to the cleanup. When the cleanup is ran,
> expr is no longer in scope, so what is at that address is probably not
> safe to use anymore. That's my guess.

yes, you are sure right, I have reverted it now.

Sorry I made that commit somehow automatically, not expecting it may have any
side effects.


Thanks,
Jan
Simon Marchi:

I think this patch is wrong. Starting with that commit (f30d5c7),
some tests (e.g. mi-break.exp) started to fail for me, because
of gdb segfaulting.

The address of expr is passed to the cleanup. When the cleanup is ran,
expr is no longer in scope, so what is at that address is probably not
safe to use anymore. That's my guess.

gdb/ChangeLog
2015-03-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Revert:
	2015-03-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
	Code cleanup.
	* printcmd.c (print_command_1): Move expr variable scope.
---
 gdb/ChangeLog  | 7 +++++++
 gdb/printcmd.c | 3 +--
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Simon Marchi March 27, 2015, 7:24 p.m. UTC | #1 
On 15-03-27 03:22 PM, Jan Kratochvil wrote:
> Hi Simon,
> 
> On Fri, 27 Mar 2015 20:15:13 +0100, Simon Marchi wrote:
>> I think this patch is wrong. Starting with that commit (f30d5c7),
>> some tests (e.g. mi-break.exp) started to fail for me, because
>> of gdb segfaulting. Backtrace here: http://paste.ubuntu.com/10690836/
>>
>> The address of expr is passed to the cleanup. When the cleanup is ran,
>> expr is no longer in scope, so what is at that address is probably not
>> safe to use anymore. That's my guess.
> 
> yes, you are sure right, I have reverted it now.
> 
> Sorry I made that commit somehow automatically, not expecting it may have any
> side effects.

I would have done the same. I must say I was very surprised when git bisect
told me that was the culprit. Very very sneaky bug...


> Thanks,
> Jan
>
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 72940b0..6c6b94e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@ 
+2015-03-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Revert:
+	2015-03-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	Code cleanup.
+	* printcmd.c (print_command_1): Move expr variable scope.
+
 2015-03-27  Joel Brobecker  <brobecker@adacore.com>
 
 	* dtrace-probe.c (dtrace_process_dof_probe): Initialize expr to NULL.
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index a1451f8..deb501a 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -946,6 +946,7 @@  validate_format (struct format_data fmt, const char *cmdname)
 static void
 print_command_1 (const char *exp, int voidprint)
 {
+  struct expression *expr;
   struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   char format = 0;
   struct value *val;
@@ -968,8 +969,6 @@  print_command_1 (const char *exp, int voidprint)
 
   if (exp && *exp)
     {
-      struct expression *expr;
-
       expr = parse_expression (exp);
       make_cleanup (free_current_contents, &expr);
       val = evaluate_expression (expr);