public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Tom Tromey <tom@tromey.com>
To: gdb-patches@sourceware.org
Cc: Tom Tromey <tom@tromey.com>
Subject: [PATCH] Fix line table regression
Date: Fri, 17 Mar 2023 07:14:37 -0600	[thread overview]
Message-ID: <20230317131437.1852342-1-tom@tromey.com> (raw)

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


             reply	other threads:[~2023-03-17 13:14 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-17 13:14 Tom Tromey [this message]
2023-03-17 15:21 ` Simon Marchi
2023-03-17 22:17   ` Tom Tromey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230317131437.1852342-1-tom@tromey.com \
    --to=tom@tromey.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).