Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Pedro Alves]

On 03/20/2019 02:08 PM, Tom Tromey wrote:
> I noticed that handle_inferior_event is just a small wrapper that
> frees the value chain.  This patch replaces it with a
> scoped_value_mark, reducing the number of lines of code here.
> 
> Regression tested on x86-64 Fedora 29.
> 
> gdb/ChangeLog
> 2019-03-20  Tom Tromey  <tromey@adacore.com>
> 
> 	* infrun.c (handle_inferior_event): Rename from
> 	handle_inferior_event_1.  Create a scoped_value_mark.
> 	(handle_inferior_event): Remove.

OK.

Thanks,
Pedro Alves

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Sergio Durigan Junior]

On Wednesday, March 20 2019, Tom Tromey wrote:

> I noticed that handle_inferior_event is just a small wrapper that
> frees the value chain.  This patch replaces it with a
> scoped_value_mark, reducing the number of lines of code here.
>
> Regression tested on x86-64 Fedora 29.

A few days ago (March 18th) a user reported a bug against Fedora GDB:

https://bugzilla.redhat.com/show_bug.cgi?id=1690120

And, after hours and hours investigating this, I found that your commit
actually fixes it.  Not sure if it was intended or not, but what a
coincidence...

Thanks,

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

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Sergio Durigan Junior]

On Thursday, March 21 2019, I wrote:

> On Wednesday, March 20 2019, Tom Tromey wrote:
>
>> I noticed that handle_inferior_event is just a small wrapper that
>> frees the value chain.  This patch replaces it with a
>> scoped_value_mark, reducing the number of lines of code here.
>>
>> Regression tested on x86-64 Fedora 29.
>
> A few days ago (March 18th) a user reported a bug against Fedora GDB:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1690120
>
> And, after hours and hours investigating this, I found that your commit
> actually fixes it.  Not sure if it was intended or not, but what a
> coincidence...

Of course, something I'd also like to ask is: do you think this change
is trivial enough to be included in the 8.3 release?  I'd like to see it
happen.  I can create a bug if needed.

Thanks,

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

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Tom Tromey]

>>>>> ">" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

>> A few days ago (March 18th) a user reported a bug against Fedora GDB:
>> 
>> https://bugzilla.redhat.com/show_bug.cgi?id=1690120
>> 
>> And, after hours and hours investigating this, I found that your commit
>> actually fixes it.  Not sure if it was intended or not, but what a
>> coincidence...

I am curious to know how this change could fix this bug.
That seems mysterious to me.

>> Of course, something I'd also like to ask is: do you think this change
>> is trivial enough to be included in the 8.3 release?  I'd like to see it
>> happen.  I can create a bug if needed.

I would have thought so but the fact that it causes this behavior makes
me worry a bit.  Maybe that's backward!

Tom

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Pedro Alves]

On 03/25/2019 03:35 PM, Tom Tromey wrote:
>>>>>> ">" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
> 
>>> A few days ago (March 18th) a user reported a bug against Fedora GDB:
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1690120
>>>
>>> And, after hours and hours investigating this, I found that your commit
>>> actually fixes it.  Not sure if it was intended or not, but what a
>>> coincidence...
> 
> I am curious to know how this change could fix this bug.
> That seems mysterious to me.

There's one behavior change the patch makes, which is that before the patch,
if handle_inferior_event_1 threw an error, value_free_to_mark would not
be called.  After the patch, it's always called, from scoped_value_mark's dtor.

From the backtrace in the bug report, we see that the crash happens late
during teardown, destroying an xmethod value in the global "all_values" vector:

 ...
 #13 std::vector<gdb::ref_ptr<value, value_ref_policy>, std::allocator<gdb::ref_ptr<value, value_ref_policy> > >::~vector (this=0x559d15b24a10 <all_values>, __in_chrg=<optimized out>) at /usr/include/c++/9/bits/stl_vector.h:677
 No locals.
 #14 0x00007f92a8fe0700 in __run_exit_handlers () from /lib64/libc.so.6
 No symbol table info available.
 #15 0x00007f92a8fe0840 in exit () from /lib64/libc.so.6
 No symbol table info available.
 ...

So it looks like your patch made it so that the value in question
avoids the crash because the value in question is no longer in the
all_values vector by the time gdb is tearing down?

