RFA: [symfile.c} Fix to symbol_file_add()

RFA: [symfile.c} Fix to symbol_file_add()

[Fernando Nasser]

Paul N. Hilfinger has pointed out to me that a few operations should be
done every time a new symbol file is read.  This was an oversight in a
patch I wrote in January.  The small patch attached fixes that.

ChangeLog:

	* symfile.c (symbol_file_command): Move cleanup operations
	from here...
	(symbol_file_add): ...to here, so they are run every time a new
	symbol file is read.

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.31
diff -c -p -r1.31 symfile.c
*** symfile.c	2001/04/05 02:02:13	1.31
--- symfile.c	2001/04/29 16:13:12
*************** symbol_file_add (char *name, int from_tt
*** 893,898 ****
--- 893,907 ----
    if (target_new_objfile_hook)
      target_new_objfile_hook (objfile);
  
+ #ifdef HPUXHPPA
+   RESET_HP_UX_GLOBALS ();
+ #endif
+   /* Getting new symbols may change our opinion about
+      what is frameless.  */
+   reinit_frame_cache ();
+ 
+   set_initial_language ();
+ 
    return (objfile);
  }
  
*************** symbol_file_command (char *args, int fro
*** 980,993 ****
  		{
                    name = *argv;
  		  symbol_file_add (name, from_tty, NULL, 1, flags);
- #ifdef HPUXHPPA
- 		  RESET_HP_UX_GLOBALS ();
- #endif
- 		  /* Getting new symbols may change our opinion about
- 		     what is frameless.  */
- 		  reinit_frame_cache ();
- 
- 		  set_initial_language ();
  		}
  	  argv++;
  	}
--- 989,994 ----
From nsd@redhat.com Mon Apr 30 09:32:00 2001
From: Nicholas Duffek <nsd@redhat.com>
To: gdb-patches@sourceware.cygnus.com
Cc: ezannoni@cygnus.com, phdm@macqel.be, kevinb@cygnus.com, Peter.Schauer@regent.e-technik.tu-muenchen.de, jimb@cygnus.com
Subject: [RFA] patch to skip bigtoc fixup code
Date: Mon, 30 Apr 2001 09:32:00 -0000
Message-id: <200104301626.MAA00208@nog.bosbc.com>
X-SW-Source: 2001-04/msg00274.html
Content-length: 8515

This patch is an improvement over the one I committed a couple of weeks
ago.  With the new patch, GDB works similarly to dbx:

  1. "step" silently steps through bigtoc fixup code.

  2. Numeric fixup code addresses resolve to symbols @FIX*.  For example,
     when debugging a cc1plus binary containing bigtoc fixup section @FIX1
     in address range 0x1001b614 through 0x1001bfc8, "x/i 0x1001b6fc"
     displays "<@FIX1+232>: addis r3,r2,1".

Without the patch, "step" stops at bigtoc fixup code because GDB detects
that it has stepped outside the current function into the middle of
another function.  A subsequent "step" would return to the original
function.

The patch uses some of the trampoline hooks in handle_inferior_event() to
tell GDB to skip bigtoc fixup code.  The hooks detect bigtoc fixup code by
looking for "@FIX" in the first 4 bytes of the code's function name.  "@"
symbols are no longer ignored by xcoffread.c; instead, they introduduce
new symbol tables and generate function minimal_symbols via the
misc_func_recorded mechanism.

ChangeLog:

	* config/rs6000/tm-rs6000.h (IN_SOLIB_RETURN_TRAMPOLINE): Define.
	(rs6000_in_solib_return_trampoline): Declare.
	* rs6000-tdep.c (rs6000_in_solib_return_trampoline): New
	function.
	(rs6000_skip_trampoline_code): Skip bigtoc fixup code.
	* xcoffread.c (read_xcoff_symtab): Perform the ISFCN function
	check after the CSECT check rather than before it.  Allocate
	separate symtabs for CSECTs whose names begin with '@'.
	(scan_xcoff_symtab): Don't ignore symbols beginning with '@'.
	Activate the misc_func_recorded mechanism for whose names begin
	with '@'.

Tested on powerpc-ibm-aix4.3.3.0.  Okay to apply?

Nicholas Duffek
<nsd@redhat.com>

[patch follows]

