CORE_ADDR representation

CORE_ADDR representation

[Daniel Jacobowitz]

This comes up again and again, and has at least three times in the
past month with Jan's PIE patches.  Is it time for us to have opaque
arithmetic on target addresses?

My latest problem:

struct section_addr_info *
build_section_addr_info_from_objfile (const struct objfile *objfile)
{
...
  CORE_ADDR mask = CORE_ADDR_MAX;

  if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
    mask = ((CORE_ADDR) 1 << addr_bit) - 1;
...
      sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
                            + objfile->section_offsets->offsets[i]) & mask;

This truncates the high bits.  MIPS sign-extends pointers, even
internally in CORE_ADDR, and this results in separate debug info files
for MIPS executables being relocated off to la-la land.  I had to add
this awful thing:

      if (bfd_get_sign_extend_vma (objfile->obfd)
          && addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT)
          && (sap->other[i].addr & ((CORE_ADDR) 1 << (addr_bit - 1))) != 0)
        sap->other[i].addr |= ~mask;

Which I'm not really proposing for inclusion, well, unless no one has
a better idea; sepdebug.exp on mips-elf currently fails without this.

For instance, should we always internally sign-extend CORE_ADDR?
Always internally zero-extend?  Having it vary by target has been a
recurring problem.

-- 
Daniel Jacobowitz
CodeSourcery

Re: CORE_ADDR representation

[Stan Shebs]

Daniel Jacobowitz wrote:
> This comes up again and again, and has at least three times in the
> past month with Jan's PIE patches.  Is it time for us to have opaque
> arithmetic on target addresses?
>   
Urgh.  On the plus side, the months of busywork lets us avoid dealing 
with the brain-strainingly hard problems. :-)
> This truncates the high bits.  MIPS sign-extends pointers, even
> internally in CORE_ADDR, and this results in separate debug info files
> for MIPS executables being relocated off to la-la land.
Heh, I remember getting hosed that way by a MIPS in 1994...

> For instance, should we always internally sign-extend CORE_ADDR?
> Always internally zero-extend?  Having it vary by target has been a
> recurring problem.
>   
I would say to declare that CORE_ADDR is fundamentally 0..memtop, so it 
should be unsigned and zero-extend.

Can unsigned->signed->diddle->unsigned be encapsulated for MIPS only?

Stan

Re: CORE_ADDR representation

[Mark Kettenis]

> Date: Wed, 17 Feb 2010 23:44:19 -0500
> From: Daniel Jacobowitz <dan@codesourcery.com>
> 
> This comes up again and again, and has at least three times in the
> past month with Jan's PIE patches.  Is it time for us to have opaque
> arithmetic on target addresses?

I'm no terribly excited by having opaque arithmetic.  I fear that it
will make the code significantly harder to read :(.

> My latest problem:
> 
> struct section_addr_info *
> build_section_addr_info_from_objfile (const struct objfile *objfile)
> {
> ...
>   CORE_ADDR mask = CORE_ADDR_MAX;
> 
>   if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
>     mask = ((CORE_ADDR) 1 << addr_bit) - 1;
> ...
>       sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
>                             + objfile->section_offsets->offsets[i]) & mask;
> 
> This truncates the high bits.  MIPS sign-extends pointers, even
> internally in CORE_ADDR, and this results in separate debug info files
> for MIPS executables being relocated off to la-la land.  I had to add
> this awful thing:
> 
>       if (bfd_get_sign_extend_vma (objfile->obfd)
>           && addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT)
>           && (sap->other[i].addr & ((CORE_ADDR) 1 << (addr_bit - 1))) != 0)
>         sap->other[i].addr |= ~mask;
> 
> Which I'm not really proposing for inclusion, well, unless no one has
> a better idea; sepdebug.exp on mips-elf currently fails without this.

Perhaps we should introduce a function to "normalize" addresses (mask
off high-bits or sign extend) that we call in places that need it?
It'd be a no-op for a N-bit debugger debugging an N-bit target, so
you'd be able to call it unconditionally.  That should clear away
quite a bit of clutter.

