s_lock.h cleanup

Started by Bruce Momjianalmost 25 years ago15 messages
#1Bruce Momjian
pgman@candle.pha.pa.us
1 attachment(s)

In looking at the VAX ASM problem, I realized that the ASM in s_lock.h
is all formatted differently, making it even more confusing. I have
applied the following patch to s_lock.h to try and clean it up.

The new standard format is:

/*
* Standard __asm__ format:
*
* __asm__(
* "command;"
* "command;"
* "command;"
* : "=r"(_res) return value, in register
* : "r"(lock) argument, 'lock pointer', in register
* : "r0"); inline code uses this register
*/

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Attachments:

/bjm/difftext/plainDownload
? config.log
? config.cache
? config.status
? GNUmakefile
? src/Makefile.custom
? src/GNUmakefile
? src/Makefile.global
? src/log
? src/crtags
? src/backend/postgres
? src/backend/catalog/global.bki
? src/backend/catalog/global.description
? src/backend/catalog/template1.bki
? src/backend/catalog/template1.description
? src/backend/port/Makefile
? src/bin/initdb/initdb
? src/bin/initlocation/initlocation
? src/bin/ipcclean/ipcclean
? src/bin/pg_config/pg_config
? src/bin/pg_ctl/pg_ctl
? src/bin/pg_dump/pg_dump
? src/bin/pg_dump/pg_restore
? src/bin/pg_dump/pg_dumpall
? src/bin/pg_id/pg_id
? src/bin/pg_passwd/pg_passwd
? src/bin/pgaccess/pgaccess
? src/bin/pgtclsh/Makefile.tkdefs
? src/bin/pgtclsh/Makefile.tcldefs
? src/bin/pgtclsh/pgtclsh
? src/bin/pgtclsh/pgtksh
? src/bin/psql/psql
? src/bin/scripts/createlang
? src/include/config.h
? src/include/stamp-h
? src/interfaces/ecpg/lib/libecpg.so.3.2.0
? src/interfaces/ecpg/preproc/ecpg
? src/interfaces/libpgeasy/libpgeasy.so.2.1
? src/interfaces/libpgtcl/libpgtcl.so.2.1
? src/interfaces/libpq/libpq.so.2.1
? src/interfaces/perl5/blib
? src/interfaces/perl5/Makefile
? src/interfaces/perl5/pm_to_blib
? src/interfaces/perl5/Pg.c
? src/interfaces/perl5/Pg.bs
? src/pl/plperl/blib
? src/pl/plperl/Makefile
? src/pl/plperl/pm_to_blib
? src/pl/plperl/SPI.c
? src/pl/plperl/plperl.bs
? src/pl/plpgsql/src/libplpgsql.so.1.0
? src/pl/tcl/Makefile.tcldefs
Index: src/include/storage/s_lock.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/storage/s_lock.h,v
retrieving revision 1.78
diff -c -r1.78 s_lock.h
*** src/include/storage/s_lock.h	2001/01/18 23:40:26	1.78
--- src/include/storage/s_lock.h	2001/01/19 02:52:49
***************
*** 35,41 ****
   *
   *	int TAS(slock_t *lock)
   *		Atomic test-and-set instruction.  Attempt to acquire the lock,
!  *		but do *not* wait.  Returns 0 if successful, nonzero if unable
   *		to acquire the lock.
   *
   *	TAS() is a lower-level part of the API, but is used directly in a
--- 35,41 ----
   *
   *	int TAS(slock_t *lock)
   *		Atomic test-and-set instruction.  Attempt to acquire the lock,
!  *		but do *not* wait.	Returns 0 if successful, nonzero if unable
   *		to acquire the lock.
   *
   *	TAS() is a lower-level part of the API, but is used directly in a
***************
*** 48,56 ****
   *		unsigned	spins = 0;
   *
   *		while (TAS(lock))
-  *		{
   *			S_LOCK_SLEEP(lock, spins++);
-  *		}
   *	}
   *
   *	where S_LOCK_SLEEP() checks for timeout and sleeps for a short
--- 48,54 ----
***************
*** 87,96 ****
  
  /* Platform-independent out-of-line support routines */
  extern void s_lock(volatile slock_t *lock,
! 				   const char *file, const int line);
  extern void s_lock_sleep(unsigned spins, int microsec,
! 						 volatile slock_t *lock,
! 						 const char *file, const int line);
  
  
  #if defined(HAS_TEST_AND_SET)
--- 85,94 ----
  
  /* Platform-independent out-of-line support routines */
  extern void s_lock(volatile slock_t *lock,
! 	   const char *file, const int line);
  extern void s_lock_sleep(unsigned spins, int microsec,
! 			 volatile slock_t *lock,
! 			 const char *file, const int line);
  
  
  #if defined(HAS_TEST_AND_SET)
***************
*** 101,106 ****
--- 99,116 ----
   * All the gcc inlines
   */
  
+ /*
+  * Standard __asm__ format:
+  *
+  *	__asm__(
+  *			"command;"
+  *			"command;"
+  *			"command;"
+  *		:	"=r"(_res)			return value, in register
+  *		:	"r"(lock)			argument, 'lock pointer', in register
+  *		:	"r0");				inline code uses this register
+  */
+ 
  
  #if defined(__i386__)
  #define TAS(lock) tas(lock)
***************
*** 110,116 ****
  {
  	register slock_t _res = 1;
  
! __asm__("lock; xchgb %0,%1": "=q"(_res), "=m"(*lock):"0"(_res));
  	return (int) _res;
  }
  
--- 120,130 ----
  {
  	register slock_t _res = 1;
  
! 	__asm__(
! 			"lock;"
! 			"xchgb %0,%1;"
! :			"=q"(_res), "=m"(*lock)
! :			"0"(_res));
  	return (int) _res;
  }
  