Index: gdb/config/rs6000/tm-rs6000.h
===================================================================
diff -up gdb/config/rs6000/tm-rs6000.h gdb/config/rs6000/tm-rs6000.h
--- gdb/config/rs6000/tm-rs6000.h	Fri Apr 27 13:41:03 2001
+++ gdb/config/rs6000/tm-rs6000.h	Fri Apr 27 12:09:00 2001
@@ -36,6 +36,13 @@ extern char *pc_load_segment_name (CORE_
 #undef CPLUS_MARKER
 #define CPLUS_MARKER '.'
 
+/* Return whether PC in function NAME is in code that should be skipped when
+   single-stepping.  */
+
+#define IN_SOLIB_RETURN_TRAMPOLINE(pc, name) \
+  rs6000_in_solib_return_trampoline (pc, name)
+extern int rs6000_in_solib_return_trampoline (CORE_ADDR, char *);
+
 /* If PC is in some function-call trampoline code, return the PC
    where the function itself actually starts.  If not, return NULL.  */
 
Index: gdb/rs6000-tdep.c
===================================================================
diff -up gdb/rs6000-tdep.c gdb/rs6000-tdep.c
--- gdb/rs6000-tdep.c	Fri Apr 27 13:41:08 2001
+++ gdb/rs6000-tdep.c	Fri Apr 27 13:40:52 2001
@@ -1045,19 +1045,55 @@ rs6000_extract_return_value (struct type
 
 static CORE_ADDR rs6000_struct_return_address;
 
-/* Indirect function calls use a piece of trampoline code to do context
-   switching, i.e. to set the new TOC table. Skip such code if we are on
-   its first instruction (as when we have single-stepped to here). 
-   Also skip shared library trampoline code (which is different from
+/* Return whether handle_inferior_event() should proceed through code
+   starting at PC in function NAME when stepping.
+
+   The AIX -bbigtoc linker option generates functions @FIX0, @FIX1, etc. to
+   handle memory references that are too distant to fit in instructions
+   generated by the compiler.  For example, if 'foo' in the following
+   instruction:
+
+     lwz r9,foo(r2)
+
+   is greater than 32767, the linker might replace the lwz with a branch to
+   somewhere in @FIX1 that does the load in 2 instructions and then branches
+   back to where execution should continue.
+
+   GDB should silently step over @FIX code, just like AIX dbx does.
+   Unfortunately, the linker uses the "b" instruction for the branches,
+   meaning that the link register doesn't get set.  Therefore, GDB's usual
+   step_over_function() mechanism won't work.
+
+   Instead, use the IN_SOLIB_RETURN_TRAMPOLINE and SKIP_TRAMPOLINE_CODE hooks
+   in handle_inferior_event() to skip past @FIX code.  */
+
+int
+rs6000_in_solib_return_trampoline (CORE_ADDR pc, char *name)
+{
+  return name && !strncmp (name, "@FIX", 4);
+}
+
+/* Skip code that the user doesn't want to see when stepping:
+
+   1. Indirect function calls use a piece of trampoline code to do context
+   switching, i.e. to set the new TOC table.  Skip such code if we are on
+   its first instruction (as when we have single-stepped to here).
+
+   2. Skip shared library trampoline code (which is different from
    indirect function call trampolines).
+
+   3. Skip bigtoc fixup code.
+
    Result is desired PC to step until, or NULL if we are not in
-   trampoline code.  */
+   code that should be skipped.  */
 
 CORE_ADDR
 rs6000_skip_trampoline_code (CORE_ADDR pc)
 {
   register unsigned int ii, op;
+  int rel;
   CORE_ADDR solib_target_pc;
+  struct minimal_symbol *msymbol;
 
   static unsigned trampoline_code[] =
   {
@@ -1070,6 +1106,21 @@ rs6000_skip_trampoline_code (CORE_ADDR p
     0x4e800020,			/*    br                */
     0
   };
+
+  /* Check for bigtoc fixup code.  */
+  msymbol = lookup_minimal_symbol_by_pc (pc);
+  if (msymbol && rs6000_in_solib_return_trampoline (pc, SYMBOL_NAME (msymbol)))
+    {
+      /* Double-check that the third instruction from PC is relative "b".  */
+      op = read_memory_integer (pc + 8, 4);
+      if ((op & 0xfc000003) == 0x48000000)
+	{
+	  /* Extract bits 6-29 as a signed 24-bit relative word address and
+	     add it to the containing PC.  */
+	  rel = ((int)(op << 6) >> 6);
+	  return pc + 8 + rel;
+	}
+    }
 
   /* If pc is in a shared library trampoline, return its target.  */
   solib_target_pc = find_solib_trampoline_target (pc);
Index: gdb/xcoffread.c
===================================================================
diff -up gdb/xcoffread.c gdb/xcoffread.c
--- gdb/xcoffread.c	Fri Apr 27 13:41:13 2001
+++ gdb/xcoffread.c	Fri Apr 27 12:52:10 2001
@@ -1100,14 +1100,6 @@ read_xcoff_symtab (pst)
 	  /* done with all files, everything from here on is globals */
 	}
 
-      /* if explicitly specified as a function, treat is as one. */
-      if (ISFCN (cs->c_type) && cs->c_sclass != C_TPDEF)
-	{
-	  bfd_coff_swap_aux_in (abfd, raw_auxptr, cs->c_type, cs->c_sclass,
-				0, cs->c_naux, &main_aux);
-	  goto function_entry_point;
-	}
-
       if ((cs->c_sclass == C_EXT || cs->c_sclass == C_HIDEXT)
 	  && cs->c_naux == 1)
 	{
@@ -1177,7 +1169,8 @@ read_xcoff_symtab (pst)
 						SECT_OFF_TEXT (objfile));
 		      file_end_addr = file_start_addr + CSECT_LEN (&main_aux);
 
-		      if (cs->c_name && cs->c_name[0] == '.')
+		      if (cs->c_name && (cs->c_name[0] == '.'
+					 || cs->c_name[0] == '@'))
 			{
 			  last_csect_name = cs->c_name;
 			  last_csect_val = cs->c_value;
@@ -1251,6 +1244,16 @@ read_xcoff_symtab (pst)
 	    }
 	}
 
+      /* If explicitly specified as a function, treat is as one.  This check
+	 evaluates to true for @FIX* bigtoc CSECT symbols, so it must occur
+	 after the above CSECT check.  */
+      if (ISFCN (cs->c_type) && cs->c_sclass != C_TPDEF)
+	{
+	  bfd_coff_swap_aux_in (abfd, raw_auxptr, cs->c_type, cs->c_sclass,
+				0, cs->c_naux, &main_aux);
+	  goto function_entry_point;
+	}
+
       switch (cs->c_sclass)
 	{
 
@@ -2295,14 +2298,8 @@ scan_xcoff_symtab (objfile)
 	    else
 	      csect_aux = main_aux[0];
 
-	    /* If symbol name starts with ".$" or "$", ignore it. 
-
-	       A symbol like "@FIX1" introduces a section for -bbigtoc jump
-	       tables, which contain anonymous linker-generated code. 
-	       Ignore those sections to avoid "pc 0x... in read in psymtab,
-	       but not in symtab" warnings from find_pc_sect_symtab.  */
-
-	    if (namestring[0] == '$' || namestring[0] == '@'
+	    /* If symbol name starts with ".$" or "$", ignore it.  */
+	    if (namestring[0] == '$'
 		|| (namestring[0] == '.' && namestring[1] == '$'))
 	      break;
 
@@ -2348,7 +2345,11 @@ scan_xcoff_symtab (objfile)
 			       objfile->static_psymbols.next);
 			  }
 		      }
-		    if (namestring && namestring[0] == '.')
+		    /* Activate the misc_func_recorded mechanism for
+		       compiler- and linker-generated CSECTs like ".strcmp"
+		       and "@FIX1".  */ 
+		    if (namestring && (namestring[0] == '.'
+				       || namestring[0] == '@'))
 		      {
 			last_csect_name = namestring;
 			last_csect_val = symbol.n_value;

Re: RFA: [symfile.c} Fix to symbol_file_add()

[Elena Zannoni]

Fernando Nasser writes:
 > Paul N. Hilfinger has pointed out to me that a few operations should be
 > done every time a new symbol file is read.  This was an oversight in a
 > patch I wrote in January.  The small patch attached fixes that.
 > 
 > ChangeLog:
 > 
 > 	* symfile.c (symbol_file_command): Move cleanup operations
 > 	from here...
 > 	(symbol_file_add): ...to here, so they are run every time a new
 > 	symbol file is read.
 > 

This would change the behavior of all the calls to symbol_file_add().
Which may not be what you want. Later on in symfile.c there is another
call to symbol_file_add which is followed by reinit_frame_cache(). In
this case the same thing would be done twice, and in other calls it
would be done, when it was not done before.  I am not sure why the
other calls to symbol_file_add don't reinititalize things, but it
seems a little too risky to do this, w/o making sure those platforms
are not affected.

coff-solib.c:91:	  objfile = symbol_file_add (filename, from_tty,
cxux-nat.c:370:  objfile = symbol_file_add (LIBC_FILE, 0, NULL, 0, OBJF_READNOW);
cxux-nat.c:390:	      symbol_file_add (path_name, 1, &section_addrs, 0, 0);
irix5-nat.c:850:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
osfsolib.c:585:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
pa64solib.c:273:  so->objfile = symbol_file_add (name, from_tty, &section_addrs, 0, OBJF_SHARED);
remote-udi.c:1291:  symbol_file_add (strtok (args, " \t"), from_tty, NULL, 1, 0);
remote-vx.c:670:  objfile = symbol_file_add (name, from_tty, NULL, 0, 0);
solib.c:313:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
somsolib.c:290:  so->objfile = symbol_file_add (name, from_tty, NULL, 0, OBJF_SHARED);
symfile.c:905:  symbol_file_add (args, from_tty, NULL, 1, 0);
symfile.c:982:		  symbol_file_add (name, from_tty, NULL, 1, flags);
symfile.c:1549:  symbol_file_add (filename, from_tty, &section_addrs, 0, flags);
win32-nat.c:444:  p->ret = symbol_file_add (p->name, p->from_tty, p->addrs, p->mainline, p->flags);


Since the original code substitution was to replace calls to
symbol_file_command() with symbol_file_add_main(), I would think that
the lines below should be left in the symbol_file_command function and
added to symbol_file_add_main() instead, therefore preserving the
original semantics.

Elena


 > --
 > Fernando Nasser
 > Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
 > 2323 Yonge Street, Suite #300
 > Toronto, Ontario   M4P 2C9Index: symfile.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/symfile.c,v
 > retrieving revision 1.31
 > diff -c -p -r1.31 symfile.c
 > *** symfile.c	2001/04/05 02:02:13	1.31
 > --- symfile.c	2001/04/29 16:13:12
 > *************** symbol_file_add (char *name, int from_tt
 > *** 893,898 ****
 > --- 893,907 ----
 >     if (target_new_objfile_hook)
 >       target_new_objfile_hook (objfile);
 >   
 > + #ifdef HPUXHPPA
 > +   RESET_HP_UX_GLOBALS ();
 > + #endif
 > +   /* Getting new symbols may change our opinion about
 > +      what is frameless.  */
 > +   reinit_frame_cache ();
 > + 
 > +   set_initial_language ();
 > + 
 >     return (objfile);
 >   }
 >   
 > *************** symbol_file_command (char *args, int fro
 > *** 980,993 ****
 >   		{
 >                     name = *argv;
 >   		  symbol_file_add (name, from_tty, NULL, 1, flags);
 > - #ifdef HPUXHPPA
 > - 		  RESET_HP_UX_GLOBALS ();
 > - #endif
 > - 		  /* Getting new symbols may change our opinion about
 > - 		     what is frameless.  */
 > - 		  reinit_frame_cache ();
 > - 
 > - 		  set_initial_language ();
 >   		}
 >   	  argv++;
 >   	}
 > --- 989,994 ----

Re: RFA: [symfile.c} Fix to symbol_file_add()

[Fernando Nasser]

Thanks for reviewing my patch so promptly Elena.  Please see my comments
below.

Elena Zannoni wrote:
> 
> Fernando Nasser writes:
>  > Paul N. Hilfinger has pointed out to me that a few operations should be
>  > done every time a new symbol file is read.  This was an oversight in a
>  > patch I wrote in January.  The small patch attached fixes that.
>  >
>  > ChangeLog:
>  >
>  >      * symfile.c (symbol_file_command): Move cleanup operations
>  >      from here...
>  >      (symbol_file_add): ...to here, so they are run every time a new
>  >      symbol file is read.
>  >
> 
> This would change the behavior of all the calls to symbol_file_add().
> Which may not be what you want.


You are right, this bits:
>  > + #ifdef HPUXHPPA
>  > +   RESET_HP_UX_GLOBALS ();
>  > + #endif
>  > +
>  > +   set_initial_language ();

should only be done when a new "main" is loaded.  I suggested moving it
to symbol_file_add_main(), which is called exactly in those cases.

However, the bit:
>  > +   /* Getting new symbols may change our opinion about
>  > +      what is frameless.  */
>  > +   reinit_frame_cache ();

should _always_ be done, whenever we load new symbols.

What happens today, and happened _before_ my patch (it hasn't changed
any call to symbol_file_add() nor symbol_file_add() itself except for
the clear part), is that _some_ of the callers do it and some do not.

I guess this will be very difficult to control unless we move the
reinit_frame_cache () call to inside symbol_file_add().

So, I would, if you agree, post a patch (and ask Paul to help me test
it) that does:

1) Move the HP and set_initial_language() bits to symbol_file_add_main()

2) Move the reinit_frame_cache () bit to symbol_file_add(), as the
current patch does.

3) Remove the duplicate call to reinit_frame_cache () from the targets.


