gdb/dwarf: don't enqueue CU in maybe_queue_comp_unit if already expanded

The previous commit log described how items could be left lingering in
the dwarf2_per_bfd::queue and how that could cause trouble.

This patch fixes the issue by changing maybe_queue_comp_unit so that it
doesn't put a CU in the to-expand queue if that CU is already expanded.
This will make it so that when dwarf2_fetch_die_type_sect_off calls
follow_die_offset and maybe_queue_comp_unit, it won't enqueue the target
CU, because it will see the CU is already expanded.

This assumes that if a CU is dwarf2_fetch_die_type_sect_off's target CU,
it will have previously been expanded.  I think it is the case, but I
can't be 100% sure.  If that's not true, the assertions added in the
following patch will catch it, and it means we'll have to re-think a bit
more how things work (it wouldn't be well handled at all today anyway).

This fixes something else in maybe_queue_comp_unit that looks wrong.
Imagine the DIEs of a CU are loaded in memory, but that CU is not
expanded.  In that case, maybe_queue_comp_unit will use this early
return:

  /* If the compilation unit is already loaded, just mark it as
     used.  */
  dwarf2_cu *cu = per_objfile->get_cu (per_cu);
  if (cu != nullptr)
    {
      cu->last_used = 0;
      return 0;
    }

... so the CU won't be queued for expansion.  Whether the DIEs of a CU
are loaded in memory and whether that CU is expanded are two orthogonal
things, but that function appears to mix them.  So, move the queuing
above that check / early return, so that if the CU's DIEs are loaded in
memory but the CU is not expanded yet, it gets enqueued.

I tried to improve maybe_queue_comp_unit's documentation to clarify what
the return value means.  By clarifying this, I noticed that two callers
(follow_die_offset and follow_die_sig_1) access the CU's DIEs after
calling maybe_queue_comp_unit, only relying on maybe_queue_comp_unit's
return value to tell whether DIEs need to be loaded first or not.  As
explained in the new comment, this is problematic:
maybe_queue_comp_unit's return value doesn't tell whether DIEs are
currently loaded, it means whether maybe_queue_comp_unit requires the
caller to load them.  If the CU is already expanded but the DIEs to have
been freed, maybe_queue_comp_unit returns 0, meaning "I don't need you
to load the DIEs".  So if these two functions (follow_die_offset and
follow_die_sig_1) need to access the DIEs in any case, for their own
usage, they should make sure to load them if they are not loaded
already.  I therefore added an extra check to the condition they use,
making it so they will always load the DIEs if they aren't already.

From what I found, other callers don't care for the CU's DIEs, they call
maybe_queue_comp_unit to ensure the CU gets expanded eventually, but
don't care for it after that.

gdb/ChangeLog:

	PR gdb/26828
	* dwarf2/read.c (maybe_queue_comp_unit): Check if CU is expanded
	to decide whether or not to enqueue it for expansion.
	(follow_die_offset, follow_die_sig_1): Ensure we load the DIEs
	after calling maybe_queue_comp_unit.

Change-Id: Id98c6b60669f4b4b21b9be16d0518fc62bdf686a

2 files changed, 61 insertions, 17 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index da78e06..6aa71ea 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2021-02-23  Simon Marchi  <simon.marchi@polymtl.ca>
 
+	PR gdb/26828
+	* dwarf2/read.c (maybe_queue_comp_unit): Check if CU is expanded
+	to decide whether or not to enqueue it for expansion.
+	(follow_die_offset, follow_die_sig_1): Ensure we load the DIEs
+	after calling maybe_queue_comp_unit.
+
+2021-02-23  Simon Marchi  <simon.marchi@polymtl.ca>
+
 	* linux-nat.c (linux_nat_filter_event): Return void.
 
 2021-02-22  Tom Tromey  <tromey@adacore.com>
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 51bf0fb..b36694e 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -9183,14 +9183,30 @@ queue_comp_unit (dwarf2_per_cu_data *per_cu,
   per_cu->per_bfd->queue.emplace (per_cu, per_objfile, pretend_language);
 }
 
