[RFA,5/6] Add validity bits for psymtab high and low fields

Message ID 20180503223621.22544-6-tom@tromey.com
State New
Headers show
Series
  • Make psymbols independent of the progspace
Related show

Commit Message

Tom Tromey May 3, 2018, 10:36 p.m.
Right now some psymtab code checks whether a psymtab's textlow or
texthigh fields are valid by comparing against 0.

I imagine this is mildly wrong in the current environment, but once
psymtabs are relocated dynamically, it will no longer be correct,
because it will be much more normal to see a psymtab with a textlow of
zero -- this will just mean it appears at the start of the text
section.

This patch introduces validity bits to handle this situation more
nicely, and changes users of the code to follow.

ChangeLog
2018-05-03  Tom Tromey  <tromey@redhat.com>

	* dbxread.c (end_psymtab): Use texthigh_valid and textlow_valid.
	* mdebugread.c (parse_partial_symbols): Use textlow_valid.
	(psymtab_to_symtab_1): Use texthigh_valid and textlow_valid.
	* psympriv.h (struct partial_symtab) <textlow_, texthigh_>: Update
	comment.
	<textlow_valid, texthigh_valid>: New fields.
	(set_psymtab_textlow, set_psymtab_texthigh): New inline functions.
	(SET_PSYMTAB_TEXTLOW, SET_PSYMTAB_TEXTHIGH): Redefine.
	* xcoffread.c (scan_xcoff_symtab): Use textlow_valid.
---
 gdb/ChangeLog    | 12 ++++++++++++
 gdb/dbxread.c    |  6 ++----
 gdb/mdebugread.c |  8 ++++----
 gdb/psympriv.h   | 33 ++++++++++++++++++++++++++++++---
 gdb/xcoffread.c  |  2 +-
 5 files changed, 49 insertions(+), 12 deletions(-)

-- 
2.13.6

Comments

Keith Seitz June 1, 2018, 9:22 p.m. | #1
On 05/03/2018 03:36 PM, Tom Tromey wrote:
> Right now some psymtab code checks whether a psymtab's textlow or

> texthigh fields are valid by comparing against 0.

> 

> I imagine this is mildly wrong in the current environment, but once

> psymtabs are relocated dynamically, it will no longer be correct,

> because it will be much more normal to see a psymtab with a textlow of

> zero -- this will just mean it appears at the start of the text

> section.

> 


Also wrong if 0 is a valid address! [as a recent patch* serves to remind]

> This patch introduces validity bits to handle this situation more

> nicely, and changes users of the code to follow.

> 

> ChangeLog

> 2018-05-03  Tom Tromey  <tromey@redhat.com>

> 

> 	* dbxread.c (end_psymtab): Use texthigh_valid and textlow_valid.


Does this line not also qualify for texthigh_valid:

  if (PSYMTAB_TEXTHIGH (pst) == 0 && last_function_name
      && gdbarch_sofun_address_maybe_missing (gdbarch))

?

There's also this line in dbx_read_symtab that might qualify (from case N_SCOPE):

	      valu = nlist.n_value + last_function_start;
	      if (PSYMTAB_TEXTHIGH (pst) == 0 || valu > PSYMTAB_TEXTHIGH (pst))
		SET_PSYMTAB_TEXTHIGH (pst, valu);
	      break;

Also in this function, can textlow_not_set be replaced by textlow_valid? [The same with the textlow_not_set parameter to dbx_end_psymtab?]

> 	* mdebugread.c (parse_partial_symbols): Use textlow_valid.

> 	(psymtab_to_symtab_1): Use texthigh_valid and textlow_valid.

> 	* psympriv.h (struct partial_symtab) <textlow_, texthigh_>: Update

> 	comment.

> 	<textlow_valid, texthigh_valid>: New fields.

> 	(set_psymtab_textlow, set_psymtab_texthigh): New inline functions.

> 	(SET_PSYMTAB_TEXTLOW, SET_PSYMTAB_TEXTHIGH): Redefine.

> 	* xcoffread.c (scan_xcoff_symtab): Use textlow_valid.


Otherwise, this all LGTM (IANAM).

Keith

* https://sourceware.org/ml/gdb-patches/2018-05/msg00325.html
Tom Tromey June 5, 2018, 5:25 p.m. | #2
>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:


>> * dbxread.c (end_psymtab): Use texthigh_valid and textlow_valid.


Keith> Does this line not also qualify for texthigh_valid:
Keith>   if (PSYMTAB_TEXTHIGH (pst) == 0 && last_function_name
Keith>       && gdbarch_sofun_address_maybe_missing (gdbarch))
Keith> ?