Open questions then would be:

 - what was the exception in question that was thrown from
   within handle_inferior_event_1?  (And if there was
   no exception, then the theory above is incorrect.)

 - why don't non-temporary xmethod values put in the value history
   cause a similar crash at tear down time?

>>> Of course, something I'd also like to ask is: do you think this change
>>> is trivial enough to be included in the 8.3 release?  I'd like to see it
>>> happen.  I can create a bug if needed.
> 
> I would have thought so but the fact that it causes this behavior makes
> me worry a bit.  Maybe that's backward!

Thanks,
Pedro Alves

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> So it looks like your patch made it so that the value in question
Pedro> avoids the crash because the value in question is no longer in the
Pedro> all_values vector by the time gdb is tearing down?

Makes sense.

Pedro> Open questions then would be:

Pedro>  - what was the exception in question that was thrown from
Pedro>    within handle_inferior_event_1?  (And if there was
Pedro>    no exception, then the theory above is incorrect.)

Pedro>  - why don't non-temporary xmethod values put in the value history
Pedro>    cause a similar crash at tear down time?

Also, if a crash can occur during value destruction like this, then I
think it must indicate a bug somewhere else, like invalid reference
counting somewhere.

Tom

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Sergio Durigan Junior]

On Monday, March 25 2019, Tom Tromey wrote:

>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> So it looks like your patch made it so that the value in question
> Pedro> avoids the crash because the value in question is no longer in the
> Pedro> all_values vector by the time gdb is tearing down?
>
> Makes sense.

Thanks Pedro for the explanation.  Yeah, after investigating it became
clear to me that the patch fixes (or "fixes") the bug by destroying the
value instances before we exit GDB.

What's interesting is the fact that I cannot seem to reproduce easily.
For example, if you print anything other than an xmethod, and then you
print an xmethod, and then exit GDB, everything works correctly.  For
that reason, I'm still not able to write a testcase for it, because our
testing infrastructure prints some stuff as a preparation for the test.

> Pedro> Open questions then would be:
>
> Pedro>  - what was the exception in question that was thrown from
> Pedro>    within handle_inferior_event_1?  (And if there was
> Pedro>    no exception, then the theory above is incorrect.)
>
> Pedro>  - why don't non-temporary xmethod values put in the value history
> Pedro>    cause a similar crash at tear down time?
>
> Also, if a crash can occur during value destruction like this, then I
> think it must indicate a bug somewhere else, like invalid reference
> counting somewhere.

That's a good point, yeah.  I can try investigating more.

Having said that, I think the patch is simple enough to be safely
backportable to 8.3, and it doesn't really break anything (in fact, as
Pedro mentioned, it makes things more correct, because now we're sure
that we will destroy the objects if an exception is thrown), so IMHO
there's no downside to it.  WDYT?

Thanks,

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

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

>> Also, if a crash can occur during value destruction like this, then I
>> think it must indicate a bug somewhere else, like invalid reference
>> counting somewhere.

Sergio> That's a good point, yeah.  I can try investigating more.

Maybe one idea would be to back out my patch and then run the failing
test under valgrind.

Sergio> Having said that, I think the patch is simple enough to be safely
Sergio> backportable to 8.3, and it doesn't really break anything (in fact, as
Sergio> Pedro mentioned, it makes things more correct, because now we're sure
Sergio> that we will destroy the objects if an exception is thrown), so IMHO
Sergio> there's no downside to it.  WDYT?

I agree.

Tom

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Sergio Durigan Junior]

On Tuesday, March 26 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>>> Also, if a crash can occur during value destruction like this, then I
>>> think it must indicate a bug somewhere else, like invalid reference
>>> counting somewhere.
>
> Sergio> That's a good point, yeah.  I can try investigating more.
>
> Maybe one idea would be to back out my patch and then run the failing
> test under valgrind.

Thanks.

> Sergio> Having said that, I think the patch is simple enough to be safely
> Sergio> backportable to 8.3, and it doesn't really break anything (in fact, as
> Sergio> Pedro mentioned, it makes things more correct, because now we're sure
> Sergio> that we will destroy the objects if an exception is thrown), so IMHO
> Sergio> there's no downside to it.  WDYT?
>
> I agree.

Thanks, Tom.  I created
<https://sourceware.org/bugzilla/show_bug.cgi?id=24391>.  Would you like
me to go ahead and backport the patch to the 8.3 branch, or do you want
to do that?