***************
*** 121,139 ****
  #define TAS(lock) tas(lock)
  
  static __inline__ int
! tas (volatile slock_t *lock)
  {
!   long int ret;
  
!   __asm__ __volatile__(
!        "xchg4 %0=%1,%2"
!        : "=r"(ret), "=m"(*lock)
!        : "r"(1), "1"(*lock)
!        : "memory");
  
!   return (int) ret;
  }
! #endif /* __ia64__ */
  
  
  #if defined(__arm__) || defined(__arm__)
--- 135,154 ----
  #define TAS(lock) tas(lock)
  
  static __inline__ int
! tas(volatile slock_t *lock)
  {
! 	long int	ret;
  
! 	__asm__		__volatile__(
! 										 "xchg4 %0=%1,%2;"
! 							 :			 "=r"(ret), "=m"(*lock)
! 							 :			 "r"(1), "1"(*lock)
! 							 :			 "memory");
  
! 	return (int) ret;
  }
! 
! #endif	 /* __ia64__ */
  
  
  #if defined(__arm__) || defined(__arm__)
***************
*** 142,180 ****
  static __inline__ int
  tas(volatile slock_t *lock)
  {
!         register slock_t _res = 1;
  
! __asm__("swpb %0, %0, [%3]": "=r"(_res), "=m"(*lock):"0"(_res), "r" (lock));
!         return (int) _res;
  }
  
! #endif   /* __arm__ */
  
  #if defined(__s390__)
  /*
   * S/390 Linux
   */
! #define TAS(lock)      tas(lock)
  
  static inline int
  tas(volatile slock_t *lock)
  {
!  int _res;
  
!         __asm__ __volatile("    la    1,1\n"
!                            "    l     2,%2\n"
!                            "    slr   0,0\n"
!                            "    cs    0,1,0(2)\n"
!                            "    lr    %1,0"
!                            : "=m" (lock), "=d" (_res)
!                            : "m" (lock)
!                            : "0", "1", "2");
  
!        return (_res);
  }
- #endif  /* __s390__ */
  
  
  #if defined(__sparc__)
  #define TAS(lock) tas(lock)
  
--- 157,200 ----
  static __inline__ int
  tas(volatile slock_t *lock)
  {
! 	register slock_t _res = 1;
  
! 	__asm__(
! 			"swpb %0, %0, [%3];"
! :			"=r"(_res), "=m"(*lock)
! :			"0"(_res), "r"(lock));
! 	return (int) _res;
  }
  
! #endif	 /* __arm__ */
  
  #if defined(__s390__)
  /*
   * S/390 Linux
   */
! #define TAS(lock)	   tas(lock)
  
  static inline int
  tas(volatile slock_t *lock)
  {
! 	int			_res;
  
! 	__asm__		__volatile(
! 									   "la 1,1;"
! 									   "l 2,%2;"
! 									   "slr 0,0;"
! 									   "cs 0,1,0(2);"
! 									   "lr %1,0;"
! 						   :		   "=m"(lock), "=d"(_res)
! 						   :		   "m"(lock)
! 						   :		   "0", "1", "2");
  
! 	return (_res);
  }
  
+ #endif	 /* __s390__ */
  
+ 
  #if defined(__sparc__)
  #define TAS(lock) tas(lock)
  
***************
*** 183,190 ****
  {
  	register slock_t _res = 1;
  
! 	__asm__("ldstub [%2], %0" \
! :			"=r"(_res), "=m"(*lock) \
  :			"r"(lock));
  	return (int) _res;
  }
--- 203,211 ----
  {
  	register slock_t _res = 1;
  
! 	__asm__(
! 			"ldstub [%2], %0;"
! :			"=r"(_res), "=m"(*lock)
  :			"r"(lock));
  	return (int) _res;
  }
***************
*** 199,214 ****
  tas(volatile slock_t *lock)
  {
  	register int rv;
! 	
! 	__asm__ __volatile__ (
! 		"tas %1; sne %0"
! 		: "=d" (rv), "=m"(*lock)
! 		: "1" (*lock)
! 		: "cc" );
  	return rv;
  }
  
! #endif /* defined(__mc68000__) && defined(__linux__) */
  
  
  #if defined(NEED_VAX_TAS_ASM)
--- 220,237 ----
  tas(volatile slock_t *lock)
  {
  	register int rv;
! 
! 	__asm__		__volatile__(
! 										 "tas %1;"
! 										 "sne %0;"
! 							 :			 "=d"(rv), "=m"(*lock)
! 							 :			 "1"(*lock)
! 							 :			 "cc");
! 
  	return rv;
  }
  
! #endif	 /* defined(__mc68000__) && defined(__linux__) */
  
  
  #if defined(NEED_VAX_TAS_ASM)
***************
*** 225,237 ****
  {
  	register	_res;
  
! 	__asm__("	movl $1, r0 \
! 			bbssi $0, (%1), 1f \
! 			clrl r0 \
! 1:			movl r0, %0 "
! :			"=r"(_res)			/* return value, in register */
! :			"r"(lock)			/* argument, 'lock pointer', in register */
! :			"r0");				/* inline code uses this register */
  	return (int) _res;
  }
  
--- 248,261 ----
  {
  	register	_res;
  
! 	__asm__(
! 			"movl $1, r0;"
! 			"bbssi $0, (%1), 1f;"
! 			"clrl r0;"
! 			"1: movl r0, %0;"
! :			"=r"(_res)
! :			"r"(lock)
! :			"r0");
  	return (int) _res;
  }
  
