[parisc-linux] Comments?

[parisc-linux] Comments?

[Carlos O'Donell]

jda, tausq,

Comments on the assembly? 

I'm cleaning up the libc trampoline routines that are called during lazy
symbol resolution. We need to make changes to the profile version in
order to support library auditing.

The complete mechanics of this function call aren't that important, I'm
looking for comments on any ABI bits I missed.

For those that like to know mechanics:

a. In early ELF setup code we know we are going to use _dl_fixup from
   a particular shared object, so we plunk down the ltp for that code
   into the PLT.

b. Then when you call a function that hasn't been resolved you get
   bounced through the PLT to the trampoline, and not the function.
   This is lazy resolution. At startup we didn't bother to bind all
   the symbols, instead we filled the PLT with bounces to the trampoline
   routine (and eventually this leads to symbol resolution).

c. The bounce from the PLT to the trampoline is a bit of code at the
   end of the PLT. It loads up the following parameters and calls the
   trampoline with a non-standard non-abi function call (it doesn't 
   make a stack/frame or use the proper registers).

   Trampoline parameters:
	r19 = Relocation offset.
	r20 = Somwhere in the GOT (used to get your own link_map)
	r21 = _dl_fixup's PIC register value (ltp)

   Trampoline calls _dl_fixup with:
	r26 = got[1] (your own link_map)
	r25 = relocation offset

   If you are profiling:
	r24 = contains your rp.
	The stack has all the library auditing parameters.

d. _dl_fixup uses the relocation offset, and the link_map to find the
   symbol you need. Then it sets up the PLT so this doesn't happen
   again and then returns. 

   If we are profiling, there is the posibility that the code can call
   one of two functions PLTENTER or PLTEXIT. That is a user can register
   a set of functions to be called as you enter the PLT and as you exit 
   the PLT. This allows user code to audit the loading process.

   TODO: Working on this last bit, hence the note in the assemble.

e. Walking the bottom half of the trampoline restores the users
   arguments they were passing to the function in the first place
   and continues execution.

---

/* PLT trampolines. hppa version.
   Copyright (C) 2005 Free Software Foundation, Inc.
   This file is part of the GNU C Library.

   The GNU C Library is free software; you can redistribute it and/or
   modify it under the terms of the GNU Lesser General Public
   License as published by the Free Software Foundation; either
   version 2.1 of the License, or (at your option) any later version.

   The GNU C Library is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
   Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public
   License along with the GNU C Library; if not, write to the Free
   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
   02111-1307 USA.  */

#include <sysdep.h>

/* This code gets called via the .plt stub, and is used in
   dl-runtime.c to call the `_dl_fixup' function and then redirect 
   to the    address it returns. `_dl_fixup' takes two
   arguments, however `_dl_profile_fixup' takes a number of 
   parameters for use with library auditing (LA).
   
   WARNING: This template is also used by gcc's __cffc, and expects
   that the "bl" for _dl_runtime_resolve exist at a particular offset.
   Do not change this template without changing gcc, while the prefix
   "bl" should fix everything so gcc finds the right spot, it will
   slow down __cffc when it attempts to call fixup to resolve function
   descriptor references. Please refer to gcc/gcc/config/pa/fptr.c
   
   Enter with r19 = reloc offset, r20 = got-8, r21 = fixup ltp.  */

	/* FAKE bl to provide gcc's __cffc with fixup loc. */
	.text
	bl	_dl_fixup, %r2
        .text
        .align 4
        .global _dl_runtime_resolve
        .type _dl_runtime_resolve,@function
_dl_runtime_resolve:
        .PROC
        .CALLINFO FRAME=64,CALLS,SAVE_RP,ENTRY_GR=3
        .ENTRY
        /* SAVE_RP says we do */
        stw %rp, -20(%sr0,%sp)

 	/* Save argument registers in the call stack frame. */
	stw	%r26,-36(%sp)
	stw	%r25,-40(%sp)
	stw	%r24,-44(%sp)
	stw	%r23,-48(%sp)

	/* Build a call frame, and save structure pointer. */
	copy	%sp, %r26	/* Copy previous sp */
	stwm	%r28,64(%sp)

	/* Fillin some frame info to follow ABI */
	stw	%rp,-20(%sp)	/* Set a reasonable rp */
	stw	%r26,-4(%sp)	/* Save previous sp */

 	/* Set up args to fixup func, needs only two arguments  */
	ldw	8+4(%r20),%r26		/* (1) got[1] == struct link_map */
	copy	%r19,%r25		/* (2) reloc offset  */

 	/* Call the real address resolver. */
	bl	_dl_fixup,%r2
	copy	%r21,%r19		/* set fixup func ltp */

	/* Load up the returned func ptr */
	ldw	0(%ret0),%r22		
	ldw	4(%ret0),%r19

	/* Adjust the stack */
	ldwm	-64(%sp),%r28

	/* Reload arguments. */
	ldw	-36(%sp),%r26
	ldw	-40(%sp),%r25
	ldw	-44(%sp),%r24
	ldw	-48(%sp),%r23

	/* Return */
	bv	%r0(%r22)
	ldw	-20(%sp),%r2
        .EXIT
        .PROCEND
	.size   _dl_runtime_resolve, . - _dl_runtime_resolve


	/* FIXME:
		Need to largely rewrite the bottom half of
		this code in order to save and restore the
		LA struct from the stack along with
		interpreted parameters.
	*/
        .text
        .align 4
        .global _dl_runtime_profile
        .type _dl_runtime_profile,@function
_dl_runtime_profile:
        .PROC
        .CALLINFO FRAME=64,CALLS,SAVE_RP,ENTRY_GR=3
        .ENTRY
        /* SAVE_RP says we do */
        stw %rp, -20(%sr0,%sp)

 	/* Save argument registers in the call stack frame. */
	stw	%r26,-36(%sp)
	stw	%r25,-40(%sp)
	stw	%r24,-44(%sp)
	stw	%r23,-48(%sp)

	/* Build a call frame, and save structure pointer. */
	copy	%sp, %r26	/* Copy previous sp */
	stwm	%r28,64(%sp)

	/* Fillin some frame info to follow ABI */
	stw	%rp,-20(%sp)	/* Set a reasonable rp */
	stw	%r26,-4(%sp)	/* Save previous sp */

 	/* Set up args to fixup func, needs three arguments  */
	ldw	8+4(%r20),%r26		/* (1) got[1] == struct link_map */
	copy	%r19,%r25		/* (2) reloc offset  */
	copy    %rp,%r24		/* (3) profile_fixup needs rp */

 	/* Call the real address resolver. */
	bl	_dl_profile_fixup,%rp
	copy	%r21,%r19		/* set profile_fixup func ltp */

	/* Load up the returned func ptr */
	ldw	0(%ret0),%r22		
	ldw	4(%ret0),%r19

	/* Adjust the stack */
	ldwm	-64(%sp),%r28

	/* Reload arguments. */
	ldw	-36(%sp),%r26
	ldw	-40(%sp),%r25
	ldw	-44(%sp),%r24
	ldw	-48(%sp),%r23

	/* Return */
	bv	%r0(%r22)
	ldw	-20(%sp),%r2
        .EXIT
        .PROCEND
	.size   _dl_runtime_profile, . - _dl_runtime_profile



_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> 	/* Build a call frame, and save structure pointer. */
> 	copy	%sp, %r26	/* Copy previous sp */
> 	stwm	%r28,64(%sp)

r28?  I'm thinking that the static chain (r29) and struct value
(r28) registers need to be saved as well as r23-r26.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[Carlos O'Donell]

On Wed, Mar 02, 2005 at 02:41:44PM -0500, John David Anglin wrote:
> > 	/* Build a call frame, and save structure pointer. */
> > 	copy	%sp, %r26	/* Copy previous sp */
> > 	stwm	%r28,64(%sp)
> 
> r28?  I'm thinking that the static chain (r29) and struct value
> (r28) registers need to be saved as well as r23-r26.

To be truthful I'm only saving r28 because in the legacy code we saved
r28. I didn't know our toolchain used r28 for function entry or r29 for
function entry.

Are you suggesting this because we could use them in the future?

I know that r29 is the "static link register" for entry.
I don't understand the full purpose of r28 which is the "function result
address" on entry.

This is exactly the type of feedback I wanted! Thanks.

What do I do about the floating point registers?

Cheers,
Carlos.

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> On Wed, Mar 02, 2005 at 02:41:44PM -0500, John David Anglin wrote:
> > > 	/* Build a call frame, and save structure pointer. */
> > > 	copy	%sp, %r26	/* Copy previous sp */
> > > 	stwm	%r28,64(%sp)
> > 
> > r28?  I'm thinking that the static chain (r29) and struct value
> > (r28) registers need to be saved as well as r23-r26.
> 
> To be truthful I'm only saving r28 because in the legacy code we saved
> r28. I didn't know our toolchain used r28 for function entry or r29 for
> function entry.

Yes, it uses both.  I now see that the code is saving and
restoring r28.

> Are you suggesting this because we could use them in the future?
> 
> I know that r29 is the "static link register" for entry.
> I don't understand the full purpose of r28 which is the "function result
> address" on entry.

r28 is used to pass the return address for structures larger
than 8 bytes.  Use of r29 on input is fairly rare.

GCC will clobber r28 and r29.  Thus, you need to save both.

> What do I do about the floating point registers?

Hmmm, there is one circumstance where floating point registers
can be used in integer code.  That's for the xmpyu instruction.
Thus, you should either save the argument registers, or compile
the code that does symbol resolution using -mdisable-fpregs,
or with GCC 4.0 or later compile with -mfixed-range specifying
the fp argument registers as fixed.  The latter will stop GCC
from using the argument fp argument registers.  Since the path
is only used once per function call, it's probably best to save
all the possible argument registers.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[Carlos O'Donell]

On Wed, Mar 02, 2005 at 05:38:41PM -0500, John David Anglin wrote:
> > To be truthful I'm only saving r28 because in the legacy code we saved
> > r28. I didn't know our toolchain used r28 for function entry or r29 for
> > function entry.
> 
> Yes, it uses both.  I now see that the code is saving and
> restoring r28.

Yes, I store/load it into/from the stack.  I like stwm since it
automatically adjusts the stack and gives you a free register to play
with.
 
> r28 is used to pass the return address for structures larger
> than 8 bytes.  Use of r29 on input is fairly rare.

Ok.
 
> GCC will clobber r28 and r29.  Thus, you need to save both.

Ok. I will save both.

> > What do I do about the floating point registers?
> 
> Hmmm, there is one circumstance where floating point registers
> can be used in integer code.  That's for the xmpyu instruction.
> Thus, you should either save the argument registers, or compile
> the code that does symbol resolution using -mdisable-fpregs,
> or with GCC 4.0 or later compile with -mfixed-range specifying
> the fp argument registers as fixed.  The latter will stop GCC
> from using the argument fp argument registers.  Since the path
> is only used once per function call, it's probably best to save
> all the possible argument registers.

There are a lot of files that do symbol resolution. I think I might just
save the fp argument registers, and thus allow gcc to do any sorts of
optimizations it wishes with therest of the code, knowing that the
arguments are all saved and restored on the initial function call.

Thanks.


I have to get all the LA patches added. Test that. Then add the cfi
directives and test Randolphs cfi patch.

c.

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> There are a lot of files that do symbol resolution. I think I might just
> save the fp argument registers, and thus allow gcc to do any sorts of
> optimizations it wishes with therest of the code, knowing that the
> arguments are all saved and restored on the initial function call.

Sounds good.

I think we have somehow broken __cffc.  I did a binutils build
yesterday and various ld tests are now failing.  These are tests
that compare function pointers.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[Carlos O'Donell]

On Wed, Mar 02, 2005 at 10:21:49PM -0500, John David Anglin wrote:
> > There are a lot of files that do symbol resolution. I think I might just
> > save the fp argument registers, and thus allow gcc to do any sorts of
> > optimizations it wishes with therest of the code, knowing that the
> > arguments are all saved and restored on the initial function call.
> 
> Sounds good.
> 
> I think we have somehow broken __cffc.  I did a binutils build
> yesterday and various ld tests are now failing.  These are tests
> that compare function pointers.

I didn't feed anything new into debian-glibc. All my patches have always
strived to keep __cffc working, if I messup the function pointer
comparison I get a slew of glibc testsuite failures which I always
patchup by making sure __cffc works :)

The last botch was a minor change in the trampoline template, which
prompted the addition of the 'bl' before the function that made use of
the magic '-4' check in __cffc.

c.

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> > I think we have somehow broken __cffc.  I did a binutils build
> > yesterday and various ld tests are now failing.  These are tests
> > that compare function pointers.
> 
> I didn't feed anything new into debian-glibc. All my patches have always
> strived to keep __cffc working, if I messup the function pointer
> comparison I get a slew of glibc testsuite failures which I always
> patchup by making sure __cffc works :)

I haven't touched fptr.c either, although there is a change in GCC
4.0 with respect to canonicalization.  I'm using the debian glibc,
2.3.2.ds1-20, on the system on which I noticed this problem.

2004-12-12  Nathanael Nerode  <neroden@gcc.gnu.org>
	    John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>

	PR middle-end/17564
	* dojump.c (do_compare_and_jump): Only canonicalize function pointers
	in a comparison if both sides are function pointers.

I checked that the binutils testsuite fails with 3.4, so I don't think
the problem is a GCC issue.  I was going to try and see what's happening
this afternoon.  I've been working on some testsuite cleanups today.

Another thing that I need to figure out is the cause of the libstdc++
fails:

hiauly6 4.0.0:
FAIL: 27_io/basic_filebuf/sgetn/char/1-in.cc execution test
FAIL: 27_io/basic_filebuf/sgetn/char/1-io.cc execution test
FAIL: 27_io/basic_filebuf/sgetn/char/2-in.cc execution test
FAIL: 27_io/basic_filebuf/sgetn/char/2-io.cc execution test
FAIL: 27_io/basic_filebuf/underflow/wchar_t/11603.cc execution test
FAIL: 27_io/basic_istream/readsome/char/6746-2.cc execution test
FAIL: 27_io/basic_istream/readsome/wchar_t/6746-2.cc execution test

gsyprf11 4.1.0:
FAIL: 22_locale/messages/members/char/1.cc execution test
FAIL: 22_locale/messages/members/char/2.cc execution test
FAIL: 22_locale/messages/members/char/wrapped_env.cc execution test
FAIL: 22_locale/messages/members/char/wrapped_locale.cc execution test
FAIL: 22_locale/messages_byname/named_equivalence.cc execution test

It's strange that the two systems have a completely different
sets of testsuite fails.  There aren't any fails under hpux.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[Carlos O'Donell]

On Sat, Mar 05, 2005 at 03:33:20PM -0500, John David Anglin wrote:
> > > I think we have somehow broken __cffc.  I did a binutils build
> > > yesterday and various ld tests are now failing.  These are tests
> > > that compare function pointers.
> > 
> > I didn't feed anything new into debian-glibc. All my patches have always
> > strived to keep __cffc working, if I messup the function pointer
> > comparison I get a slew of glibc testsuite failures which I always
> > patchup by making sure __cffc works :)
> 
> I haven't touched fptr.c either, although there is a change in GCC
> 4.0 with respect to canonicalization.  I'm using the debian glibc,
> 2.3.2.ds1-20, on the system on which I noticed this problem.

