Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Michael Elizabeth Chastain]

First, ignore the comment in struct minimal_symbol, the comment
is wrong and obsolete.

'msymbol.info' currently has three meanings.

The first meaning is: coffread.c stores a 'storage class' into
msymbol.info.  However, no code *ever* reads that back.
gdb 4.17 used to read this information in arm_pc_is_thumb
to decide whether an msym was a Thumb symbol or Arm symbol.
But this mechanism was replaced in gdb 4.18 with
COFF_MAKE_MSYMBOL_SPECIAL.

The first meaning can die as soon as someone can test a coff platform
with a trivial patch to coffread.c.

The second meaning is: dbxread.c stores the size of the msym
in 'msymbol.info'.  It does this in case SOFUN_ADDRESS_MAYBE_MISSING
is set, in order to synthesize texthigh for that file.

The third meaning is that arm-tdep.c, m68hc11-tdep.c, mips-tdep.c,
and sh-tdep.c use msymbol.info as a place to store one or two
flag bits.

> Consequently, I'd like to propose that "info" be superseeded by a 
> "struct bfd_symbol *" pointer.

I don't quite understand your proposal, because there are three
meanings of "msymbol.info".  Which meanings do you want to supersede?
Can you put up a little sample code?

> The consequence will be that, for a time, the minimal symbol table will 
> be bigger.

Here are some numbers from some old test run:

  msym  0.3 mb
  psym 11.9 mb
  sym  30.1 mb
  mt    8.7 mb  # main_type

The msym's are not space critical.

It sucks that msymbol.info is overloaded for three different meanings
(and none of them is a pointer so we get all those nasty casts).  Let's
separate it into different fields, and then when the data types are
nice and correct, if we want the space back (which I think we won't),
then we can go for 'union' and bit-field things to get the size back
down.

Andrew, I think what you are saying is that you want to separate
out the data field for meaning #3, and have a separate field in the
minsym.  And your new field would be a "struct bfd_symbol *" pointer
instead of the current 1-2 flag bits.  That would be fine with me.

Michael C

Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Andrew Cagney]

> The first meaning can die as soon as someone can test a coff platform
> with a trivial patch to coffread.c.
> 
> The second meaning is: dbxread.c stores the size of the msym
> in 'msymbol.info'.  It does this in case SOFUN_ADDRESS_MAYBE_MISSING
> is set, in order to synthesize texthigh for that file.
> 
> The third meaning is that arm-tdep.c, m68hc11-tdep.c, mips-tdep.c,
> and sh-tdep.c use msymbol.info as a place to store one or two
> flag bits.

The third meaning needs to be refined slightly.  MAKE_MSYMBOL_SPECIAL, 
for coff, doesn't yet take a "bfd_symbol", in fact, coffread doesn't 
even use BFD's canonicalize_symtab to read the symbols.

>> Consequently, I'd like to propose that "info" be superseeded by a 
>> "struct bfd_symbol *" pointer.
> 
> 
> I don't quite understand your proposal, because there are three
> meanings of "msymbol.info".  Which meanings do you want to supersede?
> Can you put up a little sample code?

If I ignore coff, Arm's arm_pc_is_thumb would look like:

	sym = lookup_minimal_symbol_by_pc (memaddr);
	if (sym && sym->bfd_symbol is "elf like")
	  return (ELF_ST_TYPE (((elf_symbol_type *) 
(sym->bfd_symbol))->internal_elf_sym.st_info) == STT_LOPROC)
	else
	  return 0;

MIPS would be identical.  The existing MAKE_MSYMBOL_SPECIAL wouldn't be 
needed.

The other ELF use in elfread + dbxread:

               /* Pass symbol size field in via BFD.  FIXME!!!  */
               size = ((elf_symbol_type *) sym)->internal_elf_sym.st_size;
               msym = record_minimal_symbol_and_info
                 ((char *) sym->name, symaddr,
                  ms_type, (void *) size, sym->section, objfile);

(having calls to record_minimal_symbol_and_info with a NULL info sux :-) 
could be addressed similar.  Anyone realise that the code assumes elf?

