RFA: [buildsym.c] Turn off unused addr bits in linetable

RFA: [buildsym.c] Turn off unused addr bits in linetable

[Fernando Nasser]

Some systems like ARM may have extra bits set in the address to indicate
a mode or something else (the LSB=1 means thumb mode for ARM).

So, before storing line number addresses we may need to turn this bits
off as they may be passed to gdb with the bits on as result of
relocation (ld has no way of knowing it is for a line table entry).

The following patch passes the line addresses through the target
dependent macro ADDR_BITS_REMOVE() before storing it on the table.

OK to commit?


ChangeLog:

	* buildsym.c (record_line): Turn off unused addr bits.

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9
Index: gdb/buildsym.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/buildsym.c,v
retrieving revision 2.104
diff -c -p -r2.104 buildsym.c
*** buildsym.c	2000/02/21 10:02:58	2.104
--- buildsym.c	2001/03/12 23:50:04
*************** record_line (register struct subfile *su
*** 725,731 ****
  
    e = subfile->line_vector->item + subfile->line_vector->nitems++;
    e->line = line;
!   e->pc = pc;
  }
  
  /* Needed in order to sort line tables from IBM xcoff files.  Sigh!  */
--- 725,731 ----
  
    e = subfile->line_vector->item + subfile->line_vector->nitems++;
    e->line = line;
!   e->pc = ADDR_BITS_REMOVE(pc);
  }
  
  /* Needed in order to sort line tables from IBM xcoff files.  Sigh!  */
From msnyder@cygnus.com Mon Mar 12 17:02:00 2001
From: Michael Snyder <msnyder@cygnus.com>
To: Kevin Buettner <kevinb@cygnus.com>
Cc: gdb-patches@sources.redhat.com
Subject: Re: [PATCH RFC] solib-svr4 cleanups
Date: Mon, 12 Mar 2001 17:02:00 -0000
Message-id: <3AAD6EE2.A8BA21B0@cygnus.com>
References: <1010302200140.ZM22868@ocotillo.lan> <1010310063020.ZM4272@ocotillo.lan> <3AAD24ED.6038BBA8@cygnus.com> <1010312195449.ZM14150@ocotillo.lan> <3AAD5B9C.7C4EA0BC@cygnus.com> <1010313000757.ZM14597@ocotillo.lan>
X-SW-Source: 2001-03/msg00183.html
Content-length: 669

Kevin Buettner wrote:
> 
> On Mar 12,  3:28pm, Michael Snyder wrote:
> 
> > > The _SYSCALL32 define was only needed for the (legacy) link_map stuff.
> > > It has been moved to solib-legacy.c.
> >
> > OK, good.  Sorry I haven't been following the discussion with full
> > attention.  You've tested the new method on sparc64 solaris, then?
> 
> I think so.  I tried to verify it just now, but unfortunately
> I'm unable to login to the machine that I did my testing on.
> 
> In any event, I ran into problems when I *didn't* have _SYSCALL32
> defined in solib-legacy.h.  When I moved the define from solib-svr4.c
> to solib-legacy.c, things worked fine.

OK, I'm happy.

Re: RFA: [buildsym.c] Turn off unused addr bits in linetable

[Michael Snyder]

Fernando Nasser wrote:
> 
> Some systems like ARM may have extra bits set in the address to indicate
> a mode or something else (the LSB=1 means thumb mode for ARM).
> 
> So, before storing line number addresses we may need to turn this bits
> off as they may be passed to gdb with the bits on as result of
> relocation (ld has no way of knowing it is for a line table entry).
> 
> The following patch passes the line addresses through the target
> dependent macro ADDR_BITS_REMOVE() before storing it on the table.
> 
> OK to commit?

It seems in general like the right thing -- but I can't speak to 
every target that uses this macro.  Certainly it will not hurt 
any target that doesn't define the macro (which defaults to identity).

> 
> ChangeLog:
> 
>         * buildsym.c (record_line): Turn off unused addr bits.
> 
> --
> Fernando Nasser
> Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
> 2323 Yonge Street, Suite #300
> Toronto, Ontario   M4P 2C9
> 
>   ----------------------------------------------------------------------------------------------------
> Index: gdb/buildsym.c
> ===================================================================
> RCS file: /cvs/cvsfiles/devo/gdb/buildsym.c,v
> retrieving revision 2.104
> diff -c -p -r2.104 buildsym.c
> *** buildsym.c  2000/02/21 10:02:58     2.104
> --- buildsym.c  2001/03/12 23:50:04
> *************** record_line (register struct subfile *su
> *** 725,731 ****
> 
>     e = subfile->line_vector->item + subfile->line_vector->nitems++;
>     e->line = line;
> !   e->pc = pc;
>   }
> 
>   /* Needed in order to sort line tables from IBM xcoff files.  Sigh!  */
> --- 725,731 ----
> 
>     e = subfile->line_vector->item + subfile->line_vector->nitems++;
>     e->line = line;
> !   e->pc = ADDR_BITS_REMOVE(pc);
>   }
> 
>   /* Needed in order to sort line tables from IBM xcoff files.  Sigh!  */