This is a bit obscure, and I'm not totally sure it is correct.  The idea
is that this only happens in a pathological case, which we want to
detect.  There is a comment there:

  /* Under Solaris, the N_SO symbols always have a value of 0,
     instead of the usual address of the .o file.  Therefore,
     we have to do some tricks to fill in texthigh and textlow.
..


Also the final patch in this series changes this line to:

  if (PSYMTAB_RAW_TEXTHIGH (pst) == 0 && last_function_name
      && gdbarch_sofun_address_maybe_missing (gdbarch))


Keith> There's also this line in dbx_read_symtab that might qualify (from case N_SCOPE):

Keith> 	      valu = nlist.n_value + last_function_start;
Keith> 	      if (PSYMTAB_TEXTHIGH (pst) == 0 || valu > PSYMTAB_TEXTHIGH (pst))
Keith> 		SET_PSYMTAB_TEXTHIGH (pst, valu);
Keith> 	      break;

This also gets changed to use the RAW_ forms.

Keith> Also in this function, can textlow_not_set be replaced by
Keith> textlow_valid? [The same with the textlow_not_set parameter to
Keith> dbx_end_psymtab?]

I think it is a good idea, but there is one spot setting textlow_not_set
without invoking SET_PSYMTAB_TEXTLOW.  From the N_SO case:

	    prev_textlow_not_set = textlow_not_set;

	    /* A zero value is probably an indication for the SunPRO 3.0
	       compiler.  dbx_end_psymtab explicitly tests for zero, so
	       don't relocate it.  */

	    if (nlist.n_value == 0
		&& gdbarch_sofun_address_maybe_missing (gdbarch))
	      {
		textlow_not_set = 1;
		valu = 0;
	      }
	    else
	      textlow_not_set = 0;

I don't know anything about stabs, so my basic idea in this area was to
make the changes as minimal as possible.

Tom
Keith Seitz June 5, 2018, 5:38 p.m. | #3
On 06/05/2018 10:25 AM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

> Also the final patch in this series changes this line to:

> 

>   if (PSYMTAB_RAW_TEXTHIGH (pst) == 0 && last_function_name

>       && gdbarch_sofun_address_maybe_missing (gdbarch))

> 


Sorry, I missed that. In that case, ignore my comment.

> Keith> There's also this line in dbx_read_symtab that might qualify (from case N_SCOPE):

> 

> Keith> 	      valu = nlist.n_value + last_function_start;

> Keith> 	      if (PSYMTAB_TEXTHIGH (pst) == 0 || valu > PSYMTAB_TEXTHIGH (pst))

> Keith> 		SET_PSYMTAB_TEXTHIGH (pst, valu);

> Keith> 	      break;

> 

> This also gets changed to use the RAW_ forms.


Likewise.

> Keith> Also in this function, can textlow_not_set be replaced by

> Keith> textlow_valid? [The same with the textlow_not_set parameter to

> Keith> dbx_end_psymtab?]

> 

> I think it is a good idea, but there is one spot setting textlow_not_set

> without invoking SET_PSYMTAB_TEXTLOW.  From the N_SO case:

> 

> 	    prev_textlow_not_set = textlow_not_set;

> 

> 	    /* A zero value is probably an indication for the SunPRO 3.0

> 	       compiler.  dbx_end_psymtab explicitly tests for zero, so

> 	       don't relocate it.  */

> 

> 	    if (nlist.n_value == 0

> 		&& gdbarch_sofun_address_maybe_missing (gdbarch))

> 	      {

> 		textlow_not_set = 1;

> 		valu = 0;

> 	      }

> 	    else

> 	      textlow_not_set = 0;

> 

> I don't know anything about stabs, so my basic idea in this area was to

> make the changes as minimal as possible.


I should have looked a little more closely at the viability of my suggestion. I agree with you, erring on the side of minimal invasion in "obscure" code is always prudent.

Thank you for the explanations.

Keith

Patch