>> The consequence will be that, for a time, the minimal symbol table will 
>> be bigger.
> 
> 
> Here are some numbers from some old test run:
> 
>   msym  0.3 mb
>   psym 11.9 mb
>   sym  30.1 mb
>   mt    8.7 mb  # main_type
> 
> The msym's are not space critical.

Yes, I was secretly assuming that.

> It sucks that msymbol.info is overloaded for three different meanings
> (and none of them is a pointer so we get all those nasty casts).  Let's
> separate it into different fields, and then when the data types are
> nice and correct, if we want the space back (which I think we won't),
> then we can go for 'union' and bit-field things to get the size back
> down.

This could immediatly eliminate two of them.  I think the coff version 
of ARM could also be eliminated if coffread (yea, right ... :-) fully 
utilized BFD.

Andrew

Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Elena Zannoni]

Andrew Cagney writes:
 > 
 > > The first meaning can die as soon as someone can test a coff platform
 > > with a trivial patch to coffread.c.
 > > 
 > > The second meaning is: dbxread.c stores the size of the msym
 > > in 'msymbol.info'.  It does this in case SOFUN_ADDRESS_MAYBE_MISSING
 > > is set, in order to synthesize texthigh for that file.
 > > 
 > > The third meaning is that arm-tdep.c, m68hc11-tdep.c, mips-tdep.c,
 > > and sh-tdep.c use msymbol.info as a place to store one or two
 > > flag bits.
 > 
 > The third meaning needs to be refined slightly.  MAKE_MSYMBOL_SPECIAL, 
 > for coff, doesn't yet take a "bfd_symbol", in fact, coffread doesn't 
 > even use BFD's canonicalize_symtab to read the symbols.
 > 

Hmm there is arm_coff_make_msymbol_special, but there is also:

2002-01-06  Andrew Cagney  <ac131313@redhat.com>

	* MAINTAINERS: Note that alpha-dec-osf4.0a, arc-elf, arm-coff,
	arm-elf, arm-pe, d30v-elf, fr30-elf, h8300hms, h8500hms,
	i960-coff, m32r-elf, m68k-elf, m88k, mcore-elf, mn10200-elf,
	ns32k-netbsd, hppa1.1-hp-proelf, v850-elf, vax-dec-vms5.5 and
	z8k-coff have not been multi-arched.  Update z8k-coff build
	status.

2002-01-13  Andrew Cagney  <ac131313@redhat.com>

	* MAINTAINERS: Remove arm-coff and arm-pe from target list.


If I read this right, the whole coff_make_msymbol_special can be
zapped. There is also some more perplexing coff stuff in arm-tdep.c,
that could go too, I believe.

 > >> Consequently, I'd like to propose that "info" be superseeded by a 
 > >> "struct bfd_symbol *" pointer.
 > > 
 > > 

 > [...]
 > If I ignore coff, Arm's arm_pc_is_thumb would look like:
 > 
 > 	sym = lookup_minimal_symbol_by_pc (memaddr);
 > 	if (sym && sym->bfd_symbol is "elf like")
 > 	  return (ELF_ST_TYPE (((elf_symbol_type *) 
 > (sym->bfd_symbol))->internal_elf_sym.st_info) == STT_LOPROC)
 > 	else
 > 	  return 0;
 > 
 > MIPS would be identical.  The existing MAKE_MSYMBOL_SPECIAL wouldn't be 
 > needed.
 > 
 > The other ELF use in elfread + dbxread:
 > 
 >                /* Pass symbol size field in via BFD.  FIXME!!!  */
 >                size = ((elf_symbol_type *) sym)->internal_elf_sym.st_size;
 >                msym = record_minimal_symbol_and_info
 >                  ((char *) sym->name, symaddr,
 >                   ms_type, (void *) size, sym->section, objfile);
 > 
 > (having calls to record_minimal_symbol_and_info with a NULL info sux :-) 

OK, so what you are really saying is that there are in the whole gdb
only a few calls to {prim_}record_minimal_symbol_and_info which have
a non-null info parameter (the 4th one).

I see two:

elfread.c: 	      msym = record_minimal_symbol_and_info
		((char *) sym->name, symaddr,
		 ms_type, (void *) size, sym->section, objfile);

This one, together with MSYMBOL_SIZE could be changed to use bfd information.