How does it sound?


Regards,
Fernando



> Later on in symfile.c there is another
> call to symbol_file_add which is followed by reinit_frame_cache(). In
> this case the same thing would be done twice, and in other calls it
> would be done, when it was not done before.  I am not sure why the
> other calls to symbol_file_add don't reinititalize things, but it
> seems a little too risky to do this, w/o making sure those platforms
> are not affected.
> 
> coff-solib.c:91:          objfile = symbol_file_add (filename, from_tty,
> cxux-nat.c:370:  objfile = symbol_file_add (LIBC_FILE, 0, NULL, 0, OBJF_READNOW);
> cxux-nat.c:390:       symbol_file_add (path_name, 1, &section_addrs, 0, 0);
> irix5-nat.c:850:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
> osfsolib.c:585:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
> pa64solib.c:273:  so->objfile = symbol_file_add (name, from_tty, &section_addrs, 0, OBJF_SHARED);
> remote-udi.c:1291:  symbol_file_add (strtok (args, " \t"), from_tty, NULL, 1, 0);
> remote-vx.c:670:  objfile = symbol_file_add (name, from_tty, NULL, 0, 0);
> solib.c:313:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
> somsolib.c:290:  so->objfile = symbol_file_add (name, from_tty, NULL, 0, OBJF_SHARED);
> symfile.c:905:  symbol_file_add (args, from_tty, NULL, 1, 0);
> symfile.c:982:            symbol_file_add (name, from_tty, NULL, 1, flags);
> symfile.c:1549:  symbol_file_add (filename, from_tty, &section_addrs, 0, flags);
> win32-nat.c:444:  p->ret = symbol_file_add (p->name, p->from_tty, p->addrs, p->mainline, p->flags);
> 
> Since the original code substitution was to replace calls to
> symbol_file_command() with symbol_file_add_main(), I would think that
> the lines below should be left in the symbol_file_command function and
> added to symbol_file_add_main() instead, therefore preserving the
> original semantics.
> 
> Elena
> 
>  > --
>  > Fernando Nasser
>  > Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
>  > 2323 Yonge Street, Suite #300
>  > Toronto, Ontario   M4P 2C9Index: symfile.c
>  > ===================================================================
>  > RCS file: /cvs/src/src/gdb/symfile.c,v
>  > retrieving revision 1.31
>  > diff -c -p -r1.31 symfile.c
>  > *** symfile.c        2001/04/05 02:02:13     1.31
>  > --- symfile.c        2001/04/29 16:13:12
>  > *************** symbol_file_add (char *name, int from_tt
>  > *** 893,898 ****
>  > --- 893,907 ----
>  >     if (target_new_objfile_hook)
>  >       target_new_objfile_hook (objfile);
>  >
>  > + #ifdef HPUXHPPA
>  > +   RESET_HP_UX_GLOBALS ();
>  > + #endif
>  > +   /* Getting new symbols may change our opinion about
>  > +      what is frameless.  */
>  > +   reinit_frame_cache ();
>  > +
>  > +   set_initial_language ();
>  > +
>  >     return (objfile);
>  >   }
>  >
>  > *************** symbol_file_command (char *args, int fro
>  > *** 980,993 ****
>  >              {
>  >                     name = *argv;
>  >                symbol_file_add (name, from_tty, NULL, 1, flags);
>  > - #ifdef HPUXHPPA
>  > -              RESET_HP_UX_GLOBALS ();
>  > - #endif
>  > -              /* Getting new symbols may change our opinion about
>  > -                 what is frameless.  */
>  > -              reinit_frame_cache ();
>  > -
>  > -              set_initial_language ();
>  >              }
>  >        argv++;
>  >      }
>  > --- 989,994 ----

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: RFA: [symfile.c} Fix to symbol_file_add()

[Elena Zannoni]

Fernando Nasser writes:
 > Thanks for reviewing my patch so promptly Elena.  Please see my comments
 > below.
 > 
 > Elena Zannoni wrote:
 > > 
 > > Fernando Nasser writes:
 > >  > Paul N. Hilfinger has pointed out to me that a few operations should be
 > >  > done every time a new symbol file is read.  This was an oversight in a
 > >  > patch I wrote in January.  The small patch attached fixes that.
 > >  >
 > >  > ChangeLog:
 > >  >
 > >  >      * symfile.c (symbol_file_command): Move cleanup operations
 > >  >      from here...
 > >  >      (symbol_file_add): ...to here, so they are run every time a new
 > >  >      symbol file is read.
 > >  >
 > > 
 > > This would change the behavior of all the calls to symbol_file_add().
 > > Which may not be what you want.
 > 
 > 
 > You are right, this bits:
 > >  > + #ifdef HPUXHPPA
 > >  > +   RESET_HP_UX_GLOBALS ();
 > >  > + #endif
 > >  > +
 > >  > +   set_initial_language ();
 > 
 > should only be done when a new "main" is loaded.  I suggested moving it
 > to symbol_file_add_main(), which is called exactly in those cases.
 > 

Yes, agreed.

 > However, the bit:
 > >  > +   /* Getting new symbols may change our opinion about
 > >  > +      what is frameless.  */
 > >  > +   reinit_frame_cache ();
 > 
 > should _always_ be done, whenever we load new symbols.
 > 
 > What happens today, and happened _before_ my patch (it hasn't changed
 > any call to symbol_file_add() nor symbol_file_add() itself except for
 > the clear part), is that _some_ of the callers do it and some do not.
 > 

Yes, true. But I wonder if that was intended. What happens if you
manually load a shared library? Do you need to clear the frame cache
at that point?

 > I guess this will be very difficult to control unless we move the
 > reinit_frame_cache () call to inside symbol_file_add().
 > 
 > So, I would, if you agree, post a patch (and ask Paul to help me test
 > it) that does:
 > 
 > 1) Move the HP and set_initial_language() bits to symbol_file_add_main()
 > 