I'm heavily leaning towards the addition/transition to OPD's for even
the 32-bit code. I wrote a document somewhere about handling that
transition transparently from the point of view of binutils/glibc.

I don't quite understand how to transition the use of __cffc in gcc over
to some sort of "simple compare."

I'll rustle up that document at some point. Right now I'm finding time
to finish testing the LA patches for glibc and move on to binutils
again.

As for the stdc++ failures, I'm not sure yet what causes them. Perhaps
I'll kick off a build.

c.

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> Another thing that I need to figure out is the cause of the libstdc++
> fails:
> 
> hiauly6 4.0.0:
> FAIL: 27_io/basic_filebuf/sgetn/char/1-in.cc execution test

This fail is caused by a bug in ioctl when using a 32-bit kernel.
I've attached a little test program below that demonstrates the problem.
sgetn.txt can be found in libstdc++-v3/testsuite/data/sgetn.txt but
I think any file will do.  num is still 0 after the ioctl call.

> gsyprf11 4.1.0:
> FAIL: 22_locale/messages/members/char/1.cc execution test
> FAIL: 22_locale/messages/members/char/2.cc execution test
> FAIL: 22_locale/messages/members/char/wrapped_env.cc execution test
> FAIL: 22_locale/messages/members/char/wrapped_locale.cc execution test
> FAIL: 22_locale/messages_byname/named_equivalence.cc execution test

