glibc 2.0.5 is miscompiled by egcs 970904

glibc 2.0.5 is miscompiled by egcs 970904

[H.J. Lu]

Hi,

There is the preprocessed sysdeps/alpha/__longjmp.c in glibc 2.0.5.
egcs 970904 for linux/alpha miscompiled it with -O2. -O is fine.
gcc 2.7.2.1 has no problems with -O2. I know nothing about the alpha
asm. With the incorrect __longjmp.o, setjmp/tst-setjmp gave:

Saved environment.
Shouldn't have Jumped to 19.
Test FAILED!

I think the wrong information is saved.

Thanks.


-- 
H.J. Lu (hjl@gnu.ai.mit.edu)
---
register long int
  r9 asm ("$9"), r10 asm ("$10"), r11 asm ("$11"), r12 asm ("$12"),
  r13 asm ("$13"), r14 asm ("$14");
register long int *fp asm ("$15"), *sp asm ("$30"), *retpc asm ("$26");
register double
  f2 asm ("$f2"), f3 asm ("$f3"), f4 asm ("$f4"), f5 asm ("$f5"),
  f6 asm ("$f6"), f7 asm ("$f7"), f8 asm ("$f8"), f9 asm ("$f9");
typedef struct
  {
    long int __9, __10, __11, __12, __13, __14;
    long int *__pc, *__fp, *__sp;
    double __f2, __f3, __f4, __f5, __f6, __f7, __f8, __f9;
  } __jmp_buf[1];
typedef int __sig_atomic_t;
typedef struct
  {
    unsigned long int __val[(1024 / (8 * sizeof (unsigned long int))) ];
  } __sigset_t;
typedef struct __jmp_buf_tag	 
  {
    __jmp_buf __jmpbuf;		 
    int __mask_was_saved;	 
    __sigset_t __saved_mask;	 
  } jmp_buf[1];
extern int __sigsetjmp  (jmp_buf __env, int __savemask)  ;
extern void longjmp  (jmp_buf __env, int __val)  
     __attribute__ ((__noreturn__));
extern void _longjmp  (jmp_buf __env, int __val)  
     __attribute__ ((__noreturn__));
extern void __longjmp  (__jmp_buf __env, int __val)  
     __attribute__ ((__noreturn__));
extern int __sigjmp_save  (jmp_buf __env, int __savemask)  ;
typedef jmp_buf sigjmp_buf;
extern void siglongjmp  (sigjmp_buf __env, int __val)  
     __attribute__ ((__noreturn__));
void
__longjmp (__jmp_buf env, int val)
{
  register long int retval asm ("$0");
  r9 = env[0].__9;
  r10 = env[0].__10;
  r11 = env[0].__11;
  r12 = env[0].__12;
  r13 = env[0].__13;
  r14 = env[0].__14;
  f2 = env[0].__f2;
  f3 = env[0].__f3;
  f4 = env[0].__f4;
  f5 = env[0].__f5;
  f6 = env[0].__f6;
  f7 = env[0].__f7;
  f8 = env[0].__f8;
  f9 = env[0].__f9;
  retpc = env[0].__pc;
  fp = env[0].__fp;
  sp = env[0].__sp;
  asm volatile
    ("cmoveq %1, 1, %0\n\t"	 
     "ret $31, (%2), 1"	 
     : "=r" (retval)
     : "0" (val), "r" (retpc));
  while (1)
    {
      asm volatile ("");
    }
}

Re: glibc 2.0.5 is miscompiled by egcs 970904

[Richard Henderson]

>   register long int retval asm ("$0");
[...]
>   asm volatile
>     ("cmoveq %1, 1, %0\n\t"	 
>      "ret $31, (%2), 1"	 
>      : "=r" (retval)
>      : "0" (val), "r" (retpc));

Interesting.  -O yields

        bis $17,$17,$0
        cmoveq $0, 1, $0

as intended, while -O2 yields

        cmoveq $17, 1, $17

At first blush, the code is actually seems broken, but the more proper

	asm volatile("cmoveq %0, 1, %0\n\tret (%1)"
		     : "=r"(retval) : "r"(retpc), "0"(val));

is broken too, while

	asm volatile("cmoveq %1, 1, %0\n\tret (%2)"
		     : "=r"(retval) : "r"(val), "r"(retpc), "0"(val));

works.

Now it does seem to me that explicitly named register variables
should get presidence in an asm than those that aren't.

But as we have a short term solution, and the fact that I've never
been particularly happy with implementing longjmp in C at all, I'm
not terribly motivated to dive into reload to find this.


r~

Re: glibc 2.0.5 is miscompiled by egcs 970904

[Jim Wilson]

>   asm volatile
>     ("cmoveq %1, 1, %0\n\t"	 
>      "ret $31, (%2), 1"	 
>      : "=r" (retval)
>      : "0" (val), "r" (retpc));

I would call this a bug in the asm.  It is assuming that $0 (retval) will be
used for operand 0.  If this is not done, then the pattern fails because it
contains a return instruction.

However, it is not safe to make this assumption because the use of a matching
contraint for operand 1 means that a reload is necessary.  When a reload is
required, reload makes no guarantees about where the reload instruction will
be emitted.  It may emit an input reload before the asm, or it may emit an
output reload after the asm.  In this case, it is emitting an output reload
after the asm.  This instruction then gets optimized away because reload is
smart enough to know that there aren't any following instructions that
use its value.  This is the right thing to do in general, because it means
that we emit one less instruction, and gcc is after all an optimizing
compiler.

A much better way to write this is:
	retval = val;
	asm volatile ("cmoveq %1, 1, %0\n\t"
		     "ret $31, (%2), 1"	 
		     : "=r" (retval)
		     : "0" (retval), "r" (retpc));
Now, no reloads are necessary, and the pattern will always do what you
expect.  We even get better code this way, because now the move instruction
is explicit, we get the benefit of instruction scheduling.

An even better way to write this is:
  retval = val ? val : 1;
  asm volatile ("ret $31, (%0), 1" : : "r" (retpc), "r" (retval));
This just uses the asm for stuff that can't be written in C.

Jim