coffread.c:		msym = prim_record_minimal_symbol_and_info
		  (cs->c_name, tmpaddr, ms_type, (void *) (long) cs->c_sclass,
		   sec, NULL, objfile);

This one.... I agree with Michael, I don't see it tested by pulling it
out of a minsym. So the parameter could just be removed.


Then there is the MAKE_MSYMBOL_SPECIAL macro.

coffread.c: 		if (msym)
		  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
This one goes with arm-coff, which is dead. 


elfread.c: 	      ELF_MAKE_MSYMBOL_SPECIAL (sym, msym);
this one goes with arm, sh64, mips, m68hc11.


Then there are MSYMBOL_IS_SPECIAL, msymbol_is_special, MSYMBOL_IS_RTC,
MSYMBOL_IS_RTI. All of them can just use the bfd symbol instead, and
are only in the tdep files.

So, ok. Makes sense.

elena

Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Elena Zannoni]

Michael Elizabeth Chastain writes:
 > eza> OK, so what you are really saying is that there are in the whole gdb
 > eza> only a few calls to {prim_}record_minimal_symbol_and_info which have
 > eza> a non-null info parameter (the 4th one).
 > 
 > Yes, that is true.
 > 
 > I think there are two things we could do to elfread.c:
 > 
 > (1A) Quit abusing msymbol.info to store a size in some of the bits.
 >      Just create msymbol.size with the proper data type.
 > 
 > (1B) More like Elena says (and maybe Andrew is saying), change
 >      the actual algorithm to use bfd information.

this one. However I am now not so sure that we can get rid of the arm-coff
stuff. 

 > 
 > Perhaps (1A) would be a useful patch to separate out this issue
 > from the other uses of msymbol.info.  We could do (1B) later.
 > Or just go straight to (1B).
 > 
 > Over in coffread.c, with the "cs->c_sclass" being stuffed into
 > msymbol.info, gdb 4.17 (!) used to read it back.  But then that
 > changed with gdb 4.18.  Again there are two ways to deal with this:
 > 
 > (2A) Data is being written and never read back anywhere, just
 >      mechanically kill the write.
 > 

I would like to do this. I found the original change diff and
reconstructed the genesys of this no-op.

Initially there was no info field (i.e. c_sclass) passed in to
print_record_blah in coffread.c.

Then a change was made to pass that in, and read it back in arm-tdep.c
to use the storage class to determine if a symbol was thumb.

Then another change was made, to not use the info field to read the
storage class, but instead the storage class was passed directly in via
COFF_MAKE_MSYMBOL_SPECIAL(sclass, msym) (Yuck).

Here is the relevant diff, that makes me think that eliminating the
parameter to prin_record_blah in coffread.c is OK:


--- arm-tdep.c	1 Apr 1998 05:46:35 -0000	1.19
+++ arm-tdep.c	1 Sep 1998 16:24:19 -0000	1.20
@@ -68,17 +68,11 @@ arm_pc_is_thumb (memaddr)
   if (IS_THUMB_ADDR (memaddr))
     return 1;
 
-  /* The storage class for minimal symbols is stored by coffread.c in
-     the info field.  Use this to decide if the function is Thumb or Arm.  */
+  /* Thumb function have a "special" bit set in minimal symbols */
   sym = lookup_minimal_symbol_by_pc (memaddr);
   if (sym)
     {
-      unsigned sclass = (unsigned) MSYMBOL_INFO (sym);	/* get storage class */
-      return (   sclass == C_THUMBEXT
-	      || sclass == C_THUMBSTAT
-	      || sclass == C_THUMBEXTFUNC
-	      || sclass == C_THUMBSTATFUNC
-	      || sclass == C_THUMBLABEL);
+      return (MSYMBOL_IS_SPECIAL(sym));
     }
   else
     return 0;
@@ -1528,4 +1525,15 @@ _initialize_arm_tdep ()
 				  "Set usage of ARM 32-bit mode.\n", &setlist),
 		     & showlist);
 
+}
+
+/* Test whether the coff symbol specific value corresponds to a Thumb function */
+int
+coff_sym_is_thumb(int val)
+{
+	return (val == C_THUMBEXT ||
+      val == C_THUMBSTAT ||
+      val == C_THUMBEXTFUNC ||
+      val == C_THUMBSTATFUNC ||
+      val == C_THUMBLABEL);
 }


