[PATCH v2 1/5] [gdb] Don't create registry keys in destructor

[PATCH v2 1/5] [gdb] Don't create registry keys in destructor

[Tom de Vries]

Creating a registry key using emplace calls new:
...
      DATA *result = new DATA (std::forward<Args> (args)...);
...
which can throw a bad alloc, which will terminate gdb if called from a
destructor.

Fix this in a few places.

Tested on aarch64-linux.
---
 gdb/ada-tasks.c  | 33 +++++++++++++++++++++++----------
 gdb/objfiles.c   | 21 +++++++++++++++------
 gdb/solib-svr4.c | 17 ++++++++++++-----
 gdb/source.c     | 23 +++++++++++++++--------
 gdb/source.h     |  3 ++-
 gdb/symtab.c     | 18 ++++++++++++------
 6 files changed, 79 insertions(+), 36 deletions(-)

diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index d050b269815..6d925e8e86e 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -287,18 +287,21 @@ task_to_str (int taskno, const ada_task_info *task_info)
 }
 
 /* Return the ada-tasks module's data for the given program space (PSPACE).
-   If none is found, add a zero'ed one now.
-
-   This function always returns a valid object.  */
+   If none is found, add a zero'ed one now, unless !CREATE.  */
 
 static struct ada_tasks_pspace_data *
-get_ada_tasks_pspace_data (struct program_space *pspace)
+get_ada_tasks_pspace_data (struct program_space *pspace, bool create = true)
 {
   struct ada_tasks_pspace_data *data;
 
   data = ada_tasks_pspace_data_handle.get (pspace);
   if (data == NULL)
-    data = ada_tasks_pspace_data_handle.emplace (pspace);
+    {
+      if (!create)
+	return nullptr;
+
+      data = ada_tasks_pspace_data_handle.emplace (pspace);
+    }
 
   return data;
 }
@@ -316,13 +319,18 @@ get_ada_tasks_pspace_data (struct program_space *pspace)
    not use tasking.  */
 
 static struct ada_tasks_inferior_data *
-get_ada_tasks_inferior_data (struct inferior *inf)
+get_ada_tasks_inferior_data (struct inferior *inf, bool create = true)
 {
   struct ada_tasks_inferior_data *data;
 
   data = ada_tasks_inferior_data_handle.get (inf);
   if (data == NULL)
-    data = ada_tasks_inferior_data_handle.emplace (inf);
+    {
+      if (!create)
+	return nullptr;
+
+      data = ada_tasks_inferior_data_handle.emplace (inf);
+    }
 
   return data;
 }
@@ -1447,7 +1455,10 @@ ada_task_list_changed (struct inferior *inf)
 static void
 ada_tasks_invalidate_pspace_data (struct program_space *pspace)
 {
-  get_ada_tasks_pspace_data (pspace)->initialized_p = 0;
+  auto data = get_ada_tasks_pspace_data (pspace, false);
+  if (data == nullptr)
+    return;
+  data->initialized_p = 0;
 }
 
 /* Invalidate the per-inferior data.  */
@@ -1455,8 +1466,10 @@ ada_tasks_invalidate_pspace_data (struct program_space *pspace)
 static void
 ada_tasks_invalidate_inferior_data (struct inferior *inf)
 {
-  struct ada_tasks_inferior_data *data = get_ada_tasks_inferior_data (inf);
-
+  struct ada_tasks_inferior_data *data
+    = get_ada_tasks_inferior_data (inf, false);
+  if (data == nullptr)
+    return;
   data->known_tasks_kind = ADA_TASKS_UNKNOWN;
   data->task_list_valid_p = false;
 }
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 0e076fe36be..d25d34cef6a 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -82,17 +82,22 @@ objfile_pspace_info::~objfile_pspace_info ()
   xfree (sections);
 }
 
-/* Get the current svr4 data.  If none is found yet, add it now.  This
-   function always returns a valid object.  */
+/* Get the current svr4 data.  If none is found yet, add it now, unless
+   !CREATE.  */
 
 static struct objfile_pspace_info *
