s_lock.h problem on S/Linux

Hi hackers.

I am having problems with the new spinlock code on the SPARC linux
platform. (Latest CVSup)

The compiler doesn't seem to like the "asm" part of s_lock.h for (sparc)

Here's one of the compiles that fails.

Thank you for testing and reporting this. It is my fault of course, but as
I don't have access to a sparc for testing I just did what I could. I am
guessing here, but please apply the following to your pgsql and let me know
what happens. Also, cd to src/storage/buffer and do 'make s_lock_test' as
well.

*** src/include/storage/s_lock.h.orig	Sun Jun 14 19:37:47 1998
--- src/include/storage/s_lock.h	Sat Jun 20 18:01:13 1998
***************
*** 130,136 ****

      __asm__("ldstub [%1], %0" \
              : "=r"(_res), "=m"(*lock) \
!             : "1"(lock));
  	return (int) _res;
  }
  #endif /* sparc */
--- 130,136 ----

__asm__("ldstub [%1], %0" \
: "=r"(_res), "=m"(*lock) \
! : "0"(_res));
return (int) _res;
}
#endif /* sparc */

-dg

David Gould dg@illustra.com 510.628.3783 or 510.305.9468
Informix Software (No, really) 300 Lakeside Drive Oakland, CA 94612
"Don't worry about people stealing your ideas. If your ideas are any
good, you'll have to ram them down people's throats." -- Howard Aiken

David,

After making your suggested changes and then doing a "make s_lock_test"
I get the following error messages.

[postgres@sparclinux buffer]$ make s_lock_test
gcc -g -I../../../include -I../../../backend -O2 -I../.. -DS_LOCK_TEST=1
s_lock.c -o s_lock_test
/tmp/cca10794.s: Assembler messages:
/tmp/cca10794.s:290: Error: Bad expression
/tmp/cca10794.s:290: Error: Missing ')' assumed
/tmp/cca10794.s:290: Error: Bad expression
/tmp/cca10794.s:290: Error: Missing ')' assumed
/tmp/cca10794.s:290: Error: Illegal operands
/tmp/cca10794.s:440: Error: Bad expression
/tmp/cca10794.s:440: Error: Missing ')' assumed
/tmp/cca10794.s:440: Error: Bad expression
/tmp/cca10794.s:440: Error: Missing ')' assumed
/tmp/cca10794.s:440: Error: Illegal operands
make: *** [s_lock_test] Error 1

If I compile with -save-temps and look at the s_lock.s file I see, on
line 290:-

.stabn 68,0,131,.LM9-s_lock
.LM9:
.LL45:
ldstub [[%i0]], %o0 <-------
.stabn 68,0,134,.LM10-s_lock
.LM10:
and %o0,0xff,%o0

The double square braces look strange to me so I removed the single
braces in s_lock.h.

The modified file compiles OK and in s_lock.s I can see:-

.stabn 68,0,131,.LM9-s_lock
.LM9:
.LL45:
ldstub [%i0], %o0
.stabn 68,0,134,.LM10-s_lock
.LM10:
and %o0,0xff,%o0

Now I know absoloutely nothing whatsoever about SPARC (or gnu)
assembler so the code changes could result in nothing like a
test and set function but...

[postgres@sparclinux buffer]$ make s_lock_test
gcc -g -I../../../include -I../../../backend -O2 -I../.. -DS_LOCK_TEST=1
s_lock.c -o s_lock_test
./s_lock_test
S_LOCK_TEST: this will hang for a few minutes and then abort
with a 'stuck spinlock' message if S_LOCK()
and TAS() are working.

FATAL: s_lock(00020be8) at s_lock.c:215, stuck spinlock. Aborting.