OK.

 > 2) Move the reinit_frame_cache () bit to symbol_file_add(), as the
 > current patch does.
 > 

I need to understand better what happens with shared libraries.
SOLIB_ADD ends up calling this. I see it used in the attach command
and in this case a reinit_frame_cache is OK, so is for the case in
sol-thread.c but what about the other calls?

 > 3) Remove the duplicate call to reinit_frame_cache () from the targets.
 > 

After 2 is resolved.

 > 
 > How does it sound?

Mostly ok. 


Elena

 > 
 > 
 > Regards,
 > Fernando
 > 
 > 
 > 
 > > Later on in symfile.c there is another
 > > call to symbol_file_add which is followed by reinit_frame_cache(). In
 > > this case the same thing would be done twice, and in other calls it
 > > would be done, when it was not done before.  I am not sure why the
 > > other calls to symbol_file_add don't reinititalize things, but it
 > > seems a little too risky to do this, w/o making sure those platforms
 > > are not affected.
 > > 
 > > coff-solib.c:91:          objfile = symbol_file_add (filename, from_tty,
 > > cxux-nat.c:370:  objfile = symbol_file_add (LIBC_FILE, 0, NULL, 0, OBJF_READNOW);
 > > cxux-nat.c:390:       symbol_file_add (path_name, 1, &section_addrs, 0, 0);
 > > irix5-nat.c:850:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
 > > osfsolib.c:585:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
 > > pa64solib.c:273:  so->objfile = symbol_file_add (name, from_tty, &section_addrs, 0, OBJF_SHARED);
 > > remote-udi.c:1291:  symbol_file_add (strtok (args, " \t"), from_tty, NULL, 1, 0);
 > > remote-vx.c:670:  objfile = symbol_file_add (name, from_tty, NULL, 0, 0);
 > > solib.c:313:  so->objfile = symbol_file_add (so->so_name, so->from_tty,
 > > somsolib.c:290:  so->objfile = symbol_file_add (name, from_tty, NULL, 0, OBJF_SHARED);
 > > symfile.c:905:  symbol_file_add (args, from_tty, NULL, 1, 0);
 > > symfile.c:982:            symbol_file_add (name, from_tty, NULL, 1, flags);
 > > symfile.c:1549:  symbol_file_add (filename, from_tty, &section_addrs, 0, flags);
 > > win32-nat.c:444:  p->ret = symbol_file_add (p->name, p->from_tty, p->addrs, p->mainline, p->flags);
 > > 
 > > Since the original code substitution was to replace calls to
 > > symbol_file_command() with symbol_file_add_main(), I would think that
 > > the lines below should be left in the symbol_file_command function and
 > > added to symbol_file_add_main() instead, therefore preserving the
 > > original semantics.
 > > 
 > > Elena
 > > 
 > >  > --
 > >  > Fernando Nasser
 > >  > Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
 > >  > 2323 Yonge Street, Suite #300
 > >  > Toronto, Ontario   M4P 2C9Index: symfile.c
 > >  > ===================================================================
 > >  > RCS file: /cvs/src/src/gdb/symfile.c,v
 > >  > retrieving revision 1.31
 > >  > diff -c -p -r1.31 symfile.c
 > >  > *** symfile.c        2001/04/05 02:02:13     1.31
 > >  > --- symfile.c        2001/04/29 16:13:12
 > >  > *************** symbol_file_add (char *name, int from_tt
 > >  > *** 893,898 ****
 > >  > --- 893,907 ----
 > >  >     if (target_new_objfile_hook)
 > >  >       target_new_objfile_hook (objfile);
 > >  >
 > >  > + #ifdef HPUXHPPA
 > >  > +   RESET_HP_UX_GLOBALS ();
 > >  > + #endif
 > >  > +   /* Getting new symbols may change our opinion about
 > >  > +      what is frameless.  */
 > >  > +   reinit_frame_cache ();
 > >  > +
 > >  > +   set_initial_language ();
 > >  > +
 > >  >     return (objfile);
 > >  >   }
 > >  >
 > >  > *************** symbol_file_command (char *args, int fro
 > >  > *** 980,993 ****
 > >  >              {
 > >  >                     name = *argv;
 > >  >                symbol_file_add (name, from_tty, NULL, 1, flags);
 > >  > - #ifdef HPUXHPPA
 > >  > -              RESET_HP_UX_GLOBALS ();
 > >  > - #endif
 > >  > -              /* Getting new symbols may change our opinion about
 > >  > -                 what is frameless.  */
 > >  > -              reinit_frame_cache ();
 > >  > -
 > >  > -              set_initial_language ();
 > >  >              }
 > >  >        argv++;
 > >  >      }
 > >  > --- 989,994 ----
 > 
 > -- 
 > Fernando Nasser
 > Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
 > 2323 Yonge Street, Suite #300
 > Toronto, Ontario   M4P 2C9
 > 

Re: RFA: [symfile.c} Fix to symbol_file_add()

[Fernando Nasser]

Elena Zannoni wrote:
> 
>  > However, the bit:
>  > >  > +   /* Getting new symbols may change our opinion about
>  > >  > +      what is frameless.  */
>  > >  > +   reinit_frame_cache ();
>  >
>  > should _always_ be done, whenever we load new symbols.
>  >
> 
> I need to understand better what happens with shared libraries.
> SOLIB_ADD ends up calling this. I see it used in the attach command
> and in this case a reinit_frame_cache is OK, so is for the case in
> sol-thread.c but what about the other calls?
> 

The comment "Getting new symbols may change our opinion about what is
frameless." basically answers your question.  GDB is much more reliable
at figuring out where the prologue ends when symbols are available.  If
you are stopped inside a function (in a shared library) and you did not
had the symbols when you created the frame cache, you did a less job
than you could have done if you had symbols.

That is why the frame cache is always reset, so the frames can be
reconstructed with mode information and, thus, more precisely. 

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

RFA: [symfile.c} Fix to symbol_file_add() [REPOST]

[Fernando Nasser]

After the fruitful discussions with Elena, I believe I have a better
patch, so please consider this one instead.

As Elena explained, we must let the callers of symbol_file_add() do the
cache reset as we may be loading several shared libraries.  Doing the
cache reset for each one is too much overhead as we will have to grab
the data for filling the current frame every single time.  Well,
symbol_file_add_main () is a caller, so I guess it can go there.  And
none of its callers has a a call to reinit_frame_cache() as they used to
call symbol_file_command().

I guess the previous behavior is being safely restored now.

However, several of the callers of symbol_file_add() do not call
reinit_frame_cache() as they should.  But as far as I can tell, this has
always been like that.  The only thing that is lost is the possibility
to get a better frame description of the current stack by using the
newly read symbols.  I don't know enough about all this targets to know
if this is important (I don't know why they are reading symbol files). 
The shared library case is already taken care of as Elena has pointed
out.



ChangeLog:

        * symfile.c (symbol_file_command): Move cleanup operations
        from here...
        (symbol_file_add_main): ...to here, so they are run every time
        a new main symbol file is read.


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.31
diff -c -p -r1.31 symfile.c
*** symfile.c	2001/04/05 02:02:13	1.31
--- symfile.c	2001/05/02 15:35:32
*************** symbol_file_add (char *name, int from_tt
*** 896,908 ****
    return (objfile);
  }
  
! /* Just call the above with default values.
!    Used when the file is supplied in the gdb command line. */
     
  void
  symbol_file_add_main (char *args, int from_tty)
  {
    symbol_file_add (args, from_tty, NULL, 1, 0);
  }
  
  void
--- 896,920 ----
    return (objfile);
  }
  
! /* Call the above with default values and update whatever is
!    affected by a new main().
!    Used when the file is supplied in the gdb command line
!    and by some targets. */
     
  void
  symbol_file_add_main (char *args, int from_tty)
  {
    symbol_file_add (args, from_tty, NULL, 1, 0);
+ 
+ #ifdef HPUXHPPA
+   RESET_HP_UX_GLOBALS ();
+ #endif
+ 
+   /* Getting new symbols may change our opinion about
+      what is frameless.  */
+   reinit_frame_cache ();
+ 
+   set_initial_language ();
  }
  
  void
*************** symbol_file_command (char *args, int fro
*** 980,993 ****
  		{
                    name = *argv;
  		  symbol_file_add (name, from_tty, NULL, 1, flags);
- #ifdef HPUXHPPA
- 		  RESET_HP_UX_GLOBALS ();
- #endif
- 		  /* Getting new symbols may change our opinion about
- 		     what is frameless.  */
- 		  reinit_frame_cache ();
- 
- 		  set_initial_language ();
  		}
  	  argv++;
  	}
--- 992,997 ----
From ezannoni@cygnus.com Wed May 02 09:06:00 2001
From: Elena Zannoni <ezannoni@cygnus.com>
To: Fernando Nasser <fnasser@redhat.com>
Cc: Elena Zannoni <ezannoni@cygnus.com>, gdb-patches@sources.redhat.com, hilfingr@otisco.mckusick.com
Subject: Re: RFA: [symfile.c} Fix to symbol_file_add() [REPOST]
Date: Wed, 02 May 2001 09:06:00 -0000
Message-id: <15088.12423.711167.908434@kwikemart.cygnus.com>
References: <3AED8391.C6B9A456@redhat.com> <15085.43538.55216.581538@kwikemart.cygnus.com> <3AEDF6A3.440D0C62@redhat.com> <15086.11696.278572.217415@kwikemart.cygnus.com> <3AEEB0E2.3232D2DE@redhat.com> <15087.31793.429533.289522@kwikemart.cygnus.com> <3AF02D6E.333B2AAB@redhat.com>
X-SW-Source: 2001-05/msg00010.html
Content-length: 3520

Thanks Fernando!
But shouldn't we be leaving the stuff in symbol_file_command?
I.e. only adding it to the new function?
 

Elena


Fernando Nasser writes:
 > After the fruitful discussions with Elena, I believe I have a better
 > patch, so please consider this one instead.
 > 
 > As Elena explained, we must let the callers of symbol_file_add() do the
 > cache reset as we may be loading several shared libraries.  Doing the
 > cache reset for each one is too much overhead as we will have to grab
 > the data for filling the current frame every single time.  Well,
 > symbol_file_add_main () is a caller, so I guess it can go there.  And
 > none of its callers has a a call to reinit_frame_cache() as they used to
 > call symbol_file_command().
 > 
 > I guess the previous behavior is being safely restored now.
 > 
 > However, several of the callers of symbol_file_add() do not call
 > reinit_frame_cache() as they should.  But as far as I can tell, this has
 > always been like that.  The only thing that is lost is the possibility
 > to get a better frame description of the current stack by using the
 > newly read symbols.  I don't know enough about all this targets to know
 > if this is important (I don't know why they are reading symbol files). 
 > The shared library case is already taken care of as Elena has pointed
 > out.
 > 
 > 
 > 
 > ChangeLog:
 > 
 >         * symfile.c (symbol_file_command): Move cleanup operations
 >         from here...
 >         (symbol_file_add_main): ...to here, so they are run every time
 >         a new main symbol file is read.
 > 
 > 
 > -- 
 > Fernando Nasser
 > Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
 > 2323 Yonge Street, Suite #300
 > Toronto, Ontario   M4P 2C9Index: symfile.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/symfile.c,v
 > retrieving revision 1.31
 > diff -c -p -r1.31 symfile.c
 > *** symfile.c	2001/04/05 02:02:13	1.31
 > --- symfile.c	2001/05/02 15:35:32
 > *************** symbol_file_add (char *name, int from_tt
 > *** 896,908 ****
 >     return (objfile);
 >   }
 >   
 > ! /* Just call the above with default values.
 > !    Used when the file is supplied in the gdb command line. */
 >      
 >   void
 >   symbol_file_add_main (char *args, int from_tty)
 >   {
 >     symbol_file_add (args, from_tty, NULL, 1, 0);
 >   }
 >   
 >   void
 > --- 896,920 ----
 >     return (objfile);
 >   }
 >   
 > ! /* Call the above with default values and update whatever is
 > !    affected by a new main().
 > !    Used when the file is supplied in the gdb command line
 > !    and by some targets. */
 >      
 >   void
 >   symbol_file_add_main (char *args, int from_tty)
 >   {
 >     symbol_file_add (args, from_tty, NULL, 1, 0);
 > + 
 > + #ifdef HPUXHPPA
 > +   RESET_HP_UX_GLOBALS ();
 > + #endif
 > + 
 > +   /* Getting new symbols may change our opinion about
 > +      what is frameless.  */
 > +   reinit_frame_cache ();
 > + 
 > +   set_initial_language ();
 >   }
 >   
 >   void
 > *************** symbol_file_command (char *args, int fro
 > *** 980,993 ****
 >   		{
 >                     name = *argv;
 >   		  symbol_file_add (name, from_tty, NULL, 1, flags);
 > - #ifdef HPUXHPPA
 > - 		  RESET_HP_UX_GLOBALS ();
 > - #endif
 > - 		  /* Getting new symbols may change our opinion about
 > - 		     what is frameless.  */
 > - 		  reinit_frame_cache ();
 > - 
 > - 		  set_initial_language ();
 >   		}
 >   	  argv++;
 >   	}
 > --- 992,997 ----

Re: RFA: [symfile.c} Fix to symbol_file_add() [REPOST]

[Fernando Nasser]

Elena Zannoni wrote:
> 
> Thanks Fernando!
> But shouldn't we be leaving the stuff in symbol_file_command?
> I.e. only adding it to the new function?
> 

Yes, of course.  It calls symbol_file_add(), not symbol_file_add_main()
because of the flags.  Silly me.

Yes, please consider the patch without the removal part.

P.S.: I guess  symbol_file_add_main() should have the flags argument,
but I guess this thing has been going on for too long now for me to go
after all callers and add the argument.


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: RFA: [symfile.c} Fix to symbol_file_add() [REPOST]

[Elena Zannoni]

Fernando Nasser writes:
 > Elena Zannoni wrote:
 > > 
 > > Thanks Fernando!
 > > But shouldn't we be leaving the stuff in symbol_file_command?
 > > I.e. only adding it to the new function?
 > > 
 > 
 > Yes, of course.  It calls symbol_file_add(), not symbol_file_add_main()
 > because of the flags.  Silly me.
 > 
 > Yes, please consider the patch without the removal part.

Ok, sure.  Check it in. Just for completion can you post the final
diff once you are done?

Thanks
Elena


 > 
 > P.S.: I guess  symbol_file_add_main() should have the flags argument,
 > but I guess this thing has been going on for too long now for me to go
 > after all callers and add the argument.

Add a FIXME/comment.

Elena


 > 
 > 
 > -- 
 > Fernando Nasser
 > Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
 > 2323 Yonge Street, Suite #300
 > Toronto, Ontario   M4P 2C9
 > 

Re: RFA: [symfile.c} Fix to symbol_file_add() [REPOST]

[Fernando Nasser]

Elena Zannoni wrote:
> 
> Fernando Nasser writes:
>  > Elena Zannoni wrote:
>  > >
>  > > Thanks Fernando!
>  > > But shouldn't we be leaving the stuff in symbol_file_command?
>  > > I.e. only adding it to the new function?
>  > >
>  >
>  > Yes, of course.  It calls symbol_file_add(), not symbol_file_add_main()
>  > because of the flags.  Silly me.
>  >
>  > Yes, please consider the patch without the removal part.
> 
> Ok, sure.  Check it in. Just for completion can you post the final
> diff once you are done?
> 

Absolutely.


> Thanks
> Elena
> 
>  >
>  > P.S.: I guess  symbol_file_add_main() should have the flags argument,
>  > but I guess this thing has been going on for too long now for me to go
>  > after all callers and add the argument.
> 
> Add a FIXME/comment.
> 

Good idea.


Thank you for all your help.


P.S.: I will have to do all this tomorrow afternoon as I have to prepare
a presentation.


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9