-/* If PER_CU is not yet queued, add it to the queue.
+/* If PER_CU is not yet expanded of queued for expansion, add it to the queue.
+
    If DEPENDENT_CU is non-NULL, it has a reference to PER_CU so add a
    dependency.
-   The result is non-zero if PER_CU was queued, otherwise the result is zero
-   meaning either PER_CU is already queued or it is already loaded.
 
-   N.B. There is an invariant here that if a CU is queued then it is loaded.
-   The caller is required to load PER_CU if we return non-zero.  */
+   Return true if maybe_queue_comp_unit requires the caller to load the CU's
+   DIEs, false otherwise.
+
+   Explanation: there is an invariant that if a CU is queued for expansion
+   (present in `dwarf2_per_bfd::queue`), then its DIEs are loaded
+   (a dwarf2_cu object exists for this CU, and `dwarf2_per_objfile::get_cu`
+   returns non-nullptr).  If the CU gets enqueued by this function but its DIEs
+   are not yet loaded, the the caller must load the CU's DIEs to ensure the
+   invariant is respected.
+
+   The caller is therefore not required to load the CU's DIEs (we return false)
+   if:
+
+     - the CU is already expanded, and therefore does not get enqueued
+     - the CU gets enqueued for expansion, but its DIEs are already loaded
+
+   Note that the caller should not use this function's return value as an
+   indicator of whether the CU's DIEs are loaded right now, it should check
+   that by calling `dwarf2_per_objfile::get_cu` instead.  */
 
 static int
 maybe_queue_comp_unit (struct dwarf2_cu *dependent_cu,
@@ -9221,22 +9237,32 @@ maybe_queue_comp_unit (struct dwarf2_cu *dependent_cu,
       /* Verify the invariant that if a CU is queued for expansion, its DIEs are
 	 loaded.  */
       gdb_assert (per_objfile->get_cu (per_cu) != nullptr);
+
+      /* If the CU is queued for expansion, it should not already be
+	 expanded.  */
+      gdb_assert (!per_objfile->symtab_set_p (per_cu));
+
+      /* The DIEs are already loaded, the caller doesn't need to do it.  */
       return 0;
     }
 
+  bool queued = false;
+  if (!per_objfile->symtab_set_p (per_cu))
+    {
+      /* Add it to the queue.  */
+      queue_comp_unit (per_cu, per_objfile,  pretend_language);
+      queued = true;
+    }
+
   /* If the compilation unit is already loaded, just mark it as
      used.  */
   dwarf2_cu *cu = per_objfile->get_cu (per_cu);
   if (cu != nullptr)
-    {
-      cu->last_used = 0;
-      return 0;
-    }
+    cu->last_used = 0;
 
-  /* Add it to the queue.  */
-  queue_comp_unit (per_cu, per_objfile,  pretend_language);
-
-  return 1;
+  /* Ask the caller to load the CU's DIEs if the CU got enqueued for expansion
+     and the DIEs are not already loaded.  */
+  return queued && cu == nullptr;
 }
 
 /* Process the queue.  */
@@ -23704,12 +23730,18 @@ follow_die_offset (sect_offset sect_off, int offset_in_dwz,
 				 sect_offset_str (per_cu->sect_off),
 				 per_objfile->get_cu (per_cu) != nullptr);
 
-      /* If necessary, add it to the queue and load its DIEs.  */
-      if (maybe_queue_comp_unit (cu, per_cu, per_objfile, cu->language))
+      /* If necessary, add it to the queue and load its DIEs.
+
+	 Even if maybe_queue_comp_unit doesn't require us to load the CU's DIEs,
+	 it doesn't mean they are currently loaded.  Since we require them
+	 to be loaded, we must check for ourselves.  */
+      if (maybe_queue_comp_unit (cu, per_cu, per_objfile, cu->language)
+	  || per_objfile->get_cu (per_cu) == nullptr)
 	load_full_comp_unit (per_cu, per_objfile, per_objfile->get_cu (per_cu),
 			     false, cu->language);
 
       target_cu = per_objfile->get_cu (per_cu);
+      gdb_assert (target_cu != nullptr);
     }
   else if (cu->dies == NULL)
     {
@@ -24083,10 +24115,14 @@ follow_die_sig_1 (struct die_info *src_die, struct signatured_type *sig_type,
      we can get here for DW_AT_imported_declaration where we need
      the DIE not the type.  */
 
-  /* If necessary, add it to the queue and load its DIEs.  */
+  /* If necessary, add it to the queue and load its DIEs.
 
+     Even if maybe_queue_comp_unit doesn't require us to load the CU's DIEs,
+     it doesn't mean they are currently loaded.  Since we require them
+     to be loaded, we must check for ourselves.  */
   if (maybe_queue_comp_unit (*ref_cu, &sig_type->per_cu, per_objfile,
-			     language_minimal))
+			     language_minimal)
+      || per_objfile->get_cu (&sig_type->per_cu) == nullptr)
     read_signatured_type (sig_type, per_objfile);
 
   sig_cu = per_objfile->get_cu (&sig_type->per_cu);