***************
*** 244,257 ****
  static __inline__ int
  tas(volatile slock_t *lock)
  {
!   register _res;
!   __asm__("sbitb 0, %0 \n\
! 	sfsd %1"
! 	: "=m"(*lock), "=r"(_res));
!   return (int) _res; 
  }
  
! #endif  /* NEED_NS32K_TAS_ASM */
  
  
  
--- 268,283 ----
  static __inline__ int
  tas(volatile slock_t *lock)
  {
! 	register	_res;
! 
! 	__asm__(
! 			"sbitb 0, %0;"
! 			"sfsd %1;"
! :			"=m"(*lock), "=r"(_res));
! 	return (int) _res;
  }
  
! #endif	 /* NEED_NS32K_TAS_ASM */
  
  
  
***************
*** 268,274 ****
  tas(volatile slock_t *s_lock)
  {
  /* UNIVEL wants %mem in column 1, so we don't pg_indent this file */
! %mem s_lock
  	pushl %ebx
  	movl s_lock, %ebx
  	movl $255, %eax
--- 294,300 ----
  tas(volatile slock_t *s_lock)
  {
  /* UNIVEL wants %mem in column 1, so we don't pg_indent this file */
! 	%mem s_lock
  	pushl %ebx
  	movl s_lock, %ebx
  	movl $255, %eax
***************
*** 277,283 ****
  	popl %ebx
  }
  
! #endif /* defined(NEED_I386_TAS_ASM) && defined(USE_UNIVEL_CC) */
  
  #endif	 /* defined(__GNUC__) */
  
--- 303,309 ----
  	popl %ebx
  }
  
! #endif	 /* defined(NEED_I386_TAS_ASM) && defined(USE_UNIVEL_CC) */
  
  #endif	 /* defined(__GNUC__) */
  
***************
*** 300,329 ****
  #if defined(__GNUC__)
  
  #define TAS(lock)  tas(lock)
! #define S_UNLOCK(lock)  do { __asm__ volatile ("mb"); *(lock) = 0; } while (0)
  
  static __inline__ int
  tas(volatile slock_t *lock)
  {
  	register slock_t _res;
  
! 	__asm__ volatile
! ("		ldq   $0, %0		\n\
! 		bne   $0, 2f		\n\
! 		ldq_l %1, %0		\n\
! 		bne   %1, 2f		\n\
! 		mov   1, $0			\n\
! 		stq_c $0, %0		\n\
! 		beq   $0, 2f		\n\
! 		mb					\n\
! 		br    3f			\n\
! 	 2: mov   1, %1			\n\
! 	 3:       \n" : "=m"(*lock), "=r"(_res) : : "0");
  
  	return (int) _res;
  }
  
! #else /* !defined(__GNUC__) */
  
  /*
   * The Tru64 compiler doesn't support gcc-style inline asm, but it does
--- 326,358 ----
  #if defined(__GNUC__)
  
  #define TAS(lock)  tas(lock)
! #define S_UNLOCK(lock)	do { __asm__ volatile ("mb"); *(lock) = 0; } while (0)
  
  static __inline__ int
  tas(volatile slock_t *lock)
  {
  	register slock_t _res;
  
! 	__asm__		volatile(
! 									 "ldq   $0, %0;"
! 									 "bne   $0, 2f;"
! 									 "ldq_l %1, %0;"
! 									 "bne   %1, 2f;"
! 									 "mov   1, $0;"
! 									 "stq_c $0, %0;"
! 									 "beq   $0, 2f;"
! 									 "mb;"
! 									 "br 3f;"
! 									 "2: mov   1, %1;"
! 									 "3:"
! 						 :			 "=m"(*lock), "=r"(_res)
! 						 :
! 						 :			 "0");
  
  	return (int) _res;
  }
  
! #else							/* !defined(__GNUC__) */
  
  /*
   * The Tru64 compiler doesn't support gcc-style inline asm, but it does
***************
*** 337,348 ****
  #include <alpha/builtins.h>
  
  #define S_INIT_LOCK(lock)  (*(lock) = 0)
! #define TAS(lock)          (__LOCK_LONG_RETRY((lock), 1) == 0)
! #define S_UNLOCK(lock)     __UNLOCK_LONG(lock)
  
! #endif /* defined(__GNUC__) */
  
! #endif /* __alpha */
  
  
  #if defined(__hpux)
--- 366,377 ----
  #include <alpha/builtins.h>
  
  #define S_INIT_LOCK(lock)  (*(lock) = 0)
! #define TAS(lock)		   (__LOCK_LONG_RETRY((lock), 1) == 0)
! #define S_UNLOCK(lock)	   __UNLOCK_LONG(lock)
  
! #endif	 /* defined(__GNUC__) */
  
! #endif	 /* __alpha */
  
  
  #if defined(__hpux)
***************
*** 373,390 ****
   *
   * Note that slock_t under QNX is sem_t instead of char
   */
! #define TAS(lock)       (sem_trywait((lock)) < 0)
! #define S_UNLOCK(lock)  sem_post((lock))
! #define S_INIT_LOCK(lock)       sem_init((lock), 1, 1)
! #define S_LOCK_FREE(lock)       ((lock)->value)
! #endif   /* __QNX__ */
  
  
  #if defined(__sgi)
  /*
   * SGI IRIX 5
   * slock_t is defined as a unsigned long. We use the standard SGI
!  * mutex API. 
   *
   * The following comment is left for historical reasons, but is probably
   * not a good idea since the mutex ABI is supported.
--- 402,419 ----
   *
   * Note that slock_t under QNX is sem_t instead of char
   */