FATAL: s_lock(00020be8) at s_lock.c:215, stuck spinlock. Aborting.
make: *** [s_lock_test] IOT trap/Abort (core dumped)
make: *** Deleting file `s_lock_test'

Running s_lock_test does result in a hang for a few minutes
and then a "stuck spinlock" message so perhaps it's not all
that bad. (Not sure about the core dump though :-( )

Keith.

dg@illustra.com (David Gould)

David,

After making your suggested changes and then doing a "make s_lock_test"
I get the following error messages.

[postgres@sparclinux buffer]$ make s_lock_test
gcc -g -I../../../include -I../../../backend -O2 -I../.. -DS_LOCK_TEST=1
s_lock.c -o s_lock_test
/tmp/cca10794.s: Assembler messages:
/tmp/cca10794.s:290: Error: Bad expression
/tmp/cca10794.s:290: Error: Missing ')' assumed
/tmp/cca10794.s:290: Error: Bad expression
/tmp/cca10794.s:290: Error: Missing ')' assumed
/tmp/cca10794.s:290: Error: Illegal operands
/tmp/cca10794.s:440: Error: Bad expression
/tmp/cca10794.s:440: Error: Missing ')' assumed
/tmp/cca10794.s:440: Error: Bad expression
/tmp/cca10794.s:440: Error: Missing ')' assumed
/tmp/cca10794.s:440: Error: Illegal operands
make: *** [s_lock_test] Error 1

If I compile with -save-temps and look at the s_lock.s file I see, on
line 290:-

.stabn 68,0,131,.LM9-s_lock
.LM9:
.LL45:
ldstub [[%i0]], %o0 <-------
.stabn 68,0,134,.LM10-s_lock
.LM10:
and %o0,0xff,%o0

The double square braces look strange to me so I removed the single
braces in s_lock.h.

The modified file compiles OK and in s_lock.s I can see:-

.stabn 68,0,131,.LM9-s_lock
.LM9:
.LL45:
ldstub [%i0], %o0
.stabn 68,0,134,.LM10-s_lock
.LM10:
and %o0,0xff,%o0

Now I know absoloutely nothing whatsoever about SPARC (or gnu)
assembler so the code changes could result in nothing like a
test and set function but...

[postgres@sparclinux buffer]$ make s_lock_test
gcc -g -I../../../include -I../../../backend -O2 -I../.. -DS_LOCK_TEST=1
s_lock.c -o s_lock_test
./s_lock_test
S_LOCK_TEST: this will hang for a few minutes and then abort
with a 'stuck spinlock' message if S_LOCK()
and TAS() are working.

FATAL: s_lock(00020be8) at s_lock.c:215, stuck spinlock. Aborting.

FATAL: s_lock(00020be8) at s_lock.c:215, stuck spinlock. Aborting.
make: *** [s_lock_test] IOT trap/Abort (core dumped)
make: *** Deleting file `s_lock_test'

Running s_lock_test does result in a hang for a few minutes
and then a "stuck spinlock" message so perhaps it's not all
that bad. (Not sure about the core dump though :-( )

Keith.

The core dump is expected. When a stuck spinlock is detected abort() is called
which makes the core dump. So it looks like it is working.

If you would, please send me the s_lock.s file and a diff for you changes
I will look at the generated code and if it looks ok will submit the patch.

Thanks,

-dg

David Gould dg@illustra.com 510.628.3783 or 510.305.9468
Informix Software (No, really) 300 Lakeside Drive Oakland, CA 94612
"Don't worry about people stealing your ideas. If your ideas are any
good, you'll have to ram them down people's throats." -- Howard Aiken

Patch applied.

Hi hackers.

I am having problems with the new spinlock code on the SPARC linux
platform. (Latest CVSup)

The compiler doesn't seem to like the "asm" part of s_lock.h for (sparc)

Here's one of the compiles that fails.

Thank you for testing and reporting this. It is my fault of course, but as
I don't have access to a sparc for testing I just did what I could. I am
guessing here, but please apply the following to your pgsql and let me know
what happens. Also, cd to src/storage/buffer and do 'make s_lock_test' as
well.

*** src/include/storage/s_lock.h.orig	Sun Jun 14 19:37:47 1998
--- src/include/storage/s_lock.h	Sat Jun 20 18:01:13 1998
***************
*** 130,136 ****

__asm__("ldstub [%1], %0" \
: "=r"(_res), "=m"(*lock) \
!             : "1"(lock));
return (int) _res;
}
#endif /* sparc */
--- 130,136 ----

__asm__("ldstub [%1], %0" \
: "=r"(_res), "=m"(*lock) \
! : "0"(_res));
return (int) _res;
}
#endif /* sparc */

-dg

David Gould dg@illustra.com 510.628.3783 or 510.305.9468
Informix Software (No, really) 300 Lakeside Drive Oakland, CA 94612
"Don't worry about people stealing your ideas. If your ideas are any
good, you'll have to ram them down people's throats." -- Howard Aiken

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

I haven't seen any followups to this, but I finally got around to
compiling the system again myself, and David's fix is not quite right.
He says to apply this:

