Re: fs/binfmt_aout.o, Error: suffix or operands invalid for `cmp' [was Re: 2.6.1

Re: fs/binfmt_aout.o, Error: suffix or operands invalid for `cmp' [was Re: 2.6.1

[Chuck Ebbert]

> Andrew Morton wrote:
In-Reply-To: <4499BDB4.6060503@zytor.com>

On Wed, 21 Jun 2006 14:44:20 -0700, H. Peter Anvin wrote:

> Andrew Morton wrote:
> > On Wed, 21 Jun 2006 23:16:17 +0200
> > Mattia Dongili <malattia@linux.it> wrote:
> > 
> >> On Wed, Jun 21, 2006 at 01:42:15PM -0700, Andrew Morton wrote:
> >>> On Wed, 21 Jun 2006 21:39:32 +0200
> >>> Mattia Dongili <malattia@linux.it> wrote:
> >>>
> >>>> Thanks, this is fixed, but I have a new failure:
> >>>>   CC [M]  fs/xfs/support/move.o
> >>>>   CC [M]  fs/xfs/support/uuid.o
> >>>>   LD [M]  fs/xfs/xfs.o
> >>>>   CC      fs/dnotify.o
> >>>>   CC      fs/dcookies.o
> >>>>   LD      fs/built-in.o
> >>>>   CC [M]  fs/binfmt_aout.o
> >>>> {standard input}: Assembler messages:
> >>>> {standard input}:160: Error: suffix or operands invalid for `cmp'
> >>>> make[1]: *** [fs/binfmt_aout.o] Error 1
> >>>> make: *** [fs] Error 2
> >>> what the heck?  Can you do `make fs/binfmt_aout.s' then send the relevant
> >>> parts of that file?
> >> I can't really tell which is the relevant part other than line 160 :)
> >> Full file available here:
> >> http://oioio.altervista.org/linux/binfmt_aout.s
> >>
> > 
> > It's complaining about this:
> > 
> > #APP
> >         addl %ecx,%eax ; sbbl %edx,%edx; cmpl %eax,$-1073741824; sbbl $0,%edx   # dump.u_dsize, sum, flag,
> > #NO_APP
> 
> The cmpl should have its arguments reversed.  It's quite possible some versions of the 
> assembler accepts the form given, but they're wrong (and doubly confusing when used as 
> input to sbb.)

This was built with gcc 4.0.4 20060507 (prerelease).

I don't normally build a.out support, but I just tried and it compiled
fine with gcc 4.1.1.  SO this is probably a compiler bug (almost certainly
given that it generated illegal assembler code.)

-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

Re: fs/binfmt_aout.o, Error: suffix or operands invalid for `cmp' [was Re: 2.6.1

[H. Peter Anvin]

Chuck Ebbert wrote:
>>>>
>>> It's complaining about this:
>>>
>>> #APP
>>>         addl %ecx,%eax ; sbbl %edx,%edx; cmpl %eax,$-1073741824; sbbl $0,%edx   # dump.u_dsize, sum, flag,
>>> #NO_APP
>> The cmpl should have its arguments reversed.  It's quite possible some versions of the 
>> assembler accepts the form given, but they're wrong (and doubly confusing when used as 
>> input to sbb.)
> 
> This was built with gcc 4.0.4 20060507 (prerelease).
> 
> I don't normally build a.out support, but I just tried and it compiled
> fine with gcc 4.1.1.  SO this is probably a compiler bug (almost certainly
> given that it generated illegal assembler code.)
> 

It's not (it's #APP, i.e. inline assembly); rather, it's an illegal 
constraint.

	-hpa

Re: fs/binfmt_aout.o, Error: suffix or operands invalid for `cmp' [was Re: 2.6.1

[Steven Rostedt]

I have no idea why I have binfmt_aout turned on but I did and hit this
too.

On Thu, 22 Jun 2006, H. Peter Anvin wrote:

> Chuck Ebbert wrote:
> >>>>
> >>> It's complaining about this:
> >>>
> >>> #APP
> >>>         addl %ecx,%eax ; sbbl %edx,%edx; cmpl %eax,$-1073741824; sbbl $0,%edx   # dump.u_dsize, sum, flag,
> >>> #NO_APP
> >> The cmpl should have its arguments reversed.  It's quite possible some versions of the
> >> assembler accepts the form given, but they're wrong (and doubly confusing when used as
> >> input to sbb.)
> >
> > This was built with gcc 4.0.4 20060507 (prerelease).
> >
> > I don't normally build a.out support, but I just tried and it compiled
> > fine with gcc 4.1.1.  SO this is probably a compiler bug (almost certainly
> > given that it generated illegal assembler code.)
> >
>
> It's not (it's #APP, i.e. inline assembly); rather, it's an illegal
> constraint.
>

It's GCC optimizing a little too much.

The problem code is this (in binfmt_aout.c):

/* make sure we actually have a data and stack area to dump */
	set_fs(USER_DS);
#ifdef __sparc__
	if (!access_ok(VERIFY_READ, (void __user *)START_DATA(dump), dump.u_dsize))
		dump.u_dsize = 0;
	if (!access_ok(VERIFY_READ, (void __user *)START_STACK(dump), dump.u_ssize))
		dump.u_ssize = 0;