-get_objfile_pspace_data (struct program_space *pspace)
+get_objfile_pspace_data (struct program_space *pspace, bool create = true)
 {
   struct objfile_pspace_info *info;
 
   info = objfiles_pspace_data.get (pspace);
   if (info == NULL)
-    info = objfiles_pspace_data.emplace (pspace);
+    {
+      if (!create)
+	return nullptr;
+
+      info = objfiles_pspace_data.emplace (pspace);
+    }
 
   return info;
 }
@@ -563,14 +568,18 @@ objfile::~objfile ()
 
   {
     symtab_and_line cursal
-      = get_current_source_symtab_and_line (this->pspace ());
+      = get_current_source_symtab_and_line (this->pspace (), false);
 
     if (cursal.symtab && cursal.symtab->compunit ()->objfile () == this)
       clear_current_source_symtab_and_line (this->pspace ());
   }
 
   /* Rebuild section map next time we need it.  */
-  get_objfile_pspace_data (m_pspace)->section_map_dirty = 1;
+  {
+    auto info = get_objfile_pspace_data (m_pspace, false);
+    if (info != nullptr)
+      info->section_map_dirty = 1;
+  }
 }
 
 \f
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 7999a8e6c7e..f61af431b35 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -426,16 +426,21 @@ free_probes_table (struct svr4_info *info)
   info->probes_table.reset (nullptr);
 }
 
-/* Get the svr4 data for program space PSPACE.  If none is found yet, add it now.
-   This function always returns a valid object.  */
+/* Get the svr4 data for program space PSPACE.  If none is found yet, add it
+   now, unless !CREATE.  */
 
 static struct svr4_info *
-get_svr4_info (program_space *pspace)
+get_svr4_info (program_space *pspace, bool create = true)
 {
   struct svr4_info *info = solib_svr4_pspace_data.get (pspace);
 
   if (info == NULL)
-    info = solib_svr4_pspace_data.emplace (pspace);
+    {
+      if (!create)
+	return nullptr;
+
+      info = solib_svr4_pspace_data.emplace (pspace);
+    }
 
   return info;
 }
@@ -1631,7 +1636,9 @@ probes_table_htab_remove_objfile_probes (void **slot, void *info)
 static void
 probes_table_remove_objfile_probes (struct objfile *objfile)
 {
-  svr4_info *info = get_svr4_info (objfile->pspace ());
+  svr4_info *info = get_svr4_info (objfile->pspace (), false);
+  if (info == nullptr)
+    return;
   if (info->probes_table != nullptr)
     htab_traverse_noresize (info->probes_table.get (),
 			    probes_table_htab_remove_objfile_probes, objfile);
diff --git a/gdb/source.c b/gdb/source.c
index 292d2bf71c5..8476e94bdce 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -219,29 +219,34 @@ get_lines_to_list (void)
 }
 
 /* A helper to return the current source location object for PSPACE,
-   creating it if it does not exist.  */
+   creating it if it does not exist, unless !CREATE.  */
 
 static current_source_location *
-get_source_location (program_space *pspace)
+get_source_location (program_space *pspace, bool create = true)
 {
   current_source_location *loc
     = current_source_key.get (pspace);
   if (loc == nullptr)
-    loc = current_source_key.emplace (pspace);
+    {
+      if (!create)
+	return nullptr;
+
+      loc = current_source_key.emplace (pspace);
+    }
   return loc;
 }
 
 /* See source.h.  */
    
 symtab_and_line