Thanks,

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

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> Thanks, Tom.  I created
Sergio> <https://sourceware.org/bugzilla/show_bug.cgi?id=24391>.  Would you like
Sergio> me to go ahead and backport the patch to the 8.3 branch, or do you want
Sergio> to do that?

Please do it!  Thanks.

Tom

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Sergio Durigan Junior]

On Tuesday, March 26 2019, I wrote:

> On Tuesday, March 26 2019, Tom Tromey wrote:
>
>>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>>
>>>> Also, if a crash can occur during value destruction like this, then I
>>>> think it must indicate a bug somewhere else, like invalid reference
>>>> counting somewhere.
>>
>> Sergio> That's a good point, yeah.  I can try investigating more.
>>
>> Maybe one idea would be to back out my patch and then run the failing
>> test under valgrind.
>
> Thanks.

I did that, and we (Pedro, Mark, Frank and I) did a session of
collective investigation.  I summarized what we found here:

  https://bugzilla.redhat.com/show_bug.cgi?id=1690120#c25

Thanks,

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

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Tom Tromey]

Sergio> I did that, and we (Pedro, Mark, Frank and I) did a session of
Sergio> collective investigation.  I summarized what we found here:
Sergio>   https://bugzilla.redhat.com/show_bug.cgi?id=1690120#c25

Thanks, that's very interesting.
I suppose either better control over the order of destruction is needed,
or maybe finalize_python should clear gdb_python_initialized and then
this should be checked in xmethod destructor.

Tom

Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1

[Pedro Alves]

On 03/27/2019 12:57 PM, Tom Tromey wrote:
> Sergio> I did that, and we (Pedro, Mark, Frank and I) did a session of
> Sergio> collective investigation.  I summarized what we found here:
> Sergio>   https://bugzilla.redhat.com/show_bug.cgi?id=1690120#c25
> 
> Thanks, that's very interesting.
> I suppose either better control over the order of destruction is needed,
> or maybe finalize_python should clear gdb_python_initialized and then
> this should be checked in xmethod destructor.
I think the former is better.  I think we should put an 
  all_values.clear ();
call in quit_force, before the do_final_cleanups call.  Even
better, add a new finalize_values function next to
_initialize_values, and call that.

Thanks,
Pedro Alves

[PATCH] Destroy allocated values when exiting GDB (was: Re: [PATCH] Merge handle_inferior_event and handle_inferior_event_1)

[Sergio Durigan Junior]

On Thursday, March 28 2019, Pedro Alves wrote:

> On 03/27/2019 12:57 PM, Tom Tromey wrote:
>> Sergio> I did that, and we (Pedro, Mark, Frank and I) did a session of
>> Sergio> collective investigation.  I summarized what we found here:
>> Sergio>   https://bugzilla.redhat.com/show_bug.cgi?id=1690120#c25
>> 
>> Thanks, that's very interesting.
>> I suppose either better control over the order of destruction is needed,
>> or maybe finalize_python should clear gdb_python_initialized and then
>> this should be checked in xmethod destructor.
> I think the former is better.  I think we should put an 
>   all_values.clear ();
> call in quit_force, before the do_final_cleanups call.  Even
> better, add a new finalize_values function next to
> _initialize_values, and call that.

How does this look?

From b88067edfb2289cd5af3c9b8984b0b0354c965db Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Fri, 29 Mar 2019 17:34:54 -0400
Subject: [PATCH] Destroy allocated values when exiting GDB

When the user exits GDB, we might still have some allocated values in
the chain, which, in specific scenarios, can cause problems when GDB
attempts to destroy them in "quit_force".  For example, see the bug
reported at:

  https://bugzilla.redhat.com/show_bug.cgi?id=1690120

And the thread starting at:

  https://sourceware.org/ml/gdb-patches/2019-03/msg00475.html
  Message-ID: <87r2azkhmq.fsf@redhat.com>

In order to avoid that, and to be more aware of our allocated
resources, this commit implements a new function "finalize_values" and
calls it from inside "quit_force".

Tested by the BuildBot.

2019-03-29  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	* top.c (quit_force): Call 'finalize_values'.
	* value.c (finalize_values): New function.
	* value.h (finalize_values): Declare.
---
 gdb/top.c   | 3 +++
 gdb/value.c | 8 ++++++++
 gdb/value.h | 4 ++++
 3 files changed, 15 insertions(+)

