[PATCH] Fix line table regression

[PATCH] Fix line table regression

[Tom Tromey]

Simon pointed out a line table regression, and after a couple of false
starts, I was able to reproduce it by hand using his instructions.

The bug is that most of the code in do_mixed_source_and_assembly uses
unrelocated addresses, but one spot does:

  pc = low;

... after the text offset has been removed.

This patch fixes the problem by introducing a new type to represent
unrelocated addresses in the line table.  This prevents this sort of
bug to some degree (it's still possible to manipulate a CORE_ADDR in a
bad way, this is unavoidable).

However, this did let the compiler flag a few spots in that function,
and now it's not possible to compare an unrelocated address from a
line table with an ordinary CORE_ADDR.

Regression tested on x86-64 Fedora 36, though note this setup never
reproduced the bug in the first place.  I also tested it by hand on
the disasm-optim test program.
---
 gdb/buildsym-legacy.c |  2 +-
 gdb/buildsym-legacy.h |  3 ++-
 gdb/buildsym.c        |  2 +-
 gdb/buildsym.h        |  2 +-
 gdb/coffread.c        | 10 ++++++----
 gdb/dbxread.c         | 21 ++++++++++++---------
 gdb/disasm.c          | 24 +++++++++++++++---------
 gdb/dwarf2/read.c     |  3 ++-
 gdb/jit.c             |  2 +-
 gdb/mdebugread.c      |  8 +++++---
 gdb/record-btrace.c   |  5 +++--
 gdb/symmisc.c         |  2 +-
 gdb/symtab.c          | 27 ++++++++++++++++-----------
 gdb/symtab.h          | 12 +++++++++---
 gdb/xcoffread.c       |  5 +++--
 15 files changed, 78 insertions(+), 50 deletions(-)

diff --git a/gdb/buildsym-legacy.c b/gdb/buildsym-legacy.c
index cb733e7101b..131d24fe9b5 100644
--- a/gdb/buildsym-legacy.c
+++ b/gdb/buildsym-legacy.c
@@ -205,7 +205,7 @@ finish_block (struct symbol *symbol, struct pending_block *old_blocks,
 }
 
 void
-record_line (struct subfile *subfile, int line, CORE_ADDR pc)
+record_line (struct subfile *subfile, int line, unrelocated_addr pc)
 {
   gdb_assert (buildsym_compunit != nullptr);
   /* Assume every line entry is a statement start, that is a good place to
diff --git a/gdb/buildsym-legacy.h b/gdb/buildsym-legacy.h
index 3d705a8beed..664d6320e54 100644
--- a/gdb/buildsym-legacy.h
+++ b/gdb/buildsym-legacy.h
@@ -76,7 +76,8 @@ extern struct context_stack *push_context (int desc, CORE_ADDR valu);
 
 extern struct context_stack pop_context ();
 
-extern void record_line (struct subfile *subfile, int line, CORE_ADDR pc);
+extern void record_line (struct subfile *subfile, int line,
+			 unrelocated_addr pc);
 
 extern struct compunit_symtab *start_compunit_symtab (struct objfile *objfile,
 						      const char *name,
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index 56f11dd167f..f000233dafa 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -629,7 +629,7 @@ buildsym_compunit::pop_subfile ()
 
 void
 buildsym_compunit::record_line (struct subfile *subfile, int line,
-				CORE_ADDR pc, linetable_entry_flags flags)
+				unrelocated_addr pc, linetable_entry_flags flags)
 {
   m_have_line_numbers = true;
 
diff --git a/gdb/buildsym.h b/gdb/buildsym.h
index 42fcd1fdb97..98dc8f02874 100644
--- a/gdb/buildsym.h
+++ b/gdb/buildsym.h
@@ -239,7 +239,7 @@ struct buildsym_compunit
 
   const char *pop_subfile ();
 
-  void record_line (struct subfile *subfile, int line, CORE_ADDR pc,
+  void record_line (struct subfile *subfile, int line, unrelocated_addr pc,
 		    linetable_entry_flags flags);
 
   struct compunit_symtab *get_compunit_symtab ()
diff --git a/gdb/coffread.c b/gdb/coffread.c
index 4da3799243b..fe1ee506fbe 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -1130,9 +1130,10 @@ coff_symtab_read (minimal_symbol_reader &reader,
 		 of the closing '}', and for which we do not have any
 		 other statement-line-number.  */
 	      if (fcn_last_line == 1)
-		record_line (get_current_subfile (), fcn_first_line,
-			     gdbarch_addr_bits_remove (gdbarch,
-						       fcn_first_line_addr));
+		record_line
+		  (get_current_subfile (), fcn_first_line,
+		   unrelocated_addr (gdbarch_addr_bits_remove (gdbarch,
+							       fcn_first_line_addr)));
 	      else
 		enter_linenos (fcn_line_ptr, fcn_first_line,
 			       fcn_last_line, objfile);
@@ -1460,7 +1461,8 @@ enter_linenos (file_ptr file_offset, int first_line,
 	  CORE_ADDR addr = lptr.l_addr.l_paddr;
 	  record_line (get_current_subfile (),
 		       first_line + L_LNNO32 (&lptr),
-		       gdbarch_addr_bits_remove (gdbarch, addr));
+		       unrelocated_addr (gdbarch_addr_bits_remove (gdbarch,
+								   addr)));
 	}
       else
 	break;
diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index 1e88121f11c..e5366ccd0d0 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2453,9 +2453,10 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 	    {
 	      CORE_ADDR addr = last_function_start + valu;
 
-	      record_line (get_current_subfile (), 0,
-			   gdbarch_addr_bits_remove (gdbarch, addr)
-			   - objfile->text_section_offset ());
+	      record_line
+		(get_current_subfile (), 0,
+		 unrelocated_addr (gdbarch_addr_bits_remove (gdbarch, addr)
+				   - objfile->text_section_offset ()));
 	    }
 
 	  within_function = 0;
@@ -2662,15 +2663,17 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
 	  CORE_ADDR addr = processing_gcc_compilation == 2 ?
 			   last_function_start : valu;
 
-	  record_line (get_current_subfile (), desc,
-		       gdbarch_addr_bits_remove (gdbarch, addr)
-		       - objfile->text_section_offset ());
+	  record_line
+	    (get_current_subfile (), desc,
+	     unrelocated_addr (gdbarch_addr_bits_remove (gdbarch, addr)
+			       - objfile->text_section_offset ()));
 	  sline_found_in_function = 1;
 	}
       else
-	record_line (get_current_subfile (), desc,
-		     gdbarch_addr_bits_remove (gdbarch, valu)
-		     - objfile->text_section_offset ());
+	record_line
+	  (get_current_subfile (), desc,
+	   unrelocated_addr (gdbarch_addr_bits_remove (gdbarch, valu)
+			     - objfile->text_section_offset ()));
       break;
 
     case N_BCOMM:
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 71d3b97258d..03cd4b7ee02 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -594,8 +594,11 @@ do_mixed_source_and_assembly_deprecated
     alloca (nlines * sizeof (struct deprecated_dis_line_entry));
 
   struct objfile *objfile = symtab->compunit ()->objfile ();
-  low -= objfile->text_section_offset ();
-  high -= objfile->text_section_offset ();
+
+  unrelocated_addr unrel_low
+    = unrelocated_addr (low - objfile->text_section_offset ());
+  unrelocated_addr unrel_high
+    = unrelocated_addr (high - objfile->text_section_offset ());
 
   /* Copy linetable entries for this function into our data
      structure, creating end_pc's and setting out_of_order as
@@ -603,11 +606,11 @@ do_mixed_source_and_assembly_deprecated
 
   /* First, skip all the preceding functions.  */
 
-  for (i = 0; i < nlines - 1 && le[i].raw_pc () < low; i++);
+  for (i = 0; i < nlines - 1 && le[i].raw_pc () < unrel_low; i++);
 
   /* Now, copy all entries before the end of this function.  */
 