Re: RFA: [buildsym.c] Turn off unused addr bits in linetable

[Andrew Cagney]

Fernando Nasser wrote:
> 
> Some systems like ARM may have extra bits set in the address to indicate
> a mode or something else (the LSB=1 means thumb mode for ARM).
> 
> So, before storing line number addresses we may need to turn this bits
> off as they may be passed to gdb with the bits on as result of
> relocation (ld has no way of knowing it is for a line table entry).
> 
> The following patch passes the line addresses through the target
> dependent macro ADDR_BITS_REMOVE() before storing it on the table.
> 
> OK to commit?
> 
> ChangeLog:
> 
>         * buildsym.c (record_line): Turn off unused addr bits.

I'm not sure about this.  The ARM has two ways of identifying THUMB code
- the LSB and a tacky symbol lookup.

Others, such as MIPS/MIPS16 don't.  I'm not sure how this will affect
that target.  Does the code that uses this address of line also ``turn
off unused addr bits''?

	Andrew

Re: RFA: [buildsym.c] Turn off unused addr bits in linetable

[Fernando Nasser]

Andrew Cagney wrote:
> 
> Fernando Nasser wrote:
> >
> > Some systems like ARM may have extra bits set in the address to indicate
> > a mode or something else (the LSB=1 means thumb mode for ARM).
> >
> > So, before storing line number addresses we may need to turn this bits
> > off as they may be passed to gdb with the bits on as result of
> > relocation (ld has no way of knowing it is for a line table entry).
> >
> > The following patch passes the line addresses through the target
> > dependent macro ADDR_BITS_REMOVE() before storing it on the table.
> >
> > OK to commit?
> >
> > ChangeLog:
> >
> >         * buildsym.c (record_line): Turn off unused addr bits.
> 
> I'm not sure about this.  The ARM has two ways of identifying THUMB code
> - the LSB and a tacky symbol lookup.
> 

Yes, but the loader relocation produces _some_ thumb addresses with the
LSB bit on.  The bit in the symbol table is only used to detect thumb
addresses that had the bit turned off (I have a second patch that
optimizes the ARM implementation of ADDR_BITS_REMOVE() but it is not
necessary for correctness).


> Others, such as MIPS/MIPS16 don't.  I'm not sure how this will affect
> that target.  Does the code that uses this address of line also ``turn
> off unused addr bits''?
> 

Well, they should.  BRAKPOINT_FROM_PC() is supposed to make the
necessary address adjustments.  In the ARM case, it does turn off the
thumb bit (the others will never be set where it is used).

I don't have a MIPS target to test.  I will depend on someone doing it
for me.  I will take a look at the macros implementation though.

Thanks for the heads up.


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

Re: RFA: [buildsym.c] Turn off unused addr bits in linetable

[Fernando Nasser]

Andrew,

I looked at all the targets that define ADDR_BITS_REMOVE() (which are
arm, h8500, m88k, mips, pa, w65, z8k and sh) and all that define
BREAKPOINT_FROM_PC() (which are arm, mips, mcore and mn10300, as far as
I can tell).

I am convinced that this is the right thing to do.  I really wonder, in
some cases, how could it have worked without it (maybe the stub or the
OS cleared the bits for us).

Anyway, there is only one way of knowing it for sure.  The only thing we
know right now is that ARM is broken without it.

OK to commit?

Fernando

Fernando Nasser wrote:
> 
> Andrew Cagney wrote:
> >
> > Fernando Nasser wrote:
> > >
> > > Some systems like ARM may have extra bits set in the address to indicate
> > > a mode or something else (the LSB=1 means thumb mode for ARM).
> > >
> > > So, before storing line number addresses we may need to turn this bits
> > > off as they may be passed to gdb with the bits on as result of
> > > relocation (ld has no way of knowing it is for a line table entry).
> > >
> > > The following patch passes the line addresses through the target
> > > dependent macro ADDR_BITS_REMOVE() before storing it on the table.
> > >
> > > OK to commit?
> > >
> > > ChangeLog:
> > >
> > >         * buildsym.c (record_line): Turn off unused addr bits.
> >
> > I'm not sure about this.  The ARM has two ways of identifying THUMB code
> > - the LSB and a tacky symbol lookup.
> >
> 
> Yes, but the loader relocation produces _some_ thumb addresses with the
> LSB bit on.  The bit in the symbol table is only used to detect thumb
> addresses that had the bit turned off (I have a second patch that
> optimizes the ARM implementation of ADDR_BITS_REMOVE() but it is not
> necessary for correctness).
> 
> > Others, such as MIPS/MIPS16 don't.  I'm not sure how this will affect
> > that target.  Does the code that uses this address of line also ``turn
> > off unused addr bits''?
> >
> 
> Well, they should.  BRAKPOINT_FROM_PC() is supposed to make the
> necessary address adjustments.  In the ARM case, it does turn off the
> thumb bit (the others will never be set where it is used).
> 
> I don't have a MIPS target to test.  I will depend on someone doing it
> for me.  I will take a look at the macros implementation though.
> 
> Thanks for the heads up.
> 

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