> For instance, should we always internally sign-extend CORE_ADDR?
> Always internally zero-extend?  Having it vary by target has been a
> recurring problem.

The problem is that BFD may still give you sign-extended addresses.
So you'd have to normalize them before sticking them into a CORE_ADDR.

Re: CORE_ADDR representation

[Jan Kratochvil]

On Thu, 18 Feb 2010 05:44:19 +0100, Daniel Jacobowitz wrote:
> struct section_addr_info *
> build_section_addr_info_from_objfile (const struct objfile *objfile)
> {
> ...
>   CORE_ADDR mask = CORE_ADDR_MAX;
> 
>   if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
>     mask = ((CORE_ADDR) 1 << addr_bit) - 1;
> ...
>       sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
>                             + objfile->section_offsets->offsets[i]) & mask;
> 
> This truncates the high bits.  MIPS sign-extends pointers, even
> internally in CORE_ADDR, and this results in separate debug info files
> for MIPS executables being relocated off to la-la land.

If we follow Mark Kettenis's suggestion on 64bit arithmetics for 32bit
inferiors.
	Re: [patch] bfd/: bfd_elf_bfd_from_remote_memory 32bit &= 0xffffffff
	http://sourceware.org/ml/gdb-patches/2010-02/msg00286.html
which should work now with checked-in
	[patch] Fix PIE for 64bit gdb -> 32bit inferior
	http://sourceware.org/ml/gdb-patches/2010-02/msg00289.html
(+ a similar fix may be needed even elsewhere)

these "& mask" parts can be removed.  This masking was there already before
start of the PIE(+OSX) patches.

I thought about their removal only just as a simplification in future but it
looks to be required for mips*.  Downloading some mips .iso if it will run in
qemu to test it.


Thanks,
Jan

Re: CORE_ADDR representation

[Daniel Jacobowitz]

On Thu, Feb 18, 2010 at 11:34:04AM +0100, Jan Kratochvil wrote:
> these "& mask" parts can be removed.  This masking was there already before
> start of the PIE(+OSX) patches.

I don't understand.  How can the masking possibly be removed?  If you
don't mask, 0x50000000 + 0x40000000 == 0x90000000 and that's not going
to work on MIPS where we need 0xffffffff90000000.

> I thought about their removal only just as a simplification in future but it
> looks to be required for mips*.  Downloading some mips .iso if it will run in
> qemu to test it.

This won't show the same problem, you'll need to use mips-elf instead.
MIPS Linux places application code below 0x80000000 exclusively; MIPS
ELF (at least some versions) starts applications in KSEG0, at
0x80000000.

-- 
Daniel Jacobowitz
CodeSourcery

Re: CORE_ADDR representation

[Daniel Jacobowitz]

On Thu, Feb 18, 2010 at 11:17:14AM +0100, Mark Kettenis wrote:
> Perhaps we should introduce a function to "normalize" addresses (mask
> off high-bits or sign extend) that we call in places that need it?
> It'd be a no-op for a N-bit debugger debugging an N-bit target, so
> you'd be able to call it unconditionally.  That should clear away
> quite a bit of clutter.

That does sound better than the status quo.  I worry that we'll have
otherwise the same trouble with figuring out places that 'need' it...

Hmm.  I wonder if we could use a static analysis tool for this.  It
sounds like a classic example of a static problem.

-- 
Daniel Jacobowitz
CodeSourcery

Re: CORE_ADDR representation

[Jan Kratochvil]

On Thu, 18 Feb 2010 14:41:41 +0100, Daniel Jacobowitz wrote:
> On Thu, Feb 18, 2010 at 11:34:04AM +0100, Jan Kratochvil wrote:
> > these "& mask" parts can be removed.  This masking was there already before
> > start of the PIE(+OSX) patches.
> 
> I don't understand.  How can the masking possibly be removed?  If you
> don't mask, 0x50000000 + 0x40000000 == 0x90000000 and that's not going
> to work on MIPS where we need 0xffffffff90000000.