! #define TAS(lock)		(sem_trywait((lock)) < 0)
! #define S_UNLOCK(lock)	sem_post((lock))
! #define S_INIT_LOCK(lock)		sem_init((lock), 1, 1)
! #define S_LOCK_FREE(lock)		((lock)->value)
! #endif	 /* __QNX__ */
  
  
  #if defined(__sgi)
  /*
   * SGI IRIX 5
   * slock_t is defined as a unsigned long. We use the standard SGI
!  * mutex API.
   *
   * The following comment is left for historical reasons, but is probably
   * not a good idea since the mutex ABI is supported.
***************
*** 402,408 ****
  
  #if defined(sinix)
  /*
!  * SINIX / Reliant UNIX 
   * slock_t is defined as a struct abilock_t, which has a single unsigned long
   * member. (Basically same as SGI)
   *
--- 431,437 ----
  
  #if defined(sinix)
  /*
!  * SINIX / Reliant UNIX
   * slock_t is defined as a struct abilock_t, which has a single unsigned long
   * member. (Basically same as SGI)
   *
***************
*** 412,418 ****
  #define S_INIT_LOCK(lock)	init_lock(lock)
  #define S_LOCK_FREE(lock)	(stat_lock(lock) == UNLOCKED)
  #endif	 /* sinix */
!  
  
  #if defined(_AIX)
  /*
--- 441,447 ----
  #define S_INIT_LOCK(lock)	init_lock(lock)
  #define S_LOCK_FREE(lock)	(stat_lock(lock) == UNLOCKED)
  #endif	 /* sinix */
! 
  
  #if defined(_AIX)
  /*
***************
*** 440,446 ****
  
  
  
! #else	 /* !HAS_TEST_AND_SET */
  
  /*
   * Fake spinlock implementation using SysV semaphores --- slow and prone
--- 469,475 ----
  
  
  
! #else							/* !HAS_TEST_AND_SET */
  
  /*
   * Fake spinlock implementation using SysV semaphores --- slow and prone
***************
*** 451,469 ****
  typedef struct
  {
  	/* reference to semaphore used to implement this spinlock */
! 	IpcSemaphoreId	semId;
! 	int				sem;
  } slock_t;
  
  extern bool s_lock_free_sema(volatile slock_t *lock);
  extern void s_unlock_sema(volatile slock_t *lock);
  extern void s_init_lock_sema(volatile slock_t *lock);
! extern int tas_sema(volatile slock_t *lock);
  
! #define S_LOCK_FREE(lock)   s_lock_free_sema(lock)
! #define S_UNLOCK(lock)   s_unlock_sema(lock)
! #define S_INIT_LOCK(lock)   s_init_lock_sema(lock)
! #define TAS(lock)   tas_sema(lock)
  
  #endif	 /* HAS_TEST_AND_SET */
  
--- 480,498 ----
  typedef struct
  {
  	/* reference to semaphore used to implement this spinlock */
! 	IpcSemaphoreId semId;
! 	int			sem;
  } slock_t;
  
  extern bool s_lock_free_sema(volatile slock_t *lock);
  extern void s_unlock_sema(volatile slock_t *lock);
  extern void s_init_lock_sema(volatile slock_t *lock);
! extern int	tas_sema(volatile slock_t *lock);
  
! #define S_LOCK_FREE(lock)	s_lock_free_sema(lock)
! #define S_UNLOCK(lock)	 s_unlock_sema(lock)
! #define S_INIT_LOCK(lock)	s_init_lock_sema(lock)
! #define TAS(lock)	tas_sema(lock)
  
  #endif	 /* HAS_TEST_AND_SET */
  
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#1)
Re: s_lock.h cleanup

As long as we're cleaning things up, I would suggest that all the ports
that use gcc assembler be made to declare it uniformly, as

__asm__ __volatile__ ( ... );

As I read the GCC manual, there's some risk of the asm sections getting
moved around in the program flow if they are not marked volatile. Also
we oughta be consistent about using the double-underscore keywords IMHO.

regards, tom lane

#3Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Tom Lane (#2)
Re: s_lock.h cleanup

Done and applied.

As long as we're cleaning things up, I would suggest that all the ports
that use gcc assembler be made to declare it uniformly, as

__asm__ __volatile__ ( ... );

As I read the GCC manual, there's some risk of the asm sections getting
moved around in the program flow if they are not marked volatile. Also
we oughta be consistent about using the double-underscore keywords IMHO.

regards, tom lane

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
#4Peter Eisentraut
peter_e@gmx.net
In reply to: Bruce Momjian (#1)
Re: [PATCHES] s_lock.h cleanup

Bruce Momjian writes:

In looking at the VAX ASM problem, I realized that the ASM in s_lock.h
is all formatted differently, making it even more confusing. I have
applied the following patch to s_lock.h to try and clean it up.

I don't believe in this patch at all. It makes the assumption that all
assemblers have equally forgiving lexical rules as a certain subset of
said assemblers. For example, the VAX code does not look at all like the
one back when it still worked.

--
Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/

#5Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Peter Eisentraut (#4)
Re: Re: [PATCHES] s_lock.h cleanup

Bruce Momjian writes:

In looking at the VAX ASM problem, I realized that the ASM in s_lock.h
is all formatted differently, making it even more confusing. I have
applied the following patch to s_lock.h to try and clean it up.

I don't believe in this patch at all. It makes the assumption that all
assemblers have equally forgiving lexical rules as a certain subset of
said assemblers. For example, the VAX code does not look at all like the
one back when it still worked.

I agree the VAX code was changed in the patch, but the VAX person sent
email that he had to add the semicolons to make it work on his platform,
and that the original " \n\" code did compile at all.

I believe the formatting problem was that some code had
"command;command; : lkjasfd : asldfk" while some had them spread over
separate lines, and others used \n\, all very randomly. Now at least
they are all consistent and use similar formatting.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#4)
Re: Re: [PATCHES] s_lock.h cleanup

Peter Eisentraut <peter_e@gmx.net> writes:

Bruce Momjian writes:

In looking at the VAX ASM problem, I realized that the ASM in s_lock.h
is all formatted differently, making it even more confusing. I have
applied the following patch to s_lock.h to try and clean it up.

I don't believe in this patch at all. It makes the assumption that all
assemblers have equally forgiving lexical rules as a certain subset of
said assemblers. For example, the VAX code does not look at all like the
one back when it still worked.

Good point. I think it's safe to use the split-up-string-literal
feature, but assuming that ';' can replace '\n' is sheer folly, and so
is assuming that whitespace doesn't matter (ie, that opcodes starting
in column 1 are OK). Bruce, I'd suggest a format more like