These are a result of configuring GCC with --disable-nls.  It's
a minor configuration issue.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <sys/ioctl.h>
int
main ()
{
  int fd, result;
  int num = 0;

  fd = open ("sgetn.txt", O_RDONLY);
  result = ioctl (fd, FIONREAD, &num);
  printf ("num = %d\n\n", num);
  return 0;
}
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[Randolph Chung]

> > hiauly6 4.0.0:
> > FAIL: 27_io/basic_filebuf/sgetn/char/1-in.cc execution test
> 
> This fail is caused by a bug in ioctl when using a 32-bit kernel.
> I've attached a little test program below that demonstrates the problem.
> sgetn.txt can be found in libstdc++-v3/testsuite/data/sgetn.txt but
> I think any file will do.  num is still 0 after the ioctl call.

I think the attached patch is what we should do; the problem is that we
are trying to put a 64-bit quantity (loff_t) into an int *, and we end
up only storing the top bits. (see fs/ioctl.c for details)

Is this the right thing to do? Do we need something similar for
get_user()?

Tested on 32-bit and 64-bit (thanks Kyle!)

randolph

Index: include/asm-parisc/uaccess.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/uaccess.h,v
retrieving revision 1.18
diff -u -p -r1.18 uaccess.h
--- include/asm-parisc/uaccess.h	21 Feb 2005 16:40:53 -0000	1.18
+++ include/asm-parisc/uaccess.h	13 Mar 2005 00:50:41 -0000
@@ -147,22 +147,23 @@ struct exception_data {
 #define __put_user(x,ptr)                                       \
 ({								\
 	register long __pu_err __asm__ ("r8") = 0;      	\
+        __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x);	\
 								\
 	if (segment_eq(get_fs(),KERNEL_DS)) {                   \
 	    switch (sizeof(*(ptr))) {                           \
-	    case 1: __put_kernel_asm("stb",x,ptr); break;       \
-	    case 2: __put_kernel_asm("sth",x,ptr); break;       \
-	    case 4: __put_kernel_asm("stw",x,ptr); break;       \
-	    case 8: STD_KERNEL(x,ptr); break;			\
+	    case 1: __put_kernel_asm("stb",__x,ptr); break;     \
+	    case 2: __put_kernel_asm("sth",__x,ptr); break;     \
+	    case 4: __put_kernel_asm("stw",__x,ptr); break;     \
+	    case 8: STD_KERNEL(__x,ptr); break;			\
 	    default: __put_kernel_bad(); break;			\
 	    }                                                   \
 	}                                                       \
 	else {                                                  \
 	    switch (sizeof(*(ptr))) {                           \
-	    case 1: __put_user_asm("stb",x,ptr); break;         \
-	    case 2: __put_user_asm("sth",x,ptr); break;         \
-	    case 4: __put_user_asm("stw",x,ptr); break;         \
-	    case 8: STD_USER(x,ptr); break;			\
+	    case 1: __put_user_asm("stb",__x,ptr); break;       \
+	    case 2: __put_user_asm("sth",__x,ptr); break;       \
+	    case 4: __put_user_asm("stw",__x,ptr); break;       \
+	    case 8: STD_USER(__x,ptr); break;			\
 	    default: __put_user_bad(); break;			\
 	    }                                                   \
 	}                                                       \
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

>  	register long __pu_err __asm__ ("r8") = 0;      	\
> +        __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x);	\

This is an improvement as long as the ioctl handling checks that
the loff_t value can be successfully truncated.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Re: Comments?

[Joel Soete]

Randolph Chung wrote:
...
> Is this the right thing to do? Do we need something similar for
> get_user()?
> 
would you mean something like:
--- include/asm-parisc/uaccess.h.orig   2005-02-26 13:23:35.000000000 +0100
+++ include/asm-parisc/uaccess.h        2005-03-13 13:20:11.000000000 +0100
@@ -102,8 +102,9 @@
             }                                           \
         }                                               \
                                                         \