Re: RFA: [buildsym.c] Turn off unused addr bits in linetable

[Andrew Cagney]

Fernando Nasser wrote:
> 
> Andrew,
> 
> I looked at all the targets that define ADDR_BITS_REMOVE() (which are
> arm, h8500, m88k, mips, pa, w65, z8k and sh) and all that define
> BREAKPOINT_FROM_PC() (which are arm, mips, mcore and mn10300, as far as
> I can tell).
> 
> I am convinced that this is the right thing to do.  I really wonder, in
> some cases, how could it have worked without it (maybe the stub or the
> OS cleared the bits for us).

I suspect that it is like the 32 bit MIPS - no one was sure how it
should work.  Only when the decision that the 32 bit MIPS was have all
addresses converted to cannonical form (i.e. sign extend them) did a
heap of problems get flushed.

I'd lace your patch with comments explaining how the table contains
cannonical addresses and those addresses don't contain any stray magic
bits.  That way the next person will know where the error is when they
find a comparison is doing strange things because the addresses don't
quite match.

> Anyway, there is only one way of knowing it for sure.  The only thing we
> know right now is that ARM is broken without it.
> 
> OK to commit?

I withdraw my objection.  I think the maintainer had already approved
it.

Thanks for investigating this!

	Andrew

PS: A multi-arch footnote (Hi nick :-):  At some stage or another, an
additional interface into BFD is going to be needed so that GDB can ask
BFD what the architecture/machine tupple for a given address is. 
Details are for much later.  This is just a little flag :-)

Re: RFA: [buildsym.c] Turn off unused addr bits in linetable

[Elena Zannoni]

Fernando, did you check this in?  Thanks for investigating.  

BTW. Sh doesn't define ADDR_BITS_REMOVE. But it uses it! I'll have to
fix that.


Elena


Andrew Cagney writes:
 > Fernando Nasser wrote:
 > > 
 > > Andrew,
 > > 
 > > I looked at all the targets that define ADDR_BITS_REMOVE() (which are
 > > arm, h8500, m88k, mips, pa, w65, z8k and sh) and all that define
 > > BREAKPOINT_FROM_PC() (which are arm, mips, mcore and mn10300, as far as
 > > I can tell).
 > > 
 > > I am convinced that this is the right thing to do.  I really wonder, in
 > > some cases, how could it have worked without it (maybe the stub or the
 > > OS cleared the bits for us).
 > 
 > I suspect that it is like the 32 bit MIPS - no one was sure how it
 > should work.  Only when the decision that the 32 bit MIPS was have all
 > addresses converted to cannonical form (i.e. sign extend them) did a
 > heap of problems get flushed.
 > 
 > I'd lace your patch with comments explaining how the table contains
 > cannonical addresses and those addresses don't contain any stray magic
 > bits.  That way the next person will know where the error is when they
 > find a comparison is doing strange things because the addresses don't
 > quite match.
 > 
 > > Anyway, there is only one way of knowing it for sure.  The only thing we
 > > know right now is that ARM is broken without it.
 > > 
 > > OK to commit?
 > 
 > I withdraw my objection.  I think the maintainer had already approved
 > it.
 > 
 > Thanks for investigating this!
 > 
 > 	Andrew
 > 
 > PS: A multi-arch footnote (Hi nick :-):  At some stage or another, an
 > additional interface into BFD is going to be needed so that GDB can ask
 > BFD what the architecture/machine tupple for a given address is. 
 > Details are for much later.  This is just a little flag :-)
 > 

Re: RFA: [buildsym.c] Turn off unused addr bits in linetable

[Fernando Nasser]

Elena Zannoni wrote:
> 
> Fernando, did you check this in? 

I just did.

Thanks.


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



ChangeLog:

        * buildsym.c (record_line): Turn off unused addr bits.


Index: gdb/buildsym.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gdb/buildsym.c,v
retrieving revision 2.104
diff -c -p -r2.104 buildsym.c
*** buildsym.c  2000/02/21 10:02:58     2.104
--- buildsym.c  2001/03/12 23:50:04
*************** record_line (register struct subfile *su
*** 725,731 ****
  
    e = subfile->line_vector->item + subfile->line_vector->nitems++;
    e->line = line;
!   e->pc = pc;
  }
  
  /* Needed in order to sort line tables from IBM xcoff files.  Sigh! 
*/
--- 725,731 ----
  
    e = subfile->line_vector->item + subfile->line_vector->nitems++;
    e->line = line;
!   e->pc = ADDR_BITS_REMOVE(pc);
  }
  
  /* Needed in order to sort line tables from IBM xcoff files.  Sigh! 
*/