[5/5] Fix memory leak in exception code

  PR gdb/24475 concerns a memory leak coming from gdb's exception
handling code.

The leak occurs because throw_exception_sjlj does not arrange to
destroy the exception object it is passed.  However, because
gdb_exception has a destructor, it's undefined to longjmp in this
situation.

This patch fixes the problem by avoiding the need to run any
destructors in gdb_rl_callback_handler, by making the gdb_exception
"static".

gdb/ChangeLog
2019-04-25  Tom Tromey  <tromey@adacore.com>

	PR gdb/24475:
	* event-top.c (gdb_rl_callback_handler): Make "gdb_rl_expt"
	static.
---
 gdb/ChangeLog   | 6 ++++++
 gdb/event-top.c | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)
  

On 4/25/19 5:52 PM, Tom Tromey wrote:
> PR gdb/24475 concerns a memory leak coming from gdb's exception
> handling code.
> 
> The leak occurs because throw_exception_sjlj does not arrange to
> destroy the exception object it is passed.  However, because
> gdb_exception has a destructor, it's undefined to longjmp in this
> situation.
> 
> This patch fixes the problem by avoiding the need to run any
> destructors in gdb_rl_callback_handler, by making the gdb_exception
> "static".
> 

Why fix it like this, instead of fixing it like in the guile patch?
I'd think you could even use a common POD type for both guile and here?

Thanks,
Pedro Alves
  

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

Pedro> Why fix it like this, instead of fixing it like in the guile patch?
Pedro> I'd think you could even use a common POD type for both guile and here?

No particularly good reason.  I'll change it.

Tom
  

Pedro> Why fix it like this, instead of fixing it like in the guile patch?
Pedro> I'd think you could even use a common POD type for both guile and here?

Tom> No particularly good reason.  I'll change it.

Actually, there was a reason.  Converting an exception to a POD means
copying the string contents.

Perhaps that isn't a big concern.  I guess only exceptions escaping to
the top level will do this.

Tom
  

On 4/25/19 7:30 PM, Tom Tromey wrote:
> Pedro> Why fix it like this, instead of fixing it like in the guile patch?
> Pedro> I'd think you could even use a common POD type for both guile and here?
> 
> Tom> No particularly good reason.  I'll change it.
> 
> Actually, there was a reason.  Converting an exception to a POD means
> copying the string contents.
> 
> Perhaps that isn't a big concern.  I guess only exceptions escaping to
> the top level will do this.

Yeah, I don't think it's a concern here.  

It seemed better than adding a global, thinking about threads, but now
that I think a bit more about it, I realize that we won't ever be
able to have multiple threads call into readline simultaneously -- it's
full of global state.  So I'm fine with your patch as is, afterall.

I think it'd be good to extend the comment, something like:

-  /* This is static to avoid undefined behavior when calling
-     longjmp.  */
+  /* This is static to avoid undefined behavior when calling
+     longjmp -- gdb_exception has a dtor with side effects.  */

Thanks,
Pedro Alves
  

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

Pedro> It seemed better than adding a global, thinking about threads, but now
Pedro> that I think a bit more about it, I realize that we won't ever be
Pedro> able to have multiple threads call into readline simultaneously -- it's
Pedro> full of global state.  So I'm fine with your patch as is, afterall.

Alright, I'll check in that version, after making the tweaks you
requested.

thanks,
Tom