-get_current_source_symtab_and_line (program_space *pspace)
+get_current_source_symtab_and_line (program_space *pspace, bool create)
 {
   symtab_and_line cursal;
-  current_source_location *loc = get_source_location (pspace);
+  current_source_location *loc = get_source_location (pspace, create);
 
   cursal.pspace = pspace;
-  cursal.symtab = loc->symtab ();
-  cursal.line = loc->line ();
+  cursal.symtab = loc == nullptr ? nullptr : loc->symtab ();
+  cursal.line = loc == nullptr ? 0 : loc->line ();
   cursal.pc = 0;
   cursal.end = 0;
   
@@ -300,7 +305,9 @@ set_current_source_symtab_and_line (const symtab_and_line &sal)
 void
 clear_current_source_symtab_and_line (program_space *pspace)
 {
-  current_source_location *loc = get_source_location (pspace);
+  current_source_location *loc = current_source_key.get (pspace);
+  if (loc == nullptr)
+    return;
   loc->set (nullptr, 0);
 }
 
diff --git a/gdb/source.h b/gdb/source.h
index 541ee3b7a12..fc5cb8bad5b 100644
--- a/gdb/source.h
+++ b/gdb/source.h
@@ -108,9 +108,10 @@ extern int get_first_line_listed (void);
 extern int get_lines_to_list (void);
 
 /* Return the current source file for listing and next line to list.
+   Don't create a current source location object, unless CREATE.
    NOTE: The returned sal pc and end fields are not valid.  */
 extern symtab_and_line get_current_source_symtab_and_line
-  (program_space *pspace);
+  (program_space *pspace, bool create = true);
 
 /* If the current source file for listing is not set, try and get a default.
    Usually called before get_current_source_symtab_and_line() is called.
diff --git a/gdb/symtab.c b/gdb/symtab.c
index a479e920c60..6c6f4da966e 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -102,7 +102,7 @@ static struct block_symbol
 			    const domain_search_flags domain);
 
 static void set_main_name (program_space *pspace, const char *name,
-			   language lang);
+			   language lang, bool create = true);
 
 /* Type of the data stored on the program space.  */
 
@@ -1756,7 +1756,7 @@ symtab_all_objfiles_removed (program_space *pspace)
   symbol_cache_flush (pspace);
 
   /* Forget everything we know about the main function.  */
-  set_main_name (pspace, nullptr, language_unknown);
+  set_main_name (pspace, nullptr, language_unknown, false);
 }
 
 /* This module's 'free_objfile' observer.  */
@@ -6445,15 +6445,18 @@ make_source_files_completion_list (const char *text, const char *word)
 
 /* Return the "main_info" object for the current program space.  If
    the object has not yet been created, create it and fill in some
-   default values.  */
+   default values, unless !CREATE.  */
 
 static main_info *