-  for (; i < nlines - 1 && le[i].raw_pc () < high; i++)
+  for (; i < nlines - 1 && le[i].raw_pc () < unrel_high; i++)
     {
       if (le[i] == le[i + 1])
 	continue;		/* Ignore duplicates.  */
@@ -627,7 +630,7 @@ do_mixed_source_and_assembly_deprecated
   /* If we're on the last line, and it's part of the function,
      then we need to get the end pc in a special way.  */
 
-  if (i == nlines - 1 && le[i].raw_pc () < high)
+  if (i == nlines - 1 && le[i].raw_pc () < unrel_high)
     {
       mle[newlines].line = le[i].line;
       mle[newlines].start_pc = le[i].pc (objfile);
@@ -739,8 +742,11 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch,
   htab_up dis_line_table (allocate_dis_line_table ());
 
   struct objfile *objfile = main_symtab->compunit ()->objfile ();
-  low -= objfile->text_section_offset ();
-  high -= objfile->text_section_offset ();
+
+  unrelocated_addr unrel_low
+    = unrelocated_addr (low - objfile->text_section_offset ());
+  unrelocated_addr unrel_high
+    = unrelocated_addr (high - objfile->text_section_offset ());
 
   pc = low;
 
@@ -755,10 +761,10 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch,
   first_le = NULL;
 
   /* Skip all the preceding functions.  */
-  for (i = 0; i < nlines && le[i].raw_pc () < low; i++)
+  for (i = 0; i < nlines && le[i].raw_pc () < unrel_low; i++)
     continue;
 
-  if (i < nlines && le[i].raw_pc () < high)
+  if (i < nlines && le[i].raw_pc () < unrel_high)
     first_le = &le[i];
 
   /* Add lines for every pc value.  */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 138537cfefd..a1c75655ff8 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18209,7 +18209,8 @@ dwarf_record_line_1 (struct gdbarch *gdbarch, struct subfile *subfile,
 		     linetable_entry_flags flags,
 		     struct dwarf2_cu *cu)
 {
-  CORE_ADDR addr = gdbarch_addr_bits_remove (gdbarch, address);
+  unrelocated_addr addr
+    = unrelocated_addr (gdbarch_addr_bits_remove (gdbarch, address));
 
   if (dwarf_line_debug)
     {
diff --git a/gdb/jit.c b/gdb/jit.c
index a93813b3f7f..52b65746f11 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -491,7 +491,7 @@ jit_symtab_line_mapping_add_impl (struct gdb_symbol_callbacks *cb,
   stab->linetable->nitems = nlines;
   for (i = 0; i < nlines; i++)
     {
-      stab->linetable->item[i].set_raw_pc ((CORE_ADDR) map[i].pc);
+      stab->linetable->item[i].set_raw_pc (unrelocated_addr (map[i].pc));
       stab->linetable->item[i].line = map[i].line;
       stab->linetable->item[i].is_stmt = true;
     }
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 13dc7899d44..bc466096d61 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -3982,8 +3982,10 @@ mdebug_expand_psymtab (legacy_psymtab *pst, struct objfile *objfile)
 	      else
 		{
 		  /* Handle encoded stab line number.  */
-		  record_line (get_current_subfile (), sh.index,
-			       gdbarch_addr_bits_remove (gdbarch, valu));
+		  record_line
+		    (get_current_subfile (), sh.index,
+		     unrelocated_addr (gdbarch_addr_bits_remove (gdbarch,
+								 valu)));
 		}
 	    }
 	  else if (sh.st == stProc || sh.st == stStaticProc
@@ -4513,7 +4515,7 @@ add_line (struct linetable *lt, int lineno, CORE_ADDR adr, int last)
     return lineno;
 
   lt->item[lt->nitems].line = lineno;
-  lt->item[lt->nitems++].set_raw_pc (adr << 2);
+  lt->item[lt->nitems++].set_raw_pc (unrelocated_addr (adr << 2));
   return lineno;
 }
 \f
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 3e71c6c9a10..2d88e4d20bf 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -724,7 +724,8 @@ btrace_find_line_range (CORE_ADDR pc)
     return btrace_mk_line_range (symtab, 0, 0);
 
   struct objfile *objfile = symtab->compunit ()->objfile ();
-  pc -= objfile->text_section_offset ();
+  unrelocated_addr unrel_pc
+    = unrelocated_addr (pc - objfile->text_section_offset ());
 
   range = btrace_mk_line_range (symtab, 0, 0);
   for (i = 0; i < nlines - 1; i++)
@@ -737,7 +738,7 @@ btrace_find_line_range (CORE_ADDR pc)
 	 possibly adding more line numbers to the range.  At the time this
 	 change was made I was unsure how to test this so chose to go with
 	 maintaining the existing experience.  */
-      if ((lines[i].raw_pc () == pc) && (lines[i].line != 0)
+      if (lines[i].raw_pc () == unrel_pc && lines[i].line != 0
 	  && lines[i].is_stmt)
 	range = btrace_line_range_add (range, lines[i].line);
     }
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 2f1e4f5b784..54dc570d282 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -996,7 +996,7 @@ maintenance_print_one_line_table (struct symtab *symtab, void *data)
 	  else
 	    uiout->field_string ("line", _("END"));
 	  uiout->field_core_addr ("address", objfile->arch (),
-				  item->raw_pc ());
+				  CORE_ADDR (item->raw_pc ()));
 	  uiout->field_string ("is-stmt", item->is_stmt ? "Y" : "");
 	  uiout->field_string ("prologue-end", item->prologue_end ? "Y" : "");
 	  uiout->text ("\n");
diff --git a/gdb/symtab.c b/gdb/symtab.c
index e11f9262a22..8ab1f58affe 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -332,7 +332,7 @@ search_domain_name (enum search_domain e)
 CORE_ADDR
 linetable_entry::pc (const struct objfile *objfile) const
 {
-  return m_pc + objfile->text_section_offset ();
+  return CORE_ADDR (m_pc) + objfile->text_section_offset ();
 }
 
 /* See symtab.h.  */
@@ -3158,17 +3158,18 @@ find_pc_sect_line (CORE_ADDR pc, struct obj_section *section, int notcurrent)
 	  && (!alt || item->raw_pc () < alt->raw_pc ()))
 	alt = item;
 
-      auto pc_compare = [](const CORE_ADDR & comp_pc,
-			   const struct linetable_entry & lhs)->bool
+      auto pc_compare = [] (const unrelocated_addr &comp_pc,
+			    const struct linetable_entry & lhs)
       {
 	return comp_pc < lhs.raw_pc ();
       };
 
       const linetable_entry *first = item;
       const linetable_entry *last = item + len;
-      item = std::upper_bound (first, last,
-			       pc - objfile->text_section_offset (),
-			       pc_compare);
+      item = (std::upper_bound
+	      (first, last,
+	       unrelocated_addr (pc - objfile->text_section_offset ()),
+	       pc_compare));
       if (item != first)
 	prev = item - 1;		/* Found a matching item.  */
 
@@ -3689,18 +3690,22 @@ skip_prologue_using_linetable (CORE_ADDR func_addr)
       const linetable *linetable = prologue_sal.symtab->linetable ();
 
       struct objfile *objfile = prologue_sal.symtab->compunit ()->objfile ();
-      start_pc -= objfile->text_section_offset ();
-      end_pc -= objfile->text_section_offset ();
+
+      unrelocated_addr unrel_start
+	= unrelocated_addr (start_pc - objfile->text_section_offset ());
+      unrelocated_addr unrel_end
+	= unrelocated_addr (end_pc - objfile->text_section_offset ());
 
       auto it = std::lower_bound
-	(linetable->item, linetable->item + linetable->nitems, start_pc,
-	 [] (const linetable_entry &lte, CORE_ADDR pc) -> bool
+	(linetable->item, linetable->item + linetable->nitems, unrel_start,
+	 [] (const linetable_entry &lte, unrelocated_addr pc)
 	 {
 	   return lte.raw_pc () < pc;
 	 });
 
       for (;
-	   it < linetable->item + linetable->nitems && it->raw_pc () <= end_pc;
+	   (it < linetable->item + linetable->nitems
+	    && it->raw_pc () <= unrel_end);
 	   it++)
 	if (it->prologue_end)
 	  return {it->pc (objfile)};
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 005b64b5a08..2fd56ce21bd 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1540,6 +1540,12 @@ struct rust_vtable_symbol : public symbol
 };
 
 \f
+/* Like a CORE_ADDR, but not directly convertible.  This is used to
+   represent an unrelocated CORE_ADDR.  DEFINE_OFFSET_TYPE is not used
+   here because there's no need to add or subtract values of this
+   type.  */
+enum class unrelocated_addr : CORE_ADDR { };
+
 /* Each item represents a line-->pc (or the reverse) mapping.  This is
    somewhat more wasteful of space than one might wish, but since only
    the files which are actually debugged are read in to core, we don't
@@ -1548,11 +1554,11 @@ struct rust_vtable_symbol : public symbol
 struct linetable_entry
 {
   /* Set the (unrelocated) PC for this entry.  */
-  void set_raw_pc (CORE_ADDR pc)
+  void set_raw_pc (unrelocated_addr pc)
   { m_pc = pc; }
 
   /* Return the unrelocated PC for this entry.  */
-  CORE_ADDR raw_pc () const
+  unrelocated_addr raw_pc () const
   { return m_pc; }
 
   /* Return the relocated PC for this entry.  */
@@ -1582,7 +1588,7 @@ struct linetable_entry
   bool prologue_end : 1;
 
   /* The address for this entry.  */
-  CORE_ADDR m_pc;
+  unrelocated_addr m_pc;
 };
 
 /* The order of entries in the linetable is significant.  They should
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 72c0a0db49a..ab763760f43 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -795,11 +795,12 @@ enter_line_range (struct subfile *subfile, unsigned beginoffset,
       if (int_lnno.l_lnno == 0)
 	{
 	  *firstLine = read_symbol_lineno (int_lnno.l_addr.l_symndx);
-	  record_line (subfile, 0, record_addr);
+	  record_line (subfile, 0, unrelocated_addr (record_addr));
 	  --(*firstLine);
 	}
       else
-	record_line (subfile, *firstLine + int_lnno.l_lnno, record_addr);
+	record_line (subfile, *firstLine + int_lnno.l_lnno,
+		     unrelocated_addr (record_addr));
       curoffset += linesz;
     }
 }
-- 
2.39.1

Re: [PATCH] Fix line table regression

[Simon Marchi]

On 3/17/23 09:14, Tom Tromey wrote:
> Simon pointed out a line table regression, and after a couple of false
> starts, I was able to reproduce it by hand using his instructions.
> 
> The bug is that most of the code in do_mixed_source_and_assembly uses
> unrelocated addresses, but one spot does:
> 
>   pc = low;
> 
> ... after the text offset has been removed.
> 
> This patch fixes the problem by introducing a new type to represent
> unrelocated addresses in the line table.  This prevents this sort of
> bug to some degree (it's still possible to manipulate a CORE_ADDR in a
> bad way, this is unavoidable).
> 
> However, this did let the compiler flag a few spots in that function,
> and now it's not possible to compare an unrelocated address from a
> line table with an ordinary CORE_ADDR.
> 
> Regression tested on x86-64 Fedora 36, though note this setup never
> reproduced the bug in the first place.  I also tested it by hand on
> the disasm-optim test program.

It does fix the issue in do_mixed_source_and_assembly I have seen.

I like the idea of having a separate type to catch things at compile
time.  I would like for it to be used in other objects using unrelocated
addresses.  I'm just afraid that some code paths will always need to be
handle both relocated and unrelocated addresses, so the distinction
may not always be completely clear cut.  For instance, your patch shows
that gdbarch_addr_bits_remove works with both relocated and relocated
addresses.  Regardless, if it helps, without being a perfect solution
usable all the time, I think it's good.

If this becomes more widespread, I think it would be nice to have some
helper methods like this:

  unrelocated_addr objfile::unrelocated (CORE_ADDR);
  CORE_ADDR objfile::relocate (unrelocated_addr);

It would make the code a bit clearer about what the intention is, versus
just doing additions and subtractions.

Simon

Re: [PATCH] Fix line table regression

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> It does fix the issue in do_mixed_source_and_assembly I have seen.

I'm going to check it in.

Simon> I like the idea of having a separate type to catch things at compile
Simon> time.  I would like for it to be used in other objects using unrelocated
Simon> addresses.

I'll take a quick look.

Simon> I'm just afraid that some code paths will always need to be
Simon> handle both relocated and unrelocated addresses, so the distinction
Simon> may not always be completely clear cut.  For instance, your patch shows
Simon> that gdbarch_addr_bits_remove works with both relocated and relocated
Simon> addresses.

Yeah.  The contract on some of these things is a bit vague for my taste,
as well.

Simon> If this becomes more widespread, I think it would be nice to have some
Simon> helper methods like this:

Simon>   unrelocated_addr objfile::unrelocated (CORE_ADDR);
Simon>   CORE_ADDR objfile::relocate (unrelocated_addr);

Simon> It would make the code a bit clearer about what the intention is, versus
Simon> just doing additions and subtractions.

I think they'd have to take the section index as well, but yeah.

Tom