"[label] opcode operands \n"

for each line of assembly code.

regards, tom lane

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#5)
Re: Re: [PATCHES] s_lock.h cleanup

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I believe the formatting problem was that some code had
"command;command; : lkjasfd : asldfk" while some had them spread over
separate lines, and others used \n\, all very randomly. Now at least
they are all consistent and use similar formatting.

And they may all be broken, except for the one(s) you have tested.
You shouldn't be assuming that a platform that uses gcc necessarily
also uses gas.

regards, tom lane

#8Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Tom Lane (#6)
Re: Re: [PATCHES] s_lock.h cleanup

Peter Eisentraut <peter_e@gmx.net> writes:

Bruce Momjian writes:

In looking at the VAX ASM problem, I realized that the ASM in s_lock.h
is all formatted differently, making it even more confusing. I have
applied the following patch to s_lock.h to try and clean it up.

I don't believe in this patch at all. It makes the assumption that all
assemblers have equally forgiving lexical rules as a certain subset of
said assemblers. For example, the VAX code does not look at all like the
one back when it still worked.

Good point. I think it's safe to use the split-up-string-literal
feature, but assuming that ';' can replace '\n' is sheer folly, and so
is assuming that whitespace doesn't matter (ie, that opcodes starting
in column 1 are OK). Bruce, I'd suggest a format more like

"[label] opcode operands \n"

for each line of assembly code.

Interestingly, we have very few non-gcc ASM entries in s_lock.h. The
only non-gcc one I see are Univel/i386, and I didn't touch that. Isn't
the semicolon the standard command terminator for all gcc assemblers?

I see non-gcc stuff in s_lock.c, but I didn't touch that. I also see
volatile missing in s_lock.c, which I will add for GCC entries.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
#9Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Tom Lane (#7)
Re: Re: [PATCHES] s_lock.h cleanup

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I believe the formatting problem was that some code had
"command;command; : lkjasfd : asldfk" while some had them spread over
separate lines, and others used \n\, all very randomly. Now at least
they are all consistent and use similar formatting.

And they may all be broken, except for the one(s) you have tested.
You shouldn't be assuming that a platform that uses gcc necessarily
also uses gas.

Oh, wow, I never suspected gcc could work without gas. Can it?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
#10Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Tom Lane (#7)
Re: Re: [PATCHES] s_lock.h cleanup

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I believe the formatting problem was that some code had
"command;command; : lkjasfd : asldfk" while some had them spread over
separate lines, and others used \n\, all very randomly. Now at least
they are all consistent and use similar formatting.

And they may all be broken, except for the one(s) you have tested.
You shouldn't be assuming that a platform that uses gcc necessarily
also uses gas.

regards, tom lane

I can tell you that they all used __asm__, and all used the colon
terminators for each __asm__ block:

* __asm__ __volatile__(
* "command;"
* "command;"
* "command;"
* : "=r"(_res) return value, in register
* : "r"(lock) argument, 'lock pointer', in register
* : "r0"); inline code uses this register

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#10)
Re: Re: [PATCHES] s_lock.h cleanup

Bruce Momjian <pgman@candle.pha.pa.us> writes:

And they may all be broken, except for the one(s) you have tested.
You shouldn't be assuming that a platform that uses gcc necessarily
also uses gas.

I can tell you that they all used __asm__, and all used the colon
terminators for each __asm__ block:

* __asm__ __volatile__(
* "command;"
* "command;"
* "command;"
* : "=r"(_res) return value, in register
* : "r"(lock) argument, 'lock pointer', in register
* : "r0"); inline code uses this register

The __asm___ and splitting up the assembly code into multiple string
literals and the consistent formatting of the register addendums are
all fine, because those are read by gcc and this whole code block is
gcc-only. But the assembly code string literal will be spit out
essentially verbatim by gcc to the assembler, and the assembler may
not be nearly as forgiving as you think.

Oh, wow, I never suspected gcc could work without gas. Can it?

Gcc with platform-specific as used to be the standard configuration
on HPUX, and may still be standard on some platforms.

Bottom line: I see no point in taking any risks, especially not this
late in beta, with code that you cannot test for yourself, and
*especially* not when the change is only for cosmetic reasons.

regards, tom lane