This was an incomplete cleanup, basically.

I'll send a patch. Maybe it will help understanding what to do with the rest.

elena


 > (2B) Data is being written and never read back anywhere, investigate
 >      what *should* be happening (if anything).  And it might turn out
 >      that what *should* happen is the same code should die, after
 >      someone puts some thought into it.
 > 
 > I have to say, I was a fan of (2A), but now I am not, because if
 > we just do (2A), then we have nothing to remind us of (2B) later.
 > 
 > eza> Then there are MSYMBOL_IS_SPECIAL, msymbol_is_special, MSYMBOL_IS_RTC,
 > eza> MSYMBOL_IS_RTI. All of them can just use the bfd symbol instead, and
 > eza> are only in the tdep files.
 > 
 > Yeah.
 > 
 > eza> So, ok. Makes sense.
 > 
 > Sounds like everybody is converging on a common vision.  Yay.
 > 
 > Michael C

Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Michael Elizabeth Chastain]

eza> OK, so what you are really saying is that there are in the whole gdb
eza> only a few calls to {prim_}record_minimal_symbol_and_info which have
eza> a non-null info parameter (the 4th one).

Yes, that is true.

I think there are two things we could do to elfread.c:

(1A) Quit abusing msymbol.info to store a size in some of the bits.
     Just create msymbol.size with the proper data type.

(1B) More like Elena says (and maybe Andrew is saying), change
     the actual algorithm to use bfd information.

Perhaps (1A) would be a useful patch to separate out this issue
from the other uses of msymbol.info.  We could do (1B) later.
Or just go straight to (1B).

Over in coffread.c, with the "cs->c_sclass" being stuffed into
msymbol.info, gdb 4.17 (!) used to read it back.  But then that
changed with gdb 4.18.  Again there are two ways to deal with this:

(2A) Data is being written and never read back anywhere, just
     mechanically kill the write.

(2B) Data is being written and never read back anywhere, investigate
     what *should* be happening (if anything).  And it might turn out
     that what *should* happen is the same code should die, after
     someone puts some thought into it.

I have to say, I was a fan of (2A), but now I am not, because if
we just do (2A), then we have nothing to remind us of (2B) later.

eza> Then there are MSYMBOL_IS_SPECIAL, msymbol_is_special, MSYMBOL_IS_RTC,
eza> MSYMBOL_IS_RTI. All of them can just use the bfd symbol instead, and
eza> are only in the tdep files.

Yeah.

eza> So, ok. Makes sense.

Sounds like everybody is converging on a common vision.  Yay.

Michael C

Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Michael Elizabeth Chastain]

ac> Yep!
ac> Yep!
ac> Yep!

I'm convinced.  I'm on board with your proposal, yep!  :)

Michael C

Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Andrew Cagney]

> ac says:
> 
> 	sym = lookup_minimal_symbol_by_pc (memaddr);
> 	if (sym && sym->bfd_symbol is "elf like")
> 	  return (ELF_ST_TYPE (((elf_symbol_type *) 
> (sym->bfd_symbol))->internal_elf_sym.st_info) == STT_LOPROC)
> 	else
> 	  return 0;
> 
> Okay, I see.  The existing code has a hook in the symbol reader that
> reads the bfd symbol and calls MSYMBOL_SET_SPECIAL to stash some bits in
> msymbol.info for later.  You would get rid of the hook or simplify and
> just store a simple pointer in msymbol.bfd_symbol.  Have I got that
> right?

Yep!

> Sounds like a great idea to me.  A bunch of squirrelly caching and
> casting code would go away.
> 
>                /* Pass symbol size field in via BFD.  FIXME!!!  */
>                size = ((elf_symbol_type *) sym)->internal_elf_sym.st_size;
>                msym = record_minimal_symbol_and_info
>                  ((char *) sym->name, symaddr,
>                   ms_type, (void *) size, sym->section, objfile);
> 
> Yeah, for this code, you could either have msymbol.size or use the
> information available from msymbol.bfd_symbol.  Anything but
> mysmbol.info.

Yep!

>> This could immediatly eliminate two of them.  I think the coff version 
>> of ARM could also be eliminated if coffread (yea, right ... :-) fully 
>> utilized BFD.
> 
> 
> I'm saying that this code is already dead:
> 
>                 gdb_assert (sizeof (void *) >= sizeof (cs->c_sclass));
>                 msym = prim_record_minimal_symbol_and_info
>                   (cs->c_name, tmpaddr, ms_type, (void *) (long) cs->c_sclass,
>                    sec, NULL, objfile);
> 
> I'm not sure about these lines immediately after it:
> 
>                if (msym)
>                   COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
> 
> I think you are saying that coffread.c can be made to work using
> the new fields.  I don't know whether the existing code works or not,
> I'm juse pointing to the deadness.

Yep!

(COFF_MAKE_MSYMBOL_SPECIAL is currently be used by arm.  coffread 
doesn't use bfd_canonicalize_symtab so doesn't have access to the bfd 
symbol.)

Andrew

Re: [rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Michael Elizabeth Chastain]

ac says:

	sym = lookup_minimal_symbol_by_pc (memaddr);
	if (sym && sym->bfd_symbol is "elf like")
	  return (ELF_ST_TYPE (((elf_symbol_type *) 
(sym->bfd_symbol))->internal_elf_sym.st_info) == STT_LOPROC)
	else
	  return 0;

Okay, I see.  The existing code has a hook in the symbol reader that
reads the bfd symbol and calls MSYMBOL_SET_SPECIAL to stash some bits in
msymbol.info for later.  You would get rid of the hook or simplify and
just store a simple pointer in msymbol.bfd_symbol.  Have I got that
right?

Sounds like a great idea to me.  A bunch of squirrelly caching and
casting code would go away.

               /* Pass symbol size field in via BFD.  FIXME!!!  */
               size = ((elf_symbol_type *) sym)->internal_elf_sym.st_size;
               msym = record_minimal_symbol_and_info
                 ((char *) sym->name, symaddr,
                  ms_type, (void *) size, sym->section, objfile);

Yeah, for this code, you could either have msymbol.size or use the
information available from msymbol.bfd_symbol.  Anything but
mysmbol.info.

> This could immediatly eliminate two of them.  I think the coff version 
> of ARM could also be eliminated if coffread (yea, right ... :-) fully 
> utilized BFD.

I'm saying that this code is already dead:

                gdb_assert (sizeof (void *) >= sizeof (cs->c_sclass));
                msym = prim_record_minimal_symbol_and_info
                  (cs->c_name, tmpaddr, ms_type, (void *) (long) cs->c_sclass,
                   sec, NULL, objfile);

I'm not sure about these lines immediately after it:

               if (msym)
                  COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);

I think you are saying that coffread.c can be made to work using
the new fields.  I don't know whether the existing code works or not,
I'm juse pointing to the deadness.

Michael C

[rfa] Deprecate msymbol.info, add msymbol.bfd_symbol?

[Andrew Cagney]

There isn't yet a patch yet.  I wan't to run the theory past people, 
especially the symtab maintainers.

A recent thread discussed the "info" field of the symbol table where 
MichaelC was trying to pack it down, while I was arguing that it should 
"just go", instead have the computation performed by 
MAKE_MSYMBOL_SPECIAL on every minimal-symbol performed on-demand (and 
after the symbo-table load).

The problem with eliminating "info" is that it would leave the on-demand 
code with no efficient way to find the underlying BFD info that it 
needes :-(

Consequently, I'd like to propose that "info" be superseeded by a 
"struct bfd_symbol *" pointer.

I say "superseed" as I'd prefer doing this in two steps.  In addition to 
the per-architecture MAKE_MSYMBOL_SPECIAL, readers like elfread.c also 
squirrel away info in that field :-/

The consequence will be that, for a time, the minimal symbol table will 
be bigger.  Since its the minimal-symbol table I don't feel too guilty, 
long term the minimal-symbol's size will be restored.  In fact, given 
the apparent overlap between bfd_symbol and general_symbol_info (name, 
section, bfd_section, ?), it opens the way to further savings.

thoughts? does the theory look correct?
Andrew