OK, my mistake.

(You wrote somewhere MIPS32 uses only lower 2GB, expected higher 2GB is used
for some special purposes when you care about it and those lower and higher
2GB do not mix.)


> This won't show the same problem, you'll need to use mips-elf instead.
> MIPS Linux places application code below 0x80000000 exclusively; MIPS
> ELF (at least some versions) starts applications in KSEG0, at
> 0x80000000.

OK, that explains those "lower 2GB" on MIPS32.


There can be CORE_ADDR-as-address which would be always zero-extended and
CORE_ADDR-as-displacement which would be always sign-extended.  Having them as
scalar types will not sanity check the code is right during compilation but it
would not need replacing all CORE_ADDR '+'/'-'es by functions calls as in:
	http://sourceware.org/ml/gdb-patches/2010-02/msg00415.html
	(the 200KB patch with addr_add_offset&.co.)


Thanks,
Jan

Re: CORE_ADDR representation

[Daniel Jacobowitz]

On Wed, Feb 17, 2010 at 09:41:51PM -0800, Stan Shebs wrote:
> I would say to declare that CORE_ADDR is fundamentally 0..memtop, so
> it should be unsigned and zero-extend.
> 
> Can unsigned->signed->diddle->unsigned be encapsulated for MIPS only?

I don't know that it can't.  I think I discussed this with Andrew many
years ago, and he thought that it couldn't; but I can't find the details...

-- 
Daniel Jacobowitz
CodeSourcery

Re: CORE_ADDR representation

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <dan@codesourcery.com> writes:

>> I would say to declare that CORE_ADDR is fundamentally 0..memtop, so
>> it should be unsigned and zero-extend.
>> 
>> Can unsigned->signed->diddle->unsigned be encapsulated for MIPS only?

Daniel> I don't know that it can't.  I think I discussed this with
Daniel> Andrew many years ago, and he thought that it couldn't; but I
Daniel> can't find the details...

Maybe this thread: http://sourceware.org/ml/gdb/2002-09/threads.html#00078

Tom

Re: CORE_ADDR representation

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <dan@codesourcery.com> writes:

Mark> Perhaps we should introduce a function to "normalize" addresses (mask
Mark> off high-bits or sign extend) that we call in places that need it?
Mark> It'd be a no-op for a N-bit debugger debugging an N-bit target, so
Mark> you'd be able to call it unconditionally.  That should clear away
Mark> quite a bit of clutter.

Daniel> That does sound better than the status quo.  I worry that we'll have
Daniel> otherwise the same trouble with figuring out places that 'need' it...

Daniel> Hmm.  I wonder if we could use a static analysis tool for this.  It
Daniel> sounds like a classic example of a static problem.

The appended finds all uses of "+" on a CORE_ADDR.  Well... it finds
"all" uses modulo whatever little issues coccinelle has, I didn't mess
around trying to make it look into macros.

I ran it like:

  cd src/gdb
  spatch -sp_file coreaddr.cocci -dir .

... and got 1071 hits.

On Fedora you can get the tool with "yum install coccinelle", I assume
other distros are similar.  Also it is here:

    http://coccinelle.lip6.fr/

I'm not sure if this was what you're really looking for, but it isn't
too hard to modify this script to look for other things.

The problem with doing static analysis is that you have to redo it
pretty frequently.  While moving to a struct CORE_ADDR would result in
more verbose code (which is definitely bad, don't get me wrong), it does
have the benefit that an attempt to do arithmetic on it results in a
compiler error.  This in turns means it is simpler to review the
resulting patches.

Tom

@ coreaddr
@
CORE_ADDR x;
expression y;
position p_1;
@@
x @p_1 + y
@
script:python @ loc_1 << coreaddr.p_1;
	@@
print "%s:%s:%s: CORE_ADDR + operation" % (loc_1[0].file, loc_1[0].line, loc_1[0].column)