diff --git a/gdb/top.c b/gdb/top.c
index b10b0649e9..ffbe8e517f 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1672,6 +1672,9 @@ quit_force (int *exit_arg, int from_tty)
     }
   END_CATCH
 
+  /* Destroy any values currently allocated.  */
+  finalize_values ();
+
   /* Do any final cleanups before exiting.  */
   TRY
     {
diff --git a/gdb/value.c b/gdb/value.c
index dc297dfe0f..bcfc084e09 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -4132,3 +4132,11 @@ prevents future values, larger than this size, from being allocated."),
 			    selftests::test_insert_into_bit_range_vector);
 #endif
 }
+
+/* See value.h.  */
+
+void
+finalize_values ()
+{
+  all_values.clear ();
+}
diff --git a/gdb/value.h b/gdb/value.h
index d3905cc354..7853950ca3 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1189,4 +1189,8 @@ extern struct value *call_xmethod (struct value *method,
 extern int value_union_variant (struct type *union_type,
 				const gdb_byte *contents);
 
+/* Destroy the values currently allocated.  This is mostly called when
+   GDB is exiting (e.g., on quit_force).  */
+extern void finalize_values ();
+
 #endif /* !defined (VALUE_H) */
-- 
2.17.2

Re: [PATCH] Destroy allocated values when exiting GDB

[Pedro Alves]

On 03/29/2019 09:43 PM, Sergio Durigan Junior wrote:

> 
> diff --git a/gdb/top.c b/gdb/top.c
> index b10b0649e9..ffbe8e517f 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -1672,6 +1672,9 @@ quit_force (int *exit_arg, int from_tty)
>      }
>    END_CATCH
>  
> +  /* Destroy any values currently allocated.  */
> +  finalize_values ();

I think that someone reading this without context may not realize
why we need to do that.  How about extending the comment, like:

  /* Destroy any values currently allocated now instead of leaving it
     to global destructors, because that may be too late.  For
     example, the destructors of xmethod values call into the Python
     runtime, which is finalized via a final cleanup.  */

> diff --git a/gdb/value.h b/gdb/value.h
> index d3905cc354..7853950ca3 100644
> --- a/gdb/value.h
> +++ b/gdb/value.h
> @@ -1189,4 +1189,8 @@ extern struct value *call_xmethod (struct value *method,
>  extern int value_union_variant (struct type *union_type,
>  				const gdb_byte *contents);
>  
> +/* Destroy the values currently allocated.  This is mostly called when
> +   GDB is exiting (e.g., on quit_force).  */

s/mostly//

> +extern void finalize_values ();
> +
>  #endif /* !defined (VALUE_H) */

LGTM with those changes.

Thanks,
Pedro Alves

Re: [PATCH] Destroy allocated values when exiting GDB

[Sergio Durigan Junior]

On Monday, April 01 2019, Pedro Alves wrote:

> On 03/29/2019 09:43 PM, Sergio Durigan Junior wrote:
>
>> 
>> diff --git a/gdb/top.c b/gdb/top.c
>> index b10b0649e9..ffbe8e517f 100644
>> --- a/gdb/top.c
>> +++ b/gdb/top.c
>> @@ -1672,6 +1672,9 @@ quit_force (int *exit_arg, int from_tty)
>>      }
>>    END_CATCH
>>  
>> +  /* Destroy any values currently allocated.  */
>> +  finalize_values ();
>
> I think that someone reading this without context may not realize
> why we need to do that.  How about extending the comment, like:
>
>   /* Destroy any values currently allocated now instead of leaving it
>      to global destructors, because that may be too late.  For
>      example, the destructors of xmethod values call into the Python
>      runtime, which is finalized via a final cleanup.  */

Done.

>> diff --git a/gdb/value.h b/gdb/value.h
>> index d3905cc354..7853950ca3 100644
>> --- a/gdb/value.h
>> +++ b/gdb/value.h
>> @@ -1189,4 +1189,8 @@ extern struct value *call_xmethod (struct value *method,
>>  extern int value_union_variant (struct type *union_type,
>>  				const gdb_byte *contents);
>>  
>> +/* Destroy the values currently allocated.  This is mostly called when
>> +   GDB is exiting (e.g., on quit_force).  */
>
> s/mostly//

Done.

>> +extern void finalize_values ();
>> +
>>  #endif /* !defined (VALUE_H) */
>
> LGTM with those changes.

Thanks for the review.  Pushed:

9d1447e09d4aa673826039321163b5a684e8e043

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