-       (x) = (__typeof__(*(ptr))) __gu_val;            \
-       __gu_err;                                       \
+        __typeof__(*(ptr)) __x = (__typeof__(*(ptr))) __gu_val;        \
+       (x) = (__typeof__((x))) (__x);                          \
+       __gu_err;                                               \
  })

  #ifdef __LP64__
====><====

> Tested on 32-bit and 64-bit (thanks Kyle!)
> 
Thanks,
	Joel
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> I think the attached patch is what we should do; the problem is that we
> are trying to put a 64-bit quantity (loff_t) into an int *, and we end
> up only storing the top bits. (see fs/ioctl.c for details)
> 
> Is this the right thing to do? Do we need something similar for
> get_user()?

Thinking more about this, I've decided that this change would
tend to hide problems.  The reality is that fs/ioctl.c has to
deal with how to convert the loff_t value to an int (e.g., return
INT_MAX for positive offsets exceeding INT_MAX, or return an
error, etc).  Truncation likely isn't the right thing to do
in all cases.

I don't know what is required here as the behavior of FIONREAD
for large offsets isn't defined.  Possibly, the issue is
academic in this case as it may not be possible to have more
than 2 GB of unread queued data.

GCC has a builtin __builtin_types_compatible_p that checks if
two types are compatible.  It might be used in a debug kernel
to look for other similar problems.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[Randolph Chung]

> Thinking more about this, I've decided that this change would
> tend to hide problems.  The reality is that fs/ioctl.c has to
> deal with how to convert the loff_t value to an int (e.g., return
> INT_MAX for positive offsets exceeding INT_MAX, or return an
> error, etc).  Truncation likely isn't the right thing to do
> in all cases.

I suppose the behavior should, ideally, match some specification.
Unfortunately the only place I can find where FIONREAD is documented is
in LSB, and the description there is quite value about what the return
type is supposed to be.

For now I've commited a combination of:
1) Change the FIONREAD handling code to explicitly return a 4-byte value
2) Change the put_user code to cast the result to the expected type

Willy says he will follow up with other relevant folks to find out
what's the right thing to do.

randolph
-- 
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> For now I've commited a combination of:
> 1) Change the FIONREAD handling code to explicitly return a 4-byte value
> 2) Change the put_user code to cast the result to the expected type

I got a bit paranoid about __put_user after the above bug.  Looking at
it, I believe that the LP64 version is broken for *(ptr) types of long,
unsigned long, and pointer types.  I believe that the current code
puts a 64-bit value instead of the 32-bit value that the 32-bit runtime
expects for these types.  This is fixed by adding a layer over the
old __put_user macro to do various type checking.

The above change also introduced some new warnings when building a
32-bit kernel.  These occur when the *(ptr) type is a pointer (e.g.,
void *).  The u64 cast in the size = 8 code is undefined, although
the code is actually used.  The current code is also questionable
when x is a pointer type and *(ptr) is a 64-bit type.  This is probably
an unlikely combination.  I believe that these issues are fixed by
the __put_user_cast macro.

The rest of the changes are just warning fixes that I noticed during
testing.  I don't believe that they have a functional effect.

I tested this with 32 and 64-bit builds of 2.6.12-rc1-pa1 with the
default c3000 and 64 configs.  I am running the 32-bit build and a
slighter earlier version survived a gcc build.  I haven't run the
64-bit changes.  I now don't see any cast related errors in the builds
except one in a call to atomic_read.  That's an unrelated problem.

Install if you like.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

? include/asm-parisc/uaccess.h.save
Index: drivers/net/lasi_82596.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/net/lasi_82596.c,v
retrieving revision 1.15
diff -u -p -u -3 -p -r1.15 lasi_82596.c
--- drivers/net/lasi_82596.c	29 Nov 2004 19:56:12 -0000	1.15
+++ drivers/net/lasi_82596.c	22 Mar 2005 01:55:27 -0000
@@ -640,7 +640,7 @@ static int init_i596_mem(struct net_devi
 		       (void*)(dev->base_addr + PA_I82596_RESET),
 		       dev->irq));
 	
-	gsc_writel(0, (void*)(dev->base_addr + PA_I82596_RESET)); /* Hard Reset */
+	gsc_writel(0, (unsigned long)(dev->base_addr + PA_I82596_RESET)); /* Hard Reset */
 	udelay(100);			/* Wait 100us - seems to help */
 
 	/* change the scp address */
Index: drivers/parisc/ccio-dma.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/parisc/ccio-dma.c,v
retrieving revision 1.18
diff -u -p -u -3 -p -r1.18 ccio-dma.c
--- drivers/parisc/ccio-dma.c	6 Mar 2005 23:48:39 -0000	1.18
+++ drivers/parisc/ccio-dma.c	22 Mar 2005 01:55:27 -0000
@@ -101,7 +101,7 @@
 #endif
 
 #define CCIO_INLINE	/* inline */
-#define WRITE_U32(value, addr) gsc_writel(value, (u32 *)(addr))
+#define WRITE_U32(value, addr) gsc_writel(value, (unsigned long)(addr))
 #define READ_U32(addr) gsc_readl((u32 *)(addr))
 
 #define U2_IOA_RUNWAY 0x580