#else
	if (!access_ok(VERIFY_READ, (void __user *)START_DATA(dump), dump.u_dsize << PAGE_SHIFT))
		dump.u_dsize = 0;
	if (!access_ok(VERIFY_READ, (void __user *)START_STACK(dump), dump.u_ssize << PAGE_SHIFT))
		dump.u_ssize = 0;
#endif


Previously it did a set_fs(KERNEL_DS) so it needs to do the
set_fs(USER_DS) now.

But that is:

  #define set_fs(x)	(current_thread_info()->addr_limit = (x))

and access_ok is:

  #define access_ok(type,addr,size) (likely(__range_ok(addr,size) == 0))

where __range_ok is:

#define __range_ok(addr,size) ({ \
	unsigned long flag,sum; \
	__chk_user_ptr(addr); \
	asm("addl %3,%1 ; sbbl %0,%0; cmpl %1,%4; sbbl $0,%0" \
		:"=&r" (flag), "=r" (sum) \
		:"1" (addr),"g" ((int)(size)),"g" (current_thread_info()->addr_limit.seg)); \
	flag; })

What happened was that gcc optimized the
(current_thread_info()->addre_limit.seg to be a constant. Thus cmpl
failed.

The following patch fixes this, although I don't know the intel
constraints very well, but this does compile.

-- Steve

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Index: linux-2.6.17-mm1/include/asm-i386/uaccess.h
===================================================================
--- linux-2.6.17-mm1.orig/include/asm-i386/uaccess.h	2006-06-23 05:02:15.000000000 -0400
+++ linux-2.6.17-mm1/include/asm-i386/uaccess.h	2006-06-23 05:02:29.000000000 -0400
@@ -58,7 +58,7 @@ extern struct movsl_mask {
 	__chk_user_ptr(addr); \
 	asm("addl %3,%1 ; sbbl %0,%0; cmpl %1,%4; sbbl $0,%0" \
 		:"=&r" (flag), "=r" (sum) \
-		:"1" (addr),"g" ((int)(size)),"g" (current_thread_info()->addr_limit.seg)); \
+		:"1" (addr),"g" ((int)(size)),"r" (current_thread_info()->addr_limit.seg)); \
 	flag; })

 /**

Re: fs/binfmt_aout.o, Error: suffix or operands invalid for `cmp' [was Re: 2.6.1

[H. Peter Anvin]

Steven Rostedt wrote:
>>>
>> It's not (it's #APP, i.e. inline assembly); rather, it's an illegal
>> constraint.
> 
> It's GCC optimizing a little too much.
> 

No, it's not... it's the author of the inline assembly who told gcc a 
lie at what it was allowed to optimize.  The constraint is "g" 
(equivalent to "rmi"), but "rm" is the correct constraint.

There is already a patch in Andrew's repo for this.

 >
> #define __range_ok(addr,size) ({ \
> 	unsigned long flag,sum; \
> 	__chk_user_ptr(addr); \
> 	asm("addl %3,%1 ; sbbl %0,%0; cmpl %1,%4; sbbl $0,%0" \
> 		:"=&r" (flag), "=r" (sum) \
> 		:"1" (addr),"g" ((int)(size)),"g" (current_thread_info()->addr_limit.seg)); \
> 	flag; })
> 
> What happened was that gcc optimized the
> (current_thread_info()->addre_limit.seg to be a constant. Thus cmpl
> failed.
> 

... perfectly legally so, given the "g" constraint.

	-hpa

Re: fs/binfmt_aout.o, Error: suffix or operands invalid for `cmp' [was Re: 2.6.1

[Steven Rostedt]

On Fri, 23 Jun 2006, H. Peter Anvin wrote:

> Steven Rostedt wrote:
> >>>
> >> It's not (it's #APP, i.e. inline assembly); rather, it's an illegal
> >> constraint.
> >
> > It's GCC optimizing a little too much.
> >
>
> No, it's not...

Yes it is! Calm down, I didn't say it was GCC's fault!

> it's the author of the inline assembly who told gcc a
> lie at what it was allowed to optimize.  The constraint is "g"
> (equivalent to "rmi"), but "rm" is the correct constraint.

And this is why it over optimized.  It was the fault of the inline
assembly author.  He/she gave it the wrong constraint and this was
the reason for GCC over optimizing.  If it didn't over optimize than
it would have worked.  But the bug is with the code and not GCC.

>
> There is already a patch in Andrew's repo for this.

Then this is all OK.

>
>  >
> > #define __range_ok(addr,size) ({ \
> > 	unsigned long flag,sum; \
> > 	__chk_user_ptr(addr); \
> > 	asm("addl %3,%1 ; sbbl %0,%0; cmpl %1,%4; sbbl $0,%0" \
> > 		:"=&r" (flag), "=r" (sum) \
> > 		:"1" (addr),"g" ((int)(size)),"g" (current_thread_info()->addr_limit.seg)); \
> > 	flag; })
> >
> > What happened was that gcc optimized the
> > (current_thread_info()->addre_limit.seg to be a constant. Thus cmpl
> > failed.
> >
>
> ... perfectly legally so, given the "g" constraint.
>

I never said it wasn't legal.

-- Steve