#12Larry Rosenman
ler@lerctr.org
In reply to: Tom Lane (#11)
Re: Re: [PATCHES] s_lock.h cleanup

* Tom Lane <tgl@sss.pgh.pa.us> [010119 13:08]:

Oh, wow, I never suspected gcc could work without gas. Can it?

Gcc with platform-specific as used to be the standard configuration
on HPUX, and may still be standard on some platforms.

Still is the standard on UnixWare with GCC. The standard assembler
and linker are used.

Bottom line: I see no point in taking any risks, especially not this
late in beta, with code that you cannot test for yourself, and
*especially* not when the change is only for cosmetic reasons.

I agree with this sentiment.

LER

--
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 972-414-9812 E-Mail: ler@lerctr.org
US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749

In reply to: Bruce Momjian (#8)
Re: Re: [PATCHES] s_lock.h cleanup

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Interestingly, we have very few non-gcc ASM entries in s_lock.h. The
only non-gcc one I see are Univel/i386, and I didn't touch that. Isn't
the semicolon the standard command terminator for all gcc assemblers?

No.

It is for most, but not for the a29k, AVR, CRIS, d10v, d30v, FR30,
H8/300, HP/PA, TIC30, TIC54x, or TIC80.

Aren't you glad you know that now?

Ian
Former GNU binutils maintainer

In reply to: Bruce Momjian (#9)
Re: Re: [PATCHES] s_lock.h cleanup

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I believe the formatting problem was that some code had
"command;command; : lkjasfd : asldfk" while some had them spread over
separate lines, and others used \n\, all very randomly. Now at least
they are all consistent and use similar formatting.

And they may all be broken, except for the one(s) you have tested.
You shouldn't be assuming that a platform that uses gcc necessarily
also uses gas.

Oh, wow, I never suspected gcc could work without gas. Can it?

Yes.

In fact, I don't think there is any Unix system on which gcc requires
gas. There used to be at least one, but I think they have all been
cleaned up at this point.

Ian

#15Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Tom Lane (#11)
1 attachment(s)
Re: Re: [PATCHES] s_lock.h cleanup

The __asm___ and splitting up the assembly code into multiple string
literals and the consistent formatting of the register addendums are
all fine, because those are read by gcc and this whole code block is
gcc-only. But the assembly code string literal will be spit out
essentially verbatim by gcc to the assembler, and the assembler may
not be nearly as forgiving as you think.

Oh, wow, I never suspected gcc could work without gas. Can it?

Gcc with platform-specific as used to be the standard configuration
on HPUX, and may still be standard on some platforms.

Bottom line: I see no point in taking any risks, especially not this
late in beta, with code that you cannot test for yourself, and
*especially* not when the change is only for cosmetic reasons.

OK, remove semicolons and put back the \n at the end of each line.
Patch attached.

I wasn't going to mess with this while in beta, but when I found the VAX
code broken, it seemed worth making sure they were all OK. The VAX
stuff was broken because in 7.0.3 it shows:

__asm__(" movl $1, r0 \
bbssi $0, (%1), 1 f \
clrl r0 \
1: movl r0, %0 "

The '1 f' we broken, but also the thing missing here is \n\. With \, it
just makes one long line, which certainly can't be asembled. The VAX
guy added semicolons, but I can see that \n\ is safer, and have done
that.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Attachments:

/bjm/difftext/plainDownload
? config.log
? config.cache
? config.status
? GNUmakefile
? src/Makefile.custom
? src/GNUmakefile
? src/Makefile.global
? src/log
? src/crtags
? src/backend/postgres
? src/backend/catalog/global.bki
? src/backend/catalog/global.description
? src/backend/catalog/template1.bki
? src/backend/catalog/template1.description
? src/backend/port/Makefile
? src/bin/initdb/initdb
? src/bin/initlocation/initlocation
? src/bin/ipcclean/ipcclean
? src/bin/pg_config/pg_config
? src/bin/pg_ctl/pg_ctl
? src/bin/pg_dump/pg_dump
? src/bin/pg_dump/pg_restore
? src/bin/pg_dump/pg_dumpall
? src/bin/pg_id/pg_id
? src/bin/pg_passwd/pg_passwd
? src/bin/pgaccess/pgaccess
? src/bin/pgtclsh/Makefile.tkdefs
? src/bin/pgtclsh/Makefile.tcldefs
? src/bin/pgtclsh/pgtclsh
? src/bin/pgtclsh/pgtksh
? src/bin/psql/psql
? src/bin/scripts/createlang
? src/include/config.h
? src/include/stamp-h
? src/interfaces/ecpg/lib/libecpg.so.3.2.0
? src/interfaces/ecpg/preproc/ecpg
? src/interfaces/libpgeasy/libpgeasy.so.2.1
? src/interfaces/libpgtcl/libpgtcl.so.2.1
? src/interfaces/libpq/libpq.so.2.1
? src/interfaces/perl5/blib
? src/interfaces/perl5/Makefile
? src/interfaces/perl5/pm_to_blib
? src/interfaces/perl5/Pg.c
? src/interfaces/perl5/Pg.bs
? src/pl/plperl/blib
? src/pl/plperl/Makefile
? src/pl/plperl/pm_to_blib
? src/pl/plperl/SPI.c
? src/pl/plperl/plperl.bs
? src/pl/plpgsql/src/libplpgsql.so.1.0
? src/pl/tcl/Makefile.tcldefs
Index: src/backend/storage/buffer/s_lock.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/storage/buffer/s_lock.c,v
retrieving revision 1.29
diff -c -r1.29 s_lock.c
*** src/backend/storage/buffer/s_lock.c	2001/01/14 05:08:15	1.29
--- src/backend/storage/buffer/s_lock.c	2001/01/19 20:28:20
***************
*** 115,123 ****
  	}
  }
  
- 
- 
- 
  /*
   * Various TAS implementations that cannot live in s_lock.h as no inline
   * definition exists (yet).
--- 115,120 ----
***************
*** 136,153 ****
  tas_dummy()						/* really means: extern int tas(slock_t
  								 * **lock); */
  {
! 	__asm__("		\n\
! .global		_tas		\n\
! _tas:				\n\
! 	movel   sp@(0x4),a0	\n\
! 	tas a0@			\n\
! 	beq _success		\n\
! 	moveq   #-128,d0	\n\
! 	rts			\n\
! _success:			\n\
! 	moveq   #0,d0		\n\
! 	rts			\n\
! 	");
  }
  
  #endif	 /* __m68k__ */
--- 133,150 ----
  tas_dummy()						/* really means: extern int tas(slock_t
  								 * **lock); */
  {
! 	__asm__ __volatile__(
! "\
! .global		_tas				\n\
! _tas:							\n\
! 			movel	sp@(0x4),a0	\n\
! 			tas 	a0@			\n\
! 			beq 	_success	\n\
! 			moveq 	#-128,d0	\n\
! 			rts					\n\
! _success:						\n\
! 			moveq 	#0,d0		\n\
! 			rts");
  }
  
  #endif	 /* __m68k__ */
***************
*** 160,181 ****
  static void
  tas_dummy()
  {
!        __asm__("               \n\
!                .globl  tas     \n\
!                .globl  _tas    \n\
! _tas:                          \n\
! tas:                           \n\
!                lwarx   r5,0,r3 \n\
!                cmpwi   r5,0    \n\
!                bne     fail    \n\
!                addi    r5,r5,1 \n\
!                stwcx.  r5,0,r3 \n\
!                beq     success \n\
! fail:          li      r3,1    \n\
!                blr             \n\
! success:                       \n\
!                li r3,0         \n\
!                blr             \n\
  	");
  }
  
--- 157,179 ----
  static void
  tas_dummy()
  {
!        __asm__ __volatile__(
! "\
! 			.globl tas			\n\
! 			.globl _tas			\n\
! _tas:							\n\
! tas:							\n\
! 			lwarx	r5,0,r3		\n\
! 			cmpwi 	r5,0		\n\
! 			bne 	fail		\n\
! 			addi 	r5,r5,1		\n\
! 			stwcx. 	r5,0,r3		\n\
! 			beq 	success		\n\
! fail:		li 		r3,1		\n\
! 			blr 				\n\
! success:						\n\
! 			li 		r3,0		\n\
! 			blr 				\n\
  	");
  }
  
***************
*** 186,206 ****
  static void
  tas_dummy()
  {
! 	__asm__("		\n\
! .global		tas		\n\
! tas:				\n\
! 		lwarx	5,0,3	\n\
! 		cmpwi	5,0	\n\
! 		bne	fail	\n\
! 		addi	5,5,1	\n\
!         	stwcx.  5,0,3	\n\
!      		beq	success	\n\
! fail:		li	3,1	\n\
! 		blr		\n\
! success:			\n\
! 		li 3,0		\n\
!         	blr		\n\
! 	");
  }
  
  #endif	 /* __powerpc__ */
--- 184,204 ----
  static void
  tas_dummy()
  {
! 	__asm__ __volatile__(
! "\
! .global tas 					\n\
! tas:							\n\
! 			lwarx	5,0,3		\n\
! 			cmpwi 	5,0 		\n\
! 			bne 	fail		\n\
! 			addi 	5,5,1		\n\
! 			stwcx.	5,0,3		\n\
! 			beq 	success 	\n\
! fail:		li		3,1 		\n\
! 			blr 				\n\
! success:						\n\
! 			li 		3,0			\n\
! 			blr");
  }
  
  #endif	 /* __powerpc__ */
***************
*** 209,230 ****
  static void
  tas_dummy()
  {
! 	__asm__("		\n\
! .global	tas			\n\
! tas:				\n\
! 	.frame	$sp, 0, $31	\n\
! 	ll	$14, 0($4)	\n\
! 	or	$15, $14, 1	\n\
! 	sc	$15, 0($4)	\n\
! 	beq	$15, 0, fail	\n\
! 	bne	$14, 0, fail	\n\
! 	li	$2, 0		\n\
! 	.livereg 0x2000FF0E,0x00000FFF	\n\
! 	j       $31		\n\
! fail:				\n\
! 	li	$2, 1		\n\
! 	j       $31		\n\
! 	");
  }
  
  #endif	 /* __mips__ */
--- 207,228 ----
  static void
  tas_dummy()
  {
! 	__asm__  _volatile__(
! "\
! .global	tas						\n\
! tas:							\n\
! 			.frame	$sp, 0, $31	\n\
! 			ll		$14, 0($4)	\n\
! 			or		$15, $14, 1	\n\
! 			sc		$15, 0($4)	\n\
! 			beq		$15, 0, fail\n\
! 			bne		$14, 0, fail\n\
! 			li		$2, 0		\n\
! 			.livereg 0x2000FF0E,0x00000FFF	\n\
! 			j		$31			\n\
! fail:							\n\
! 			li		$2, 1		\n\
! 			j   	$31");
  }
  
  #endif	 /* __mips__ */
Index: src/include/storage/s_lock.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/storage/s_lock.h,v
retrieving revision 1.82
diff -c -r1.82 s_lock.h
*** src/include/storage/s_lock.h	2001/01/19 07:03:53	1.82
--- src/include/storage/s_lock.h	2001/01/19 20:28:21
***************
*** 103,111 ****
   * Standard _asm format:
   *
   *	__asm__ __volatile__(
!  *			"command;"
!  *			"command;"
!  *			"command;"
   *		:	"=r"(_res)			return value, in register
   *		:	"r"(lock)			argument, 'lock pointer', in register
   *		:	"r0");				inline code uses this register
--- 103,111 ----
   * Standard _asm format:
   *
   *	__asm__ __volatile__(
!  *			"command	\n"
!  *			"command	\n"
!  *			"command	\n"
   *		:	"=r"(_res)			return value, in register
   *		:	"r"(lock)			argument, 'lock pointer', in register
   *		:	"r0");				inline code uses this register
***************
*** 121,128 ****
  	register slock_t _res = 1;
  
  	__asm__ __volatile__(
! 						"lock;"
! 						"xchgb %0,%1;"
  			:			"=q"(_res), "=m"(*lock)
  			:			"0"(_res));
  	return (int) _res;
--- 121,128 ----
  	register slock_t _res = 1;
  
  	__asm__ __volatile__(
! 						"lock			\n"
! 						"xchgb	%0,%1	\n"
  			:			"=q"(_res), "=m"(*lock)
  			:			"0"(_res));
  	return (int) _res;
***************
*** 140,146 ****
  	long int	ret;
  
  	__asm__ __volatile__(
! 						"xchg4 %0=%1,%2;"
  			 :			"=r"(ret), "=m"(*lock)
  			 :			"r"(1), "1"(*lock)
  			 :			"memory");
--- 140,146 ----
  	long int	ret;
  
  	__asm__ __volatile__(
! 						"xchg4 	%0=%1,%2		\n"
  			 :			"=r"(ret), "=m"(*lock)
  			 :			"r"(1), "1"(*lock)
  			 :			"memory");
***************
*** 160,166 ****
  	register slock_t _res = 1;
  
  	__asm__ __volatile__(
! 						"swpb %0, %0, [%3];"
  			:			"=r"(_res), "=m"(*lock)
  			:			"0"(_res), "r"(lock));
  	return (int) _res;
--- 160,166 ----
  	register slock_t _res = 1;
  
  	__asm__ __volatile__(
! 						"swpb 	%0, %0, [%3]	\n"
  			:			"=r"(_res), "=m"(*lock)
  			:			"0"(_res), "r"(lock));
  	return (int) _res;
***************
*** 180,190 ****
  	int			_res;
  
  	__asm__	__volatile__(
! 						"la 1,1;"
! 						"l 2,%2;"
! 						"slr 0,0;"
! 						"cs 0,1,0(2);"
! 						"lr %1,0;"
  		   :			"=m"(lock), "=d"(_res)
  		   :			"m"(lock)
  		   :			"0", "1", "2");
--- 180,190 ----
  	int			_res;
  
  	__asm__	__volatile__(
! 						"la	1,1			\n"
! 						"l 	2,%2			\n"
! 						"slr 0,0		\n"
! 						"cs 0,1,0(2)	\n"
! 						"lr %1,0		\n"
  		   :			"=m"(lock), "=d"(_res)
  		   :			"m"(lock)
  		   :			"0", "1", "2");
***************
*** 204,210 ****
  	register slock_t _res = 1;
  
  	__asm__ __volatile__(
! 						"ldstub [%2], %0;"
  			:			"=r"(_res), "=m"(*lock)
  			:			"r"(lock));
  	return (int) _res;
--- 204,210 ----
  	register slock_t _res = 1;
  
  	__asm__ __volatile__(
! 						"ldstub	[%2], %0		\n"
  			:			"=r"(_res), "=m"(*lock)
  			:			"r"(lock));
  	return (int) _res;
***************
*** 222,229 ****
  	register int rv;
  
  	__asm__	__volatile__(
! 						"tas %1;"
! 						"sne %0;"
  			 :			"=d"(rv), "=m"(*lock)
  			 :			"1"(*lock)
  			 :			"cc");
--- 222,229 ----
  	register int rv;
  
  	__asm__	__volatile__(
! 						"tas %1		\n"
! 						"sne %0		\n"
  			 :			"=d"(rv), "=m"(*lock)
  			 :			"1"(*lock)
  			 :			"cc");
***************
*** 249,258 ****
  	register	_res;
  
  	__asm__ __volatile__(
! 						"movl $1, r0;"
! 						"bbssi $0, (%1), 1f;"
! 						"clrl r0;"
! 						"1: movl r0, %0;"
  			:			"=r"(_res)
  			:			"r"(lock)
  			:			"r0");
--- 249,258 ----
  	register	_res;
  
  	__asm__ __volatile__(
! 						"movl 	$1, r0			\n"
! 						"bbssi 	$0, (%1), 1f	\n"
! 						"clrl 	r0				\n"
! 						"1: movl r0, %0			\n"
  			:			"=r"(_res)
  			:			"r"(lock)
  			:			"r0");
***************
*** 271,278 ****
  	register	_res;
  
  	__asm__ __volatile__(
! 						"sbitb 0, %0;"
! 						"sfsd %1;"
  			:			"=m"(*lock), "=r"(_res));
  	return (int) _res;
  }
--- 271,278 ----
  	register	_res;
  
  	__asm__ __volatile__(
! 						"sbitb 	0, %0	\n"
! 						"sfsd 	%1		\n"
  			:			"=m"(*lock), "=r"(_res));
  	return (int) _res;
  }
***************
*** 339,354 ****
  	register slock_t _res;
  
  	__asm__	__volatile__(
! 						"ldq   $0, %0;"
! 						"bne   $0, 2f;"
! 						"ldq_l %1, %0;"
! 						"bne   %1, 2f;"
! 						"mov   1, $0;"
! 						"stq_c $0, %0;"
! 						"beq   $0, 2f;"
! 						"mb;"
! 						"br 3f;"
! 						"2: mov   1, %1;"
  						"3:"
  			 :			"=m"(*lock), "=r"(_res)
  			 :
--- 339,354 ----
  	register slock_t _res;
  
  	__asm__	__volatile__(
! 						"ldq   $0, %0	\n"
! 						"bne   $0, 2f	\n"
! 						"ldq_l %1, %0	\n"
! 						"bne   %1, 2f	\n"
! 						"mov   1,  $0	\n"
! 						"stq_c $0, %0	\n"
! 						"beq   $0, 2f	\n"
! 						"mb				\n"
! 						"br 3f			\n"
! 						"2: mov 1, %1	\n"
  						"3:"
  			 :			"=m"(*lock), "=r"(_res)
  			 :