*** src/include/storage/s_lock.h.orig	Sun Jun 14 19:37:47 1998
--- src/include/storage/s_lock.h	Sat Jun 20 18:01:13 1998
***************
*** 130,136 ****

__asm__("ldstub [%1], %0" \
: "=r"(_res), "=m"(*lock) \
!             : "1"(lock));
return (int) _res;
}
#endif /* sparc */
--- 130,136 ----

__asm__("ldstub [%1], %0" \
: "=r"(_res), "=m"(*lock) \
! : "0"(_res));
return (int) _res;
}
#endif /* sparc */

However, the reference to the lock pointer as "1" was closer to being
correct that then "0" is! The trouble is that the compiler doesn't
like the mixed indirection in the references for the lock pointer with
the "1" there in the original. Changing the input parameter as shown
to indicate _res fixes this, but is wrong, since that's not the input.
In the current sources, the "1" has been changed to a "0", erroneously
calling _res an input, but the name of the variable to use is still
'lock', making it really confusing by fetching the right input (the
pointer), and stuffing it into the wrong register -- and causing the
assembler to join in the chorus of complaints when it sees the double
dereferencing brackets in its source... :-)

Much better is to actually specify the constraints individually, and
then simply refer to the input parameter in the instruction. Here's
the patch I have to apply to the current sources to get it to compile
and work right (I've tested it with s_lock_test, of course):

Index: s_lock.h
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/include/storage/s_lock.h,v
retrieving revision 1.39
diff -r1.39 s_lock.h
131,133c131,133
<     __asm__("ldstub [%1], %0" \
<             : "=r"(_res), "=m"(*lock) \
<             : "0"(lock));
---

__asm__("ldstub [%2], %0"
: "=r" (_res), "=m" (*lock)
: "r" (lock));

Oh, yeah, I guess I didn't have to remove the backslashes, but this is
the GCC section, and they're not needed.

-tih
--
Popularity is the hallmark of mediocrity. --Niles Crane, "Frasier"

Patch applied.

I haven't seen any followups to this, but I finally got around to
compiling the system again myself, and David's fix is not quite right.
He says to apply this:

*** src/include/storage/s_lock.h.orig	Sun Jun 14 19:37:47 1998
--- src/include/storage/s_lock.h	Sat Jun 20 18:01:13 1998
***************
*** 130,136 ****

__asm__("ldstub [%1], %0" \
: "=r"(_res), "=m"(*lock) \
!             : "1"(lock));
return (int) _res;
}
#endif /* sparc */
--- 130,136 ----

__asm__("ldstub [%1], %0" \
: "=r"(_res), "=m"(*lock) \
! : "0"(_res));
return (int) _res;
}
#endif /* sparc */

However, the reference to the lock pointer as "1" was closer to being
correct that then "0" is! The trouble is that the compiler doesn't
like the mixed indirection in the references for the lock pointer with
the "1" there in the original. Changing the input parameter as shown
to indicate _res fixes this, but is wrong, since that's not the input.
In the current sources, the "1" has been changed to a "0", erroneously
calling _res an input, but the name of the variable to use is still
'lock', making it really confusing by fetching the right input (the
pointer), and stuffing it into the wrong register -- and causing the
assembler to join in the chorus of complaints when it sees the double
dereferencing brackets in its source... :-)

Much better is to actually specify the constraints individually, and
then simply refer to the input parameter in the instruction. Here's
the patch I have to apply to the current sources to get it to compile
and work right (I've tested it with s_lock_test, of course):

Index: s_lock.h
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/include/storage/s_lock.h,v
retrieving revision 1.39
diff -r1.39 s_lock.h
131,133c131,133
<     __asm__("ldstub [%1], %0" \
<             : "=r"(_res), "=m"(*lock) \
<             : "0"(lock));
---

__asm__("ldstub [%2], %0"
: "=r" (_res), "=m" (*lock)
: "r" (lock));

Oh, yeah, I guess I didn't have to remove the backslashes, but this is
the GCC section, and they're not needed.

-tih
--
Popularity is the hallmark of mediocrity. --Niles Crane, "Frasier"

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

Hi,

I grabbed the latest from CVS and everything compiles fine and works
well.

Regression only fails int8, float8, geometry and union (union all clauses)

I'll be having another play later with my test DB.

Keith.

Bruce Momjian <maillist@candle.pha.pa.us>