Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC)

[Pedro Alves <pedro@palves.net>]

On 7/10/20 3:55 AM, Simon Marchi wrote:
>>> I don't know if it would be worth it, but I'd like if we could assert (abort
>>> GDB) if an exception does try to exit the destructor.  The `restore` method
>>> is non-trivial and calls into other non-trivial functions, so it would be
>>> possible for a change far far away to cause that to happen.
>>
>> It will already abort.  Destructors are noexcept by default, so if an exception
>> escapes a destructor, std::terminate() is called, and that calls abort by default.
> 
> Oh, didn't know that!  I thought it was just "undefined behavior".
> 
>>> What do you think of keeping the try/catch, but using `gdb_assert_not_reached`
>>> in it?
>>
>> Not sure.  If we do that, we do get a nicer error message.  However if the user
>> says "n" to "Quit this debugging session" we still abort.
>>
>> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello
>> A problem internal to GDB has been detected,
>> further debugging may prove unreliable.
>> Quit this debugging session? (y or n) n
>>
>> This is a bug, please report it.  For instructions, see:
>> <https://www.gnu.org/software/gdb/bugs/>.
>>
>> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello
>> A problem internal to GDB has been detected,
>> further debugging may prove unreliable.
>> Create a core file of GDB? (y or n) n
>> terminate called after throwing an instance of 'gdb_exception_quit'
>> Aborted (core dumped)
>>
>> Maybe it would be interesting to add a variant of internal_error that did
>> not throw a quit, so the user could swallow the exception...  Maybe consider
>> wrapping that as a generic facility to add to all non-trivial RAII destructors
>> we have?  Like a function that takes a function_view as parameter, so
>> we would write:
>>
>> foo::~foo ()
>> {
>>   safe_dtor (__FILE__, __LINE__, [&] () 
>>     {
>>       restore ();
>>     });
>> }
>>
>> Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT
>> macro uses to be able to write:
>>
>> foo::~foo ()
>> {
>>   SAFE_DTOR { restore (); };
>> }
> 
> That's fancier than what I hoped for :).  My goal was just to make sure
> we catch it if we ever make a change that causes an exception to escape.
> Although I wouldn't be against what you proposed.
> 
>> Here's the current version of the patch.
> 
> That looks fine to me.

FYI, I've finally merged this to master.

Thanks,
Pedro Alves