@@ -1391,12 +1391,13 @@ ccio_ioc_init(struct ioc *ioc)
 static void
 ccio_init_resource(struct resource *res, char *name, unsigned long ioaddr)
 {
+	void *addr = (void *) ioaddr;
 	int result;
 
 	res->parent = NULL;
 	res->flags = IORESOURCE_MEM;
-	res->start = (unsigned long)(signed) __raw_readl(ioaddr) << 16;
-	res->end = (unsigned long)(signed) (__raw_readl(ioaddr + 4) << 16) - 1;
+	res->start = (unsigned long)(signed) __raw_readl(addr) << 16;
+	res->end = (unsigned long)(signed) (__raw_readl(addr + 4) << 16) - 1;
 	res->name = name;
 	if (res->end + 1 == res->start)
 		return;
@@ -1486,15 +1487,15 @@ int ccio_allocate_resource(const struct 
 
 	if (!expand_ioc_area(parent, size, min, max, align)) {
 		__raw_writel(((parent->start)>>16) | 0xffff0000,
-			     (unsigned long)&(ioc->ioc_hpa->io_io_low));
+			     (void *)&(ioc->ioc_hpa->io_io_low));
 		__raw_writel(((parent->end)>>16) | 0xffff0000,
-			     (unsigned long)&(ioc->ioc_hpa->io_io_high));
+			     (void *)&(ioc->ioc_hpa->io_io_high));
 	} else if (!expand_ioc_area(parent + 1, size, min, max, align)) {
 		parent++;
 		__raw_writel(((parent->start)>>16) | 0xffff0000,
-			     (unsigned long)&(ioc->ioc_hpa->io_io_low_hv));
+			     (void *)&(ioc->ioc_hpa->io_io_low_hv));
 		__raw_writel(((parent->end)>>16) | 0xffff0000,
-			     (unsigned long)&(ioc->ioc_hpa->io_io_high_hv));
+			     (void *)&(ioc->ioc_hpa->io_io_high_hv));
 	} else {
 		return -EBUSY;
 	}