-get_main_info (program_space *pspace)
+get_main_info (program_space *pspace, bool create = true)
 {
   main_info *info = main_progspace_key.get (pspace);
 
   if (info == NULL)
     {
+      if (!create)
+	return nullptr;
+
       /* It may seem strange to store the main name in the progspace
 	 and also in whatever objfile happens to see a main name in
 	 its debug info.  The reason for this is mainly historical:
@@ -6467,9 +6470,12 @@ get_main_info (program_space *pspace)
 }
 
 static void
-set_main_name (program_space *pspace, const char *name, enum language lang)
+set_main_name (program_space *pspace, const char *name, enum language lang,
+	       bool create)
 {
-  main_info *info = get_main_info (pspace);
+  main_info *info = get_main_info (pspace, create);
+  if (info == nullptr)
+    return;
 
   if (!info->name_of_main.empty ())
     {

base-commit: 5bc1b13676bc5eeefc50d25379991f5f9255eb80
-- 
2.35.3

[PATCH v2 2/5] [gdb] Handle bad alloc handling in gdb_bfd_open

[Tom de Vries]

Say we simulate a bad alloc in gdb_bfd_init_data:
...
+  {
+    static bool throw_bad_alloc = true;
+    if (throw_bad_alloc)
+      {
+       throw_bad_alloc = false;
+
+       va_list dummy;
+       throw gdb_quit_bad_alloc (gdb_exception_quit ("bad alloc", dummy));
+      }
+  }
   gdata = new gdb_bfd_data (abfd, st);
...

That works out fine for doing "file a.out" once:
...
$ gdb -q -batch -ex "file a.out"
bad alloc
$
...
but doing so twice get us:
...
$ gdb -q -batch -ex "file a.out" -ex "file a.out"
bad alloc

Fatal signal: Segmentation fault
----- Backtrace -----
0x5183f7 gdb_internal_backtrace_1
        /home/vries/gdb/src/gdb/bt-utils.c:121
0x5183f7 _Z22gdb_internal_backtracev
        /home/vries/gdb/src/gdb/bt-utils.c:167
0x62329b handle_fatal_signal
        /home/vries/gdb/src/gdb/event-top.c:917
0x6233ef handle_sigsegv
        /home/vries/gdb/src/gdb/event-top.c:990
0xfffeffba483f ???
0x65554c eq_bfd
        /home/vries/gdb/src/gdb/gdb_bfd.c:231
0xeaca77 htab_find_with_hash
        /home/vries/gdb/src/libiberty/hashtab.c:597
0x657487 _Z12gdb_bfd_openPKcS0_ib
        /home/vries/gdb/src/gdb/gdb_bfd.c:580
0x6272d7 _Z16exec_file_attachPKci
        /home/vries/gdb/src/gdb/exec.c:451
0x627e67 exec_file_command
        /home/vries/gdb/src/gdb/exec.c:550
0x627f23 file_command
        /home/vries/gdb/src/gdb/exec.c:565
Segmentation fault (core dumped)
$
...

The problem is in gdb_bfd_open, where we insert abfd into gdb_bfd_cache:
...
  if (bfd_sharing)
    {
      slot = htab_find_slot_with_hash (gdb_bfd_cache, &search, hash, INSERT);
      gdb_assert (!*slot);
      *slot = abfd;
    }

  gdb_bfd_init_data (abfd, &st);
...
while the bad alloc means that gdb_bfd_init_data is interrupted and abfd is
not properly initialized.

Fix this by reversing the order, inserting abfd into gdb_bfd_cache only after
a successful call to gdb_bfd_init_data, such that we get:
...
$ gdb -q -batch -ex "file a.out" -ex "file a.out"
bad alloc
$
...

Tested on aarch64-linux.
---
 gdb/gdb_bfd.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
index 0854d571ecf..b142f985dcd 100644
--- a/gdb/gdb_bfd.c
+++ b/gdb/gdb_bfd.c
@@ -587,6 +587,14 @@ gdb_bfd_open (const char *name, const char *target, int fd,
 			  host_address_to_string (abfd),
 			  bfd_get_filename (abfd));
 
+  /* It's important to pass the already-computed stat info here,
+     rather than, say, calling gdb_bfd_ref_ptr::new_reference.  BFD by
+     default will "stat" the file each time bfd_get_mtime is called --
+     and since we will enter it into the hash table using this
+     mtime, if the file changed at the wrong moment, the race would
+     lead to a hash table corruption.  */
+  gdb_bfd_init_data (abfd, &st);
+
   if (bfd_sharing)
     {
       slot = htab_find_slot_with_hash (gdb_bfd_cache, &search, hash, INSERT);
@@ -594,13 +602,6 @@ gdb_bfd_open (const char *name, const char *target, int fd,
       *slot = abfd;
     }
 
-  /* It's important to pass the already-computed stat info here,
-     rather than, say, calling gdb_bfd_ref_ptr::new_reference.  BFD by
-     default will "stat" the file each time bfd_get_mtime is called --
-     and since we already entered it into the hash table using this
-     mtime, if the file changed at the wrong moment, the race would
-     lead to a hash table corruption.  */
-  gdb_bfd_init_data (abfd, &st);
   return gdb_bfd_ref_ptr (abfd);
 }
 
-- 
2.35.3

[PATCH v2 3/5] [gdb] Handle bad alloc in cooked_index_debug_info::do_reading

[Tom de Vries]

Say we simulate done_reading throwing a bad alloc in
cooked_index_debug_info::do_reading:
...
   gdb::task_group workers ([this] ()
   {
+    {
+      static bool throw_bad_alloc = true;
+      if (throw_bad_alloc)
+      {
+        throw_bad_alloc = false;
+
+        va_list dummy;
+        throw gdb_quit_bad_alloc (gdb_exception_quit ("bad alloc", dummy));
+      }
+    }
     this->done_reading ();
   });
...

This gets gdb to terminate:
...
$ gdb -q -batch -ex "file a.out"
terminate called after throwing an instance of 'gdb_quit_bad_alloc'
...
because the gdb::task_group finalizer is called from the destructor
gdb::~task_group.

Fix this by catching the exception and storing it in m_failed, as done in
cooked_index_worker::start, such that we get:
...
$ gdb -q -batch -ex "file a.out"
bad alloc
$
...

Tested on aarch64-linux.
---
 gdb/dwarf2/read.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ea31d8dd851..771be5bfdfe 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -5012,7 +5012,15 @@ cooked_index_debug_info::do_reading ()
   /* Work is done in a task group.  */
   gdb::task_group workers ([this] ()
   {
-    this->done_reading ();
+    try
+      {
+	this->done_reading ();
+      }
+    catch (gdb_exception &exc)
+      {
+	this->m_failed = std::move (exc);
+	this->set (cooked_state::CACHE_DONE);
+      }
   });
 
   auto end = per_bfd->all_units.end ();
-- 
2.35.3

[PATCH v2 4/5] [gdb] Handle bad alloc in gdb_rl_callback_read_char_wrapper_noexcept

[Tom de Vries]

Say we simulate a bad alloc in exceptions_state_mc_init:
...
 jmp_buf *
 exceptions_state_mc_init ()
 {
+  {
+    static bool throw_bad_alloc = true;
+    if (throw_bad_alloc)
+      {
+       throw_bad_alloc = false;
+
+       va_list dummy;
+       throw gdb_quit_bad_alloc (gdb_exception_quit ("bad alloc", dummy));
+      }
+  }
   catchers.emplace_front ();
   return &catchers.front ().buf;
 }
...

After starting gdb and typing "q", gdb terminates:
...
$ gdb -q
(gdb) terminate called after throwing an instance of 'gdb_quit_bad_alloc'
  what():  std::bad_alloc
...
because the bad alloc (thrown in TRY_SJLJ) is caught by the noexcept on
gdb_rl_callback_read_char_wrapper_noexcept:
...
static struct gdb_exception
gdb_rl_callback_read_char_wrapper_noexcept () noexcept
{
  struct gdb_exception gdb_expt;

  /* C++ exceptions can't normally be thrown across readline (unless
     it is built with -fexceptions, but it won't by default on many
     ABIs).  So we instead wrap the readline call with a sjlj-based
     TRY/CATCH, and rethrow the GDB exception once back in GDB.  */
  TRY_SJLJ
...

Fix this by renaming gdb_rl_callback_read_char_wrapper_noexcept to
gdb_rl_callback_read_char_wrapper_sjlj and calling it from a wrapper function
that catches the bad alloc expection:
...
static struct gdb_exception
gdb_rl_callback_read_char_wrapper_noexcept () noexcept
{
  try
    {
      return gdb_rl_callback_read_char_wrapper_sjlj ();
    }
  catch (gdb_exception &ex)
    {
      return std::move (ex);
    }
}
...
getting us instead:
...
$ gdb -q
(gdb) bad alloc
(gdb) q
...

Tested on aarch64-linux.
---
 gdb/event-top.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/gdb/event-top.c b/gdb/event-top.c
index cdc7874b7fa..d3cf144958a 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -184,7 +184,7 @@ extern "C" void _rl_signal_handler (int);
    (sjlj-based) C++ exceptions.  */
 
 static struct gdb_exception
-gdb_rl_callback_read_char_wrapper_noexcept () noexcept
+gdb_rl_callback_read_char_wrapper_sjlj ()
 {
   struct gdb_exception gdb_expt;
 
@@ -229,6 +229,22 @@ gdb_rl_callback_read_char_wrapper_noexcept () noexcept
   return gdb_expt;
 }
 
+/* Wrapper around gdb_rl_callback_read_char_wrapper_sjlj to ensure
+   noexcept.  */
+
+static struct gdb_exception
+gdb_rl_callback_read_char_wrapper_noexcept () noexcept
+{
+  try
+    {
+      return gdb_rl_callback_read_char_wrapper_sjlj ();
+    }
+  catch (gdb_exception &ex)
+    {
+      return std::move (ex);
+    }
+}
+
 static void
 gdb_rl_callback_read_char_wrapper (gdb_client_data client_data)
 {
-- 
2.35.3

[PATCH v2 5/5] [gdb] Handle bad alloc in cooked_index_worker::start

[Tom de Vries]

Say we simulate a bad alloc in post_task in cooked_index_worker::start:
...
 void
 cooked_index_worker::start ()
 {
+  {
+    static bool throw_bad_alloc = true;
+    if (throw_bad_alloc)
+      {
+       throw_bad_alloc = false;
+
+       va_list dummy;
+       throw gdb_quit_bad_alloc (gdb_exception_quit ("bad alloc", dummy));
+      }
+  }
   gdb::thread_pool::g_thread_pool->post_task ([=] ()
...

That makes gdb hang:
...
$ gdb -q -batch a.out
bad alloc
<hang>
...
because we're waiting for the cooked index to transfer into
cooked_state::CACHE_DONE.

Fix that by catching the bad alloc in cooked_index_worker::start, and
setting the cooked index to cooked_state::CACHE_DONE, as done in the posted
task, getting us instead:
...
$ gdb -q -batch a.out
bad alloc
$
...

Tested on aarch64-linux.
---
 gdb/dwarf2/cooked-index.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 4073c924890..51e24b74239 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -465,20 +465,28 @@ cooked_index_shard::find (const std::string &name, bool completing) const
 void
 cooked_index_worker::start ()
 {
-  gdb::thread_pool::g_thread_pool->post_task ([=] ()
-  {
-    try
-      {
-	do_reading ();
-      }
-    catch (const gdb_exception &exc)
+  try
+    {
+      gdb::thread_pool::g_thread_pool->post_task ([=] ()
       {
-	m_failed = exc;
-	set (cooked_state::CACHE_DONE);
-      }
+	try
+	  {
+	    do_reading ();
+	  }
+	catch (const gdb_exception &exc)
+	  {
+	    m_failed = exc;
+	    set (cooked_state::CACHE_DONE);
+	  }
 
-    bfd_thread_cleanup ();
-  });
+	bfd_thread_cleanup ();
+      });
+    }
+  catch (const gdb_exception &exc)
+    {
+      m_failed = exc;
+      set (cooked_state::CACHE_DONE);
+    }
 }
 
 /* See cooked-index.h.  */
-- 
2.35.3

Re: [PATCH v2 1/5] [gdb] Don't create registry keys in destructor

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Creating a registry key using emplace calls new:
Tom> ...
Tom>       DATA *result = new DATA (std::forward<Args> (args)...);
Tom> ...
Tom> which can throw a bad alloc, which will terminate gdb if called from a
Tom> destructor.

I don't think gdb generally tries to be robust if allocation fails.

Tom>  static struct ada_tasks_pspace_data *
Tom> -get_ada_tasks_pspace_data (struct program_space *pspace)
Tom> +get_ada_tasks_pspace_data (struct program_space *pspace, bool create = true)
Tom>  {

In many of these cases, it is fine to just delete the data.

 
Tom>    data = ada_tasks_pspace_data_handle.get (pspace);
Tom>    if (data == NULL)

In cases where it is not, the caller can use .get() instead and do
nothing when the data hasn't been set.  IMO this is preferable to adding
optional 'bool create' flags.

Tom

Re: [PATCH v2 2/5] [gdb] Handle bad alloc handling in gdb_bfd_open

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this by reversing the order, inserting abfd into gdb_bfd_cache only after
Tom> a successful call to gdb_bfd_init_data, such that we get:
Tom> ...
Tom> $ gdb -q -batch -ex "file a.out" -ex "file a.out"
Tom> bad alloc

I believe this is fine.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH v2 1/5] [gdb] Don't create registry keys in destructor

[Tom de Vries]

On 10/17/24 21:09, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Creating a registry key using emplace calls new:
> Tom> ...
> Tom>       DATA *result = new DATA (std::forward<Args> (args)...);
> Tom> ...
> Tom> which can throw a bad alloc, which will terminate gdb if called from a
> Tom> destructor.
> 
> I don't think gdb generally tries to be robust if allocation fails.
> 

I suppose that is warned about with the "further debugging may prove 
unreliable" message, but I don't see any reason not to attempt to make 
things more robust.

> Tom>  static struct ada_tasks_pspace_data *
> Tom> -get_ada_tasks_pspace_data (struct program_space *pspace)
> Tom> +get_ada_tasks_pspace_data (struct program_space *pspace, bool create = true)
> Tom>  {
> 
> In many of these cases, it is fine to just delete the data.
> 

I'm not entirely sure what you mean, but having updated the patch to 
drop the create parameter, I hope it's no longer relevant.

> Tom>    data = ada_tasks_pspace_data_handle.get (pspace);
> Tom>    if (data == NULL)
> 
> In cases where it is not, the caller can use .get() instead and do
> nothing when the data hasn't been set.  IMO this is preferable to adding
> optional 'bool create' flags.

I've updated the patch this way, submitted a v2, not the whole series, 
just this patch ( 
https://sourceware.org/pipermail/gdb-patches/2024-October/212472.html ).

That approach didn't work seamlessly in objfile::~objfile.  I could have 
exported current_source_key in source.h to make that work, but instead I 
chose to introduce a clear_current_source_symtab_and_line variant.

Thanks,
- Tom

[pushed][PATCH v2 4/5] [gdb] Handle bad alloc in gdb_rl_callback_read_char_wrapper_noexcept

[Tom de Vries]

On 10/11/24 16:35, Tom de Vries wrote:
> Say we simulate a bad alloc in exceptions_state_mc_init:
> ...
>   jmp_buf *
>   exceptions_state_mc_init ()
>   {
> +  {
> +    static bool throw_bad_alloc = true;
> +    if (throw_bad_alloc)
> +      {
> +       throw_bad_alloc = false;
> +
> +       va_list dummy;
> +       throw gdb_quit_bad_alloc (gdb_exception_quit ("bad alloc", dummy));
> +      }
> +  }
>     catchers.emplace_front ();
>     return &catchers.front ().buf;
>   }
> ...
> 
> After starting gdb and typing "q", gdb terminates:
> ...
> $ gdb -q
> (gdb) terminate called after throwing an instance of 'gdb_quit_bad_alloc'
>    what():  std::bad_alloc
> ...
> because the bad alloc (thrown in TRY_SJLJ) is caught by the noexcept on
> gdb_rl_callback_read_char_wrapper_noexcept:
> ...
> static struct gdb_exception
> gdb_rl_callback_read_char_wrapper_noexcept () noexcept
> {
>    struct gdb_exception gdb_expt;
> 
>    /* C++ exceptions can't normally be thrown across readline (unless
>       it is built with -fexceptions, but it won't by default on many
>       ABIs).  So we instead wrap the readline call with a sjlj-based
>       TRY/CATCH, and rethrow the GDB exception once back in GDB.  */
>    TRY_SJLJ
> ...
> 
> Fix this by renaming gdb_rl_callback_read_char_wrapper_noexcept to
> gdb_rl_callback_read_char_wrapper_sjlj and calling it from a wrapper function
> that catches the bad alloc expection:
> ...
> static struct gdb_exception
> gdb_rl_callback_read_char_wrapper_noexcept () noexcept
> {
>    try
>      {
>        return gdb_rl_callback_read_char_wrapper_sjlj ();
>      }
>    catch (gdb_exception &ex)
>      {
>        return std::move (ex);
>      }
> }
> ...
> getting us instead:
> ...
> $ gdb -q
> (gdb) bad alloc
> (gdb) q
> ...
> 

Pushed.

Thanks,
- Tom

> Tested on aarch64-linux.
> ---
>   gdb/event-top.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index cdc7874b7fa..d3cf144958a 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -184,7 +184,7 @@ extern "C" void _rl_signal_handler (int);
>      (sjlj-based) C++ exceptions.  */
>   
>   static struct gdb_exception
> -gdb_rl_callback_read_char_wrapper_noexcept () noexcept
> +gdb_rl_callback_read_char_wrapper_sjlj ()
>   {
>     struct gdb_exception gdb_expt;
>   
> @@ -229,6 +229,22 @@ gdb_rl_callback_read_char_wrapper_noexcept () noexcept
>     return gdb_expt;
>   }
>   
> +/* Wrapper around gdb_rl_callback_read_char_wrapper_sjlj to ensure
> +   noexcept.  */
> +
> +static struct gdb_exception
> +gdb_rl_callback_read_char_wrapper_noexcept () noexcept
> +{
> +  try
> +    {
> +      return gdb_rl_callback_read_char_wrapper_sjlj ();
> +    }
> +  catch (gdb_exception &ex)
> +    {
> +      return std::move (ex);
> +    }
> +}
> +
>   static void
>   gdb_rl_callback_read_char_wrapper (gdb_client_data client_data)
>   {