diff --git a/gdb/dbxread.c b/gdb/dbxread.c
index 810bdcf57b..da675abb68 100644
--- a/gdb/dbxread.c
+++ b/gdb/dbxread.c
@@ -2100,13 +2100,11 @@  dbx_end_psymtab (struct objfile *objfile, struct partial_symtab *pst,
       /* If we know our own starting text address, then walk through all other
          psymtabs for this objfile, and if any didn't know their ending text
          address, set it to our starting address.  Take care to not set our
-         own ending address to our starting address, nor to set addresses on
-         `dependency' files that have both textlow and texthigh zero.  */
+         own ending address to our starting address.  */
 
       ALL_OBJFILE_PSYMTABS (objfile, p1)
       {
-	if (PSYMTAB_TEXTHIGH (p1) == 0 && PSYMTAB_TEXTLOW (p1) != 0
-	    && p1 != pst)
+	if (!p1->texthigh_valid && p1->textlow_valid && p1 != pst)
 	  SET_PSYMTAB_TEXTHIGH (p1, PSYMTAB_TEXTLOW (pst));
       }
     }
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index 09bdb28088..fb8318a831 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -2764,7 +2764,7 @@  parse_partial_symbols (minimal_symbol_reader &reader,
 
 			  /* Kludge for Irix 5.2 zero fh->adr.  */
 			  if (!relocatable
-			      && (PSYMTAB_TEXTLOW (pst) == 0
+			      && (!pst->textlow_valid
 				  || procaddr < PSYMTAB_TEXTLOW (pst)))
 			    SET_PSYMTAB_TEXTLOW (pst, procaddr);
 			  if (high > PSYMTAB_TEXTHIGH (pst))
@@ -3543,7 +3543,7 @@  parse_partial_symbols (minimal_symbol_reader &reader,
 
 		  /* Kludge for Irix 5.2 zero fh->adr.  */
 		  if (!relocatable
-		      && (PSYMTAB_TEXTLOW (pst) == 0
+		      && (!pst->textlow_valid
 			  || procaddr < PSYMTAB_TEXTLOW (pst)))
 		    SET_PSYMTAB_TEXTLOW (pst, procaddr);
 
@@ -3742,7 +3742,7 @@  parse_partial_symbols (minimal_symbol_reader &reader,
          other cases.  */
       save_pst = fdr_to_pst[f_idx].pst;
       if (save_pst != NULL
-	  && PSYMTAB_TEXTLOW (save_pst) != 0
+	  && save_pst->textlow_valid
 	  && !(objfile->flags & OBJF_REORDERED))
 	{
 	  ALL_OBJFILE_PSYMTABS (objfile, pst)
@@ -3957,7 +3957,7 @@  psymtab_to_symtab_1 (struct objfile *objfile,
   /* Do nothing if this is a dummy psymtab.  */
 
   if (pst->n_global_syms == 0 && pst->n_static_syms == 0
-      && PSYMTAB_TEXTLOW (pst) == 0 && PSYMTAB_TEXTHIGH (pst) == 0)
+      && !pst->textlow_valid && !pst->texthigh_valid)
     return;
 
   /* Now read the symbols for this symtab.  */
diff --git a/gdb/psympriv.h b/gdb/psympriv.h
index fa175736ce..8ce3c6b2a9 100644
--- a/gdb/psympriv.h
+++ b/gdb/psympriv.h
@@ -125,7 +125,9 @@  struct partial_symtab
   /* Range of text addresses covered by this file; texthigh is the
      beginning of the next section.  Do not use if PSYMTABS_ADDRMAP_SUPPORTED
      is set.  Do not refer directly to these fields.  Instead, use the
-     accessor macros.  */
+     accessor macros.  The validity of these fields is determined by the
+     textlow_valid and texthigh_valid fields; these are located later
+     in this structure for better packing.  */
 
   CORE_ADDR textlow_;
   CORE_ADDR texthigh_;
@@ -211,6 +213,11 @@  struct partial_symtab
 
   ENUM_BITFIELD (psymtab_search_status) searched_flag : 2;
 
+  /* Validity of the textlow_ and texthigh_ fields.  */
+
+  unsigned int textlow_valid : 1;
+  unsigned int texthigh_valid : 1;
+
   /* Pointer to compunit eventually allocated for this source file, 0 if
      !readin or if we haven't looked for the symtab after it was readin.  */
 
@@ -239,8 +246,28 @@  struct partial_symtab
 #define PSYMTAB_TEXTLOW(PST) ((PST)->textlow_ + 0)
 #define PSYMTAB_TEXTHIGH(PST) ((PST)->texthigh_ + 0)
 
-#define SET_PSYMTAB_TEXTLOW(PST, V) (((PST)->textlow_) = (V))
-#define SET_PSYMTAB_TEXTHIGH(PST, V) (((PST)->texthigh_) = (V))
+/* Set the "textlow" field on the partial symbol table, and mark the
+   field as valid.  */
+
+static inline void
+set_psymtab_textlow (struct partial_symtab *pst, CORE_ADDR low)
+{
+  pst->textlow_ = low;
+  pst->textlow_valid = 1;
+}
+
+/* Set the "texthigh" field on the partial symbol table, and mark the
+   field as valid.  */
+
+static inline void
+set_psymtab_texthigh (struct partial_symtab *pst, CORE_ADDR high)
+{
+  pst->texthigh_ = high;
+  pst->texthigh_valid = 1;
+}
+
+#define SET_PSYMTAB_TEXTLOW(PST, V) set_psymtab_textlow ((PST), (V))
+#define SET_PSYMTAB_TEXTHIGH(PST, V) set_psymtab_texthigh ((PST), (V))
 
 /* Add any kind of symbol to a partial_symbol vector.  */
 
diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
index 49716a56e5..25df119d51 100644
--- a/gdb/xcoffread.c
+++ b/gdb/xcoffread.c
@@ -2353,7 +2353,7 @@  scan_xcoff_symtab (minimal_symbol_reader &reader,
 
 			if (highval > PSYMTAB_TEXTHIGH (pst))
 			  SET_PSYMTAB_TEXTHIGH (pst, highval);
-			if (PSYMTAB_TEXTLOW (pst) == 0
+			if (!pst->textlow_valid
 			    || symbol.n_value < PSYMTAB_TEXTLOW (pst))
 			  SET_PSYMTAB_TEXTLOW (pst, symbol.n_value);
 		      }