Index: drivers/parisc/hppb.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/parisc/hppb.c,v
retrieving revision 1.5
diff -u -p -u -3 -p -r1.5 hppb.c
--- drivers/parisc/hppb.c	2 Mar 2005 06:47:37 -0000	1.5
+++ drivers/parisc/hppb.c	22 Mar 2005 01:55:27 -0000
@@ -74,8 +74,8 @@ static int hppb_probe(struct parisc_devi
 	card->mmio_region.name = "HP-PB Bus";
 	card->mmio_region.flags = IORESOURCE_MEM;
 
-	card->mmio_region.start = __raw_readl(dev->hpa + IO_IO_LOW);
-	card->mmio_region.end = __raw_readl(dev->hpa + IO_IO_HIGH) - 1;
+	card->mmio_region.start = __raw_readl((void *)(dev->hpa + IO_IO_LOW));
+	card->mmio_region.end = __raw_readl((void *)(dev->hpa + IO_IO_HIGH)) - 1;
 
 	status = ccio_request_resource(dev, &card->mmio_region);
 	if(status < 0) {
Index: drivers/scsi/lasi700.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/scsi/lasi700.c,v
retrieving revision 1.12
diff -u -p -u -3 -p -r1.12 lasi700.c
--- drivers/scsi/lasi700.c	15 Mar 2005 12:40:22 -0000	1.12
+++ drivers/scsi/lasi700.c	22 Mar 2005 01:55:27 -0000
@@ -112,7 +112,7 @@ lasi700_probe(struct parisc_device *dev)
 
 	hostdata->dev = &dev->dev;
 	dma_set_mask(&dev->dev, DMA_32BIT_MASK);
-	hostdata->base = ioremap(base, 0x100);
+	hostdata->base = (unsigned long) ioremap(base, 0x100);
 	hostdata->differential = 0;
 
 	if (dev->id.sversion == LASI_700_SVERSION) {
@@ -138,7 +138,7 @@ lasi700_probe(struct parisc_device *dev)
 	return 0;
 
  out_kfree:
-	iounmap(hostdata->base);
+	iounmap((void *)hostdata->base);
 	kfree(hostdata);
 	return -ENODEV;
 }
@@ -153,7 +153,7 @@ lasi700_driver_remove(struct parisc_devi
 	scsi_remove_host(host);
 	NCR_700_release(host);
 	free_irq(host->irq, host);
-	iounmap(hostdata->base);
+	iounmap((void *)hostdata->base);
 	kfree(hostdata);
 
 	return 0;
Index: drivers/video/stifb.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/video/stifb.c,v
retrieving revision 1.15
diff -u -p -u -3 -p -r1.15 stifb.c
--- drivers/video/stifb.c	29 Nov 2004 20:42:47 -0000	1.15
+++ drivers/video/stifb.c	22 Mar 2005 01:55:27 -0000
@@ -517,7 +517,7 @@ rattlerSetupPlanes(struct stifb_info *fb
 	SETUP_HW(fb);
 	WRITE_BYTE(1, fb, REG_16b1);
 
-	fb_memset(fb->info.fix.smem_start, 0xff,
+	fb_memset((void *)fb->info.fix.smem_start, 0xff,
 		fb->info.var.yres*fb->info.fix.line_length);
     
 	CRX24_SET_OVLY_MASK(fb);
@@ -977,7 +977,7 @@ stifb_write(struct file *file, const cha
 	    err = -EFAULT;
 	    if (copy_from_user(&tmpbuf, buf, len))
 		    break;
-	    memcpy_toio(p, &tmpbuf, len);
+	    memcpy_toio((void *)p, &tmpbuf, len);
 	    c -= len;
 	    p += len;
 	    buf += len;
Index: include/asm-parisc/uaccess.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/asm-parisc/uaccess.h,v
retrieving revision 1.20
diff -u -p -u -3 -p -r1.20 uaccess.h
--- include/asm-parisc/uaccess.h	18 Mar 2005 13:17:43 -0000	1.20
+++ include/asm-parisc/uaccess.h	22 Mar 2005 01:55:28 -0000
@@ -80,7 +80,20 @@ struct exception_data {
 	unsigned long fault_addr;
 };
 
-#define __get_user(x,ptr)                               \
+#ifdef __LP64__
+#define __get_user(x,ptr)						\
+  __builtin_choose_expr(						\
+    sizeof(__typeof__(*(ptr))) < 8					\
+    || __builtin_types_compatible_p(s64, __typeof__(*(ptr)))		\
+    || __builtin_types_compatible_p(u64, __typeof__(*(ptr)))		\
+    || __builtin_types_compatible_p(double, __typeof__(*(ptr))),	\
+    __get_user_compat(x, ptr),						\
+    __get_user_compat(x, (u32 *)(unsigned long)(ptr)))
+#else
+#define __get_user __get_user_compat
+#endif
+
+#define __get_user_compat(x,ptr)                        \
 ({                                                      \
 	register long __gu_err __asm__ ("r8") = 0;      \
 	register long __gu_val __asm__ ("r9") = 0;      \
@@ -104,7 +117,7 @@ struct exception_data {
 	    }                                           \
 	}                                               \
 							\
-	(x) = (__typeof__(*(ptr))) __gu_val;            \
+	(x) = (__typeof__(x))__gu_val;			\
 	__gu_err;                                       \
 })
 
@@ -146,26 +159,66 @@ struct exception_data {
 		: "r1");
 #endif /* !__LP64__ */
 
-#define __put_user(x,ptr)                                       \
+#ifdef __LP64__
+#define __put_user(x,ptr)						\
+  __builtin_choose_expr(						\
+    sizeof(__typeof__(*(ptr))) != 8					\
+    || __builtin_types_compatible_p(s64, __typeof__(*(ptr)))		\
+    || __builtin_types_compatible_p(u64, __typeof__(*(ptr)))		\
+    || __builtin_types_compatible_p(double, __typeof__(*(ptr))),	\
+    __put_user_compat((u64)(x), ptr),					\
+    __put_user_compat((u64)(x), (u32 *)(unsigned long)(ptr)))
+#else
+/* It is necessary to cast X to a suitable type for __put_user_compat.
+   64-bit values need truncation when PTR points to a smaller object.
+   It is also difficult to handle pointer types without generating
+   warnings from the u64 cast in the code for size 8.  The (void)0
+   choice generates a compile error if an unhandled type combination
+   is encountered.  */
+#define __put_user_cast(x,ptr)						\
+  __builtin_choose_expr(						\
+    (sizeof(__typeof__(x)) == 1						\
+     || sizeof(__typeof__(x)) == 2					\
+     || sizeof(__typeof__(x)) == 4					\
+     || sizeof(__typeof__(x)) == 8)					\
+    && (sizeof(__typeof__(*(ptr))) == 1					\
+	|| sizeof(__typeof__(*(ptr))) == 2				\
+	|| sizeof(__typeof__(*(ptr))) == 4),				\
+    (u32)(x),								\
+    __builtin_choose_expr(						\
+      (sizeof(__typeof__(x)) == 1					\
+       || sizeof(__typeof__(x)) == 2					\
+       || sizeof(__typeof__(x)) == 4)					\
+      && sizeof(__typeof__(*(ptr))) == 8,				\
+      (u64)(u32)(x),							\
+      __builtin_choose_expr(						\
+	sizeof(__typeof__(x)) == 8					\
+	&& sizeof(__typeof__(*(ptr))) == 8,				\
+	(x),								\
+	(void)0)))
+
+#define __put_user(x,ptr) __put_user_compat(__put_user_cast(x, ptr), ptr)
+#endif
+
+#define __put_user_compat(x,ptr)                                \
 ({								\
 	register long __pu_err __asm__ ("r8") = 0;      	\
-        __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x);	\
 								\
 	if (segment_eq(get_fs(),KERNEL_DS)) {                   \
 	    switch (sizeof(*(ptr))) {                           \
-	    case 1: __put_kernel_asm("stb",__x,ptr); break;     \
-	    case 2: __put_kernel_asm("sth",__x,ptr); break;     \
-	    case 4: __put_kernel_asm("stw",__x,ptr); break;     \
-	    case 8: STD_KERNEL(__x,ptr); break;			\
+	    case 1: __put_kernel_asm("stb",x,ptr); break;       \
+	    case 2: __put_kernel_asm("sth",x,ptr); break;       \
+	    case 4: __put_kernel_asm("stw",x,ptr); break;       \
+	    case 8: STD_KERNEL(x,ptr); break;			\
 	    default: __put_kernel_bad(); break;			\
 	    }                                                   \
 	}                                                       \
 	else {                                                  \
 	    switch (sizeof(*(ptr))) {                           \
-	    case 1: __put_user_asm("stb",__x,ptr); break;       \
-	    case 2: __put_user_asm("sth",__x,ptr); break;       \
-	    case 4: __put_user_asm("stw",__x,ptr); break;       \
-	    case 8: STD_USER(__x,ptr); break;			\
+	    case 1: __put_user_asm("stb",x,ptr); break;         \
+	    case 2: __put_user_asm("sth",x,ptr); break;         \
+	    case 4: __put_user_asm("stw",x,ptr); break;         \
+	    case 8: STD_USER(x,ptr); break;			\
 	    default: __put_user_bad(); break;			\
 	    }                                                   \
 	}                                                       \
@@ -219,10 +272,10 @@ struct exception_data {
 		: "r"(ptr), "r"(x), "0"(__pu_err)	    \
 		: "r1")
 
-#define __put_kernel_asm64(__val,ptr) do {		    	    \
-	u64 __val64 = (u64)(__val);				    \
-	u32 hi = (__val64) >> 32;					    \
-	u32 lo = (__val64) & 0xffffffff;				    \
+#define __put_kernel_asm64(__val,ptr) do {	    	    \
+	u64 __val64 = (u64)__val;			    \
+	u32 hi = (__val64) >> 32;			    \
+	u32 lo = (__val64) & 0xffffffff;		    \
 	__asm__ __volatile__ (				    \
 		"\n1:\tstw %2,0(%1)\n"			    \
 		"\n2:\tstw %3,4(%1)\n"			    \
@@ -235,10 +288,10 @@ struct exception_data {
 		: "r1");				    \
 } while (0)
 
-#define __put_user_asm64(__val,ptr) do {		    	    \
-	u64 __val64 = (u64)__val;				    \
-	u32 hi = (__val64) >> 32;					    \
-	u32 lo = (__val64) & 0xffffffff;				    \
+#define __put_user_asm64(__val,ptr) do {	    	    \
+	u64 __val64 = (u64)__val;			    \
+	u32 hi = (__val64) >> 32;			    \
+	u32 lo = (__val64) & 0xffffffff;		    \
 	__asm__ __volatile__ (				    \
 		"\n1:\tstw %2,0(%%sr3,%1)\n"		    \
 		"\n2:\tstw %3,4(%%sr3,%1)\n"		    \
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Re: Comments?

[Matthew Wilcox]

On Mon, Mar 21, 2005 at 09:25:13PM -0500, John David Anglin wrote:
> I got a bit paranoid about __put_user after the above bug.  Looking at
> it, I believe that the LP64 version is broken for *(ptr) types of long,
> unsigned long, and pointer types.  I believe that the current code
> puts a 64-bit value instead of the 32-bit value that the 32-bit runtime
> expects for these types.  This is fixed by adding a layer over the
> old __put_user macro to do various type checking.

Hum.  It's definitely a tricky problem.  On its face, you're right;
we shouldn't be writing 64-bit values to a 32-bit userspace.  However,
the compatibility layers often do something like:

asmlinkage int sys32_sendfile64(int out_fd, int in_fd, compat_loff_t __user *off
set, s32 count)
{
        mm_segment_t old_fs = get_fs();
        int ret;
        loff_t lof;
        if (offset && get_user(lof, offset))
                return -EFAULT;
        set_fs(KERNEL_DS);
        ret = sys_sendfile64(out_fd, in_fd, offset ? (loff_t __user *)&lof : NUL
L, count);
        set_fs(old_fs);
        if (offset && put_user(lof, offset))
                return -EFAULT;
        return ret;
}

Then sys_sendfile64() will do a put_user() to write to the loff_t variable,
and writing a 32-bit value to that would definitely be the wrong thing to do.

> The rest of the changes are just warning fixes that I noticed during
> testing.  I don't believe that they have a functional effect.

Those warnings are reminders that nobody's audited this code for ioremap
compatibility:

/*
 * Memory mapped I/O
 *
 * readX()/writeX() do byteswapping and take an ioremapped address
 * __raw_readX()/__raw_writeX() don't byteswap and take an ioremapped address.
 * gsc_*() don't byteswap and operate on physical addresses;
 *   eg dev->hpa or 0xfee00000.
 */

Maybe I need more verbiage in here for people who're tempted to fix these
warnings with more casts, but I doubt people would read it anyway ...

> I tested this with 32 and 64-bit builds of 2.6.12-rc1-pa1 with the
> default c3000 and 64 configs.  I am running the 32-bit build and a
> slighter earlier version survived a gcc build.  I haven't run the
> 64-bit changes.  I now don't see any cast related errors in the builds
> except one in a call to atomic_read.  That's an unrelated problem.

Someone else is fixing that -- in XFS or ieee1394, right?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Re: Comments?

[John David Anglin]

> > I got a bit paranoid about __put_user after the above bug.  Looking at
> > it, I believe that the LP64 version is broken for *(ptr) types of long,
> > unsigned long, and pointer types.  I believe that the current code
> > puts a 64-bit value instead of the 32-bit value that the 32-bit runtime
> > expects for these types.  This is fixed by adding a layer over the
> > old __put_user macro to do various type checking.
> 
> Hum.  It's definitely a tricky problem.  On its face, you're right;
> we shouldn't be writing 64-bit values to a 32-bit userspace.  However,
> the compatibility layers often do something like:
> 
> asmlinkage int sys32_sendfile64(int out_fd, int in_fd, compat_loff_t __user *off
> set, s32 count)
> {
>         mm_segment_t old_fs = get_fs();
>         int ret;
>         loff_t lof;
>         if (offset && get_user(lof, offset))
>                 return -EFAULT;
>         set_fs(KERNEL_DS);
>         ret = sys_sendfile64(out_fd, in_fd, offset ? (loff_t __user *)&lof : NUL
> L, count);
>         set_fs(old_fs);
>         if (offset && put_user(lof, offset))
>                 return -EFAULT;
>         return ret;
> }
> 
> Then sys_sendfile64() will do a put_user() to write to the loff_t variable,
> and writing a 32-bit value to that would definitely be the wrong thing to do.

I don't believe that the proposed change would do that.  In this case,
the type of *(ptr) is long long (s64).  This is one of the special cases
in the __LP64__ __builtin_choose_expr check.  If we have a 64-bit value
that is 64-bits in the 32-bit runtime (long long, unsigned long long
or double), we don't cast the pointer (i.e., it still points to a 64-bit
type).  I used s64 and u64 to shorten the line length in the macro.
They are defined as long long and unsigned long long.

__builtin_types_compatible_p is strict in determining whether two
types are compatible.  For example, a long is not compatible with a
long long even though they have the the same size in hppa64.  Similarly,
a char * and int * are not compatible.  I checked the long versus
long long compatibility under hpux.

There are definitely places where put_user is used to put pointers.
I believe that there also some places where a long is transferred.

The put_user uses with pointers cause warnings in 32-bit builds
because the u64 cast in the following code snippet

#define __put_kernel_asm64(__val,ptr) do {                  \
        u64 __val64 = (u64)__val;                           \

is undefined (it's not defined in the C standard how the pointer
should be extended).  As I tried to indicate in my previous mail,
this isn't a real problem as this code patch isn't used in this
case.  GCC should delete the code but there may be problems in doing
this before the warning is generated.  I like to eliminate warnings
such as this as they could easily be real problems.

Here is a place in arch/parisc/kernel/signal.c where there is a
problem with transferring a pointer:

          err |= __put_user(current->sas_ss_size, &frame->uc.uc_stack.ss_s

The type of ss_sp is void *.  There are also similar problems in
kernel/signal.c (si_ptr and si_addr).  In this case, the current version
of put_user will transfer 64-bits when using a hppa64 kernel.

> > The rest of the changes are just warning fixes that I noticed during
> > testing.  I don't believe that they have a functional effect.
> 
> Those warnings are reminders that nobody's audited this code for ioremap
> compatibility:

Ok :(

> Someone else is fixing that -- in XFS or ieee1394, right?

These are the warnings that I was referring to:

  CC [M]  fs/xfs/xfs_inode_item.o
fs/xfs/xfs_inode_item.c: In function `xfs_inode_item_pushbuf':
fs/xfs/xfs_inode_item.c:803: warning: passing arg 1 of `atomic_read' from incompatible pointer type
fs/xfs/xfs_inode_item.c:825: warning: passing arg 1 of `atomic_read' from incompatible pointer type

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

#include <stdio.h>
int
main ()
{
  printf ("%d\n", __builtin_types_compatible_p (long, long long));
  return 0;
}

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Re: Comments?

[John David Anglin]

> Here is a place in arch/parisc/kernel/signal.c where there is a
> problem with transferring a pointer:
> 
>           err |= __put_user(current->sas_ss_size, &frame->uc.uc_stack.ss_s
> 
> The type of ss_sp is void *.  There are also similar problems in
> kernel/signal.c (si_ptr and si_addr).  In this case, the current version
> of put_user will transfer 64-bits when using a hppa64 kernel.

I should have added that that the kernel should only transfer 32 bits in
this case.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> > > I think we have somehow broken __cffc.  I did a binutils build
> > > yesterday and various ld tests are now failing.  These are tests
> > > that compare function pointers.
> > 
> > I didn't feed anything new into debian-glibc. All my patches have always
> > strived to keep __cffc working, if I messup the function pointer
> > comparison I get a slew of glibc testsuite failures which I always
> > patchup by making sure __cffc works :)
> 
> I haven't touched fptr.c either, although there is a change in GCC
> 4.0 with respect to canonicalization.  I'm using the debian glibc,
> 2.3.2.ds1-20, on the system on which I noticed this problem.

Ok, ld is broken in the binutils CVS head.
__canonicalize_funcptr_for_compare doesn't load the correct address
for _GLOBAL_OFFSET_TABLE_ in shared libraries.  It loads the address
used by main.  As a result, it thinks the plabel for the function
has been resolved ;(

The testcase is:

foo.c:

extern void bar (void);
typedef void (*func_t)(void);
int
foo (func_t f)
{
  return f == bar;
}

bar.c:

void bar (void) {};
int
main ()
{
  if (!foo (bar))
    abort ();
  return 0;
}

gcc -g -fPIC -S foo.c
gcc -shared -fPIC -o foo.sl foo.o
gcc -c -g -w bar.c
gcc -o bar bar.o foo.sl

nm foo.sl
...
00010ab0 a _GLOBAL_OFFSET_TABLE_

nm bar
...
00020c54 d _GLOBAL_OFFSET_TABLE_

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> Ok, ld is broken in the binutils CVS head.
> __canonicalize_funcptr_for_compare doesn't load the correct address
> for _GLOBAL_OFFSET_TABLE_ in shared libraries.  It loads the address
> used by main.  As a result, it thinks the plabel for the function
> has been resolved ;(

Found it:

2004-11-02  Hans-Peter Nilsson  <hp@axis.com>

        * elflink.c (_bfd_elf_create_got_section): Hide _GLOBAL_OFFSET_TABLE_.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[Carlos O'Donell]

On Sat, Mar 05, 2005 at 07:22:27PM -0500, John David Anglin wrote:
> > Ok, ld is broken in the binutils CVS head.
> > __canonicalize_funcptr_for_compare doesn't load the correct address
> > for _GLOBAL_OFFSET_TABLE_ in shared libraries.  It loads the address
> > used by main.  As a result, it thinks the plabel for the function
> > has been resolved ;(
> 
> Found it:
> 
> 2004-11-02  Hans-Peter Nilsson  <hp@axis.com>
> 
>         * elflink.c (_bfd_elf_create_got_section): Hide _GLOBAL_OFFSET_TABLE_.

I'd seen that fly by on binutils and it caused a certain amount of grief
in glibc. I'm not quite sure what the rational was behind hiding _GOT_.

c.

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> > 2004-11-02  Hans-Peter Nilsson  <hp@axis.com>
> > 
> >         * elflink.c (_bfd_elf_create_got_section): Hide _GLOBAL_OFFSET_TABLE_.
> 
> I'd seen that fly by on binutils and it caused a certain amount of grief
> in glibc. I'm not quite sure what the rational was behind hiding _GOT_.

I believe that the idea was that main code would see its _GOT_ address
and shared libraries their respective _GOT_ address.  It turns out that
it was sort of a fluke that function pointer canonicalization worked.
Only function pointers in the main part of an application are not
resolved.  Shared library function pointers are resolved when the
library is loaded.  __cffc was always looking at the _GOT_ address
for the main app (i.e., the main _GLOBAL_OFFSET_TABLE_ value overrode
the shared library symbol).

This problem was fixed yesterday by Alan Modra.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[Carlos O'Donell]

On Tue, Mar 08, 2005 at 12:44:53PM -0500, John David Anglin wrote:
> > > 2004-11-02  Hans-Peter Nilsson  <hp@axis.com>
> > > 
> > >         * elflink.c (_bfd_elf_create_got_section): Hide _GLOBAL_OFFSET_TABLE_.
> > 
> > I'd seen that fly by on binutils and it caused a certain amount of grief
> > in glibc. I'm not quite sure what the rational was behind hiding _GOT_.
> 
> I believe that the idea was that main code would see its _GOT_ address
> and shared libraries their respective _GOT_ address.  It turns out that
> it was sort of a fluke that function pointer canonicalization worked.
> Only function pointers in the main part of an application are not
> resolved.  Shared library function pointers are resolved when the
> library is loaded.  __cffc was always looking at the _GOT_ address
> for the main app (i.e., the main _GLOBAL_OFFSET_TABLE_ value overrode
> the shared library symbol).
> 
> This problem was fixed yesterday by Alan Modra.

I'm not sure what you mean in your statement "shared library function
pointers are resolved when the library is loaded?" The function pointers
exist as two-byte entries in the PLT, and are non-unique, and they
aren't resolved until call time with lazy resolution.

I would imagine that the main _GOT_ is supposed to override the shared
library _GOT_. __cffc is only looking at the _GOT_ to get the fptr that
the dynamic loader wrote there during setup, so that it can get called
for symbol resolution. This setup is done for all objects, which
includes all the _GOT_'s, from the application to all the loaded shared
libraries.

Or rather are you saying, that from a shared library, the library saw
only the _GOT_ from the main applicaiton? I would think that this 
wouldn't effect the library since the loader setup r19 properly, and
aslong as it doesn't use _GOT_ then it's fine.

c.

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] Re: Comments?

[John David Anglin]

> > This problem was fixed yesterday by Alan Modra.
> 
> I'm not sure what you mean in your statement "shared library function
> pointers are resolved when the library is loaded?" The function pointers
> exist as two-byte entries in the PLT, and are non-unique, and they
> aren't resolved until call time with lazy resolution.

This is what Alasn said:

  I checked.  &_GLOBAL_OFFSET_TABLE_ in fptr.c does resolve to the main
  app GOT before the change.  That in fact is what is needed, because
  according to what I see in dl-machine.h, plabels in shared libs will be
  resolved.  I think it's only plabels in the main app for global
  functions that won't be resolved (because they point into the plt).

  Hmm.  If PLABEL32 relocs in the main app were emitted as dynamic relocs,
  then ld.so would call _dl_make_fptr for them.  I think you wouldn't need
  __canonicalize_funcptr_for_compare any more..

I don't think we could completely do away with __cffc but if the
plabels were always resolved we could just look inside the plabel
to get the function address.

> I would imagine that the main _GOT_ is supposed to override the shared
> library _GOT_. __cffc is only looking at the _GOT_ to get the fptr that
> the dynamic loader wrote there during setup, so that it can get called
> for symbol resolution. This setup is done for all objects, which
> includes all the _GOT_'s, from the application to all the loaded shared
> libraries.

If the dynamic loader always uses the main app's got for the trampoline,
__cffc would work for plabels in shared libraries that used lazy linking.

> Or rather are you saying, that from a shared library, the library saw
> only the _GOT_ from the main applicaiton? I would think that this 
> wouldn't effect the library since the loader setup r19 properly, and
> aslong as it doesn't use _GOT_ then it's fine.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux