msym->info is wack

msym->info is wack

[Michael Elizabeth Chastain]

Here are some notes on msym->info.  I am going to submit a couple
of patches based on these notes.

Michael C

struct minimal_symbol has this field:

  struct minimal_symbol
  {
    ...
    /* The info field is available for caching machine-specific information
       so it doesn't have to rederive the info constantly (over a serial line).
       It is initialized to zero and stays that way until target-dependent code
       sets it.  Storage for any data pointed to by this field should be allo-
       cated on the symbol_obstack for the associated objfile.  
       The type would be "void *" except for reasons of compatibility with older
       compilers.  This field is optional.

       Currently, the AMD 29000 tdep.c uses it to remember things it has decoded
       from the instructions in the function header, and the MIPS-16 code uses
       it to identify 16-bit procedures.  */

    char *info;
    ...
  };

The comment is outdated.  I looked for all the uses of minimal_symbol.info.
Here is what I found:

=== COFFREAD.C

coffread.c stores "storage class" into msym->info:

      if (cs->c_name[0] != '@' /* Skip tdesc symbols */ )
	{
	  struct minimal_symbol *msym;

	  /* FIXME: cagney/2001-02-01: The nasty (int) -> (long)
	     -> (void*) cast is to ensure that that the value of
	     cs->c_sclass can be correctly stored in a void
	     pointer in MSYMBOL_INFO.  Better solutions
	     welcome. */
	  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);
	  if (msym)
	    COFF_MAKE_MSYMBOL_SPECIAL (cs->c_sclass, msym);
	}

In the call to prim_record_minimal_symbol_and_info, the fourth parameter
goes into msym->info.  However, nothing ever reads back this "info" as a
c_sclass, anywhere!

Back in gdb 4.17, arm_pc_is_thumb in arm-tdep.c used this information
to determine whether a symbol is a Thumb or Arm function.  But this
mechanism was replaced in gdb 4.18 with a new mechanism based on
COFF_MAKE_MSYMBOL_SPECIAL, which sets a flag bit instead of storing
the sclass and reading it later.  That enables other readers besides
the coff reader to mark functions as special.  The new mechanism
continues to this day.

So we can just kill the whole FIXME and nasty type conversion and do:

  msym = prim_record_minimal_symbol_and_info
    (cs->c_name, tmpaddr, ms_type, NULL, sec, NULL, objfile);

=== DBXREAD.C

dbxread.c uses the 'info' field to store the size of the msym.
Later on, it retrieves the size from msym->info.

dbxread.c does this when SOFUN_ADDRESS_MAYBE_MISSING is set
(which is often the case).  The comment is:

  /* 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.
     The first trick is: if we see a static
     or global function, and the textlow for the current pst
     is not set (ie: textlow_not_set), then we use that function's
     address for the textlow of the pst.  */

  /* Now, to fill in texthigh, we remember the last function seen
     in the .o file.  Also, there's a hack in
     bfd/elf.c and gdb/elfread.c to pass the ELF st_size field
     to here via the misc_info field.  Therefore, we can fill in
     a reliable texthigh by taking the address plus size of the
     last function in the file.  */

Instead of abusing msym->info, I would like to replace this with
an explicit field msym->size.  This would get rid of some
conversions back and forth between 'long' and 'void *'.

The difficulty here is that I can't find any code that ever sets
the size to a non-zero value.  So this needs more investigation
before touching the code.

A little more deeply, it's important to ask whether msym->size
is useful in its own right.  If it is, great, let's have it.
But if it is used only for this purpose, let's not store information
in every minsym that is used just transiently to initialize
pst->texthigh.

=== ARM-TDEP.C, M68HC11-TDEP.C, MIPS-TDEP.C, SH-TDEP.C

These *-tdep.c files use msym->info as a place to store flag bits.
So, first, they are converting 'void *' to 'long' and back again.

The straightforward solution is to provide msym->tdep_flags as
an explicit place for tdep files to store flag bits.  Then they
can store their bits without nasty casting.

Plus the code becomes a lot more readable when it uses
MSYMBOL_TDEP_FLAGS instead of MSYMBOL_INFO.

They can also use less space.  These files use 1 bit or 2 bits
of flags.  We can provide 8 bits and be happy, instead of providing
a whole pointer.  But that's not very important in the minsyms.

Re: msym->info is wack

[Andrew Cagney]

> Here are some notes on msym->info.  I am going to submit a couple
> of patches based on these notes.

Thanks for the reminder.

> === ARM-TDEP.C, M68HC11-TDEP.C, MIPS-TDEP.C, SH-TDEP.C
> 
> These *-tdep.c files use msym->info as a place to store flag bits.
> So, first, they are converting 'void *' to 'long' and back again.

Begs the question, is the field needed -> can bfd provide the info directly?

Arm, MIPS, SH, use it to determine the functions CPU mode.  Thing is, 
libopcodes (in the disassembler) must duplicate that exact same 
heuristic as otherwize the 'objdum -d' won't work.

That leaves m68hc11.  Perhaphs a more lazy approach and a symbol aux can 
be used?

> The straightforward solution is to provide msym->tdep_flags as
> an explicit place for tdep files to store flag bits.  Then they
> can store their bits without nasty casting.

Notice that arm defines/uses:

#define MSYMBOL_SET_SPECIAL(msym)                                       \
         MSYMBOL_INFO (msym) = (char *) (((long) MSYMBOL_INFO (msym))    \
                                         | 0x80000000)

#define MSYMBOL_IS_SPECIAL(msym)                                \
         (((long) MSYMBOL_INFO (msym) & 0x80000000) != 0)

#define MSYMBOL_SIZE(msym)                              \
         ((long) MSYMBOL_INFO (msym) & 0x7fffffff)

i.e., it is doing masking, while dbxread.c defines/uses:

#define MSYMBOL_SIZE(msym) ((long) MSYMBOL_INFO (msym))

i.e., there is no masking.  If Arm sets that top bit, dbxread.c would 
discover a very large size.

Guess that never happens.

Andrew

Re: msym->info is wack

[Andrew Cagney]

> Hi Andrew,
> 
> mec> === ARM-TDEP.C, M68HC11-TDEP.C, MIPS-TDEP.C, SH-TDEP.C
> mec> 
> mec> These *-tdep.c files use msym->info as a place to store flag bits.
> mec> So, first, they are converting 'void *' to 'long' and back again.
> 
> ac> Begs the question, is the field needed -> can bfd provide the info directly?
> 
> I don't know.
> 
> My point of view is "incremental improvement from status quo".  From
> that point of view, changing "set flag bit in void *" to "set flag bit
> in minsym->tdep_flags" is an improvement.  It makes it easier, not
> harder, to change the tdep code later, if desired.

I can see that.  It's also important to take a look around at what is 
really happening.  Otherwize the small incremental improvements miss the 
mark.

Thinking about it, only 1 or 2 of those bazillion marked up minimal 
symbols will ever get used.  So why is the code even marking them up - 
leave it to run time.  It will make the minimal symbol smaller, it will 
speed up the load (although I suspect that on both counts the 
measurement isn't there).

Andrew

Re: msym->info is wack

[Michael Elizabeth Chastain]

Hi Andrew,

mec> === ARM-TDEP.C, M68HC11-TDEP.C, MIPS-TDEP.C, SH-TDEP.C
mec> 
mec> These *-tdep.c files use msym->info as a place to store flag bits.
mec> So, first, they are converting 'void *' to 'long' and back again.

ac> Begs the question, is the field needed -> can bfd provide the info directly?

I don't know.

My point of view is "incremental improvement from status quo".  From
that point of view, changing "set flag bit in void *" to "set flag bit
in minsym->tdep_flags" is an improvement.  It makes it easier, not
harder, to change the tdep code later, if desired.

ac> If Arm sets that top bit, dbxread.c would discover a very large size.
ac> 
ac> Guess that never happens.

True.  The code in dbxread.c is conditional on
SOFUN_ADDRESS_MAYBE_MISSING.  That macro is defined in:

  config/i386/tm-i386sol2.h
  config/i386/tm-linux.h
  powerpc/tm-linux.h
  powerpc/tm-ppc-eabi.h
  sparc/tm-sun4sol2.h

And the flag bits are in:

  arm-tdep.c
  m68hc11-tdep.c
  mips-tdep.c
  sh-tdep.c

So there is no actual overlap at this time.  But it's really disgusting!

Michael C