[9fans] Fwd: Drawterm problems

["Martín Ferrari" <martin.ferrari@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3047 bytes --]

---------- Forwarded message ----------
From: Martín Ferrari <martin.ferrari@gmail.com>
Date: Tue, Dec 9, 2008 at 03:13
Subject: Drawterm problems
To: Russ Cox <rsc@swtch.com>
Cc: Fans of the OS Plan 9 from Bell Labs <9fans@9fans.net>, Thiemo
Seufer <ths@networkno.de>, 508068@bugs.debian.org, Marc 'HE'
Brockschmidt <marc@marcbrockschmidt.de>, 508100@bugs.debian.org,
Damián Viano <des@debian.org>


(Copying 9fans in case somebody can lend a hand on this, copying the
Debian bug report(er)s)
Hi Russ,

I'm a Plan9 newbie and Debian Developer, and I'm currently tying to
fix bugs that showed after I uploaded drawterm to Debian. However I
have hit some problems that are being difficult to solve. I've managed
to fix most of them, and I'd like to share the patches.

First problem: it doesn't compile on mips at all, the assembly code
seems invalid. In [0] you can find a proposed patch.

Second problem: the arch handling is quite fragile, and it doesn't
work for sparc under linux [1] (uname -m is sparc64, not sun4u), for
example. But the biggest problem is that there are no provisions for
other architectures. I have prepared a big patch for this, see below.

Third: segfaults on amd64, possibly other arches that I couldn't test.
After a lot of guessing, I suspected of these constructs in lib.h:

typedef int            p9_long;
#define long  int

Changing them to match longs with longs made the segfault go away, as
amd64 uses 64bits for longs and the included libX.h was botched
because of the "#define long int". But then it popped many type errors
all around, since there are many inconsistencies (for example,
function prototypes that doesn't match exactly the fuction
definitions) and all the code assumes long==int. I'm trying to fix
this, but the problem is very deep: there are many places where size
specific types should be used instead of longs and ints.

I'm still trying to fix this, and would greatly appreciate any help.


~~~~

I'm including the arch-related patch: it disables all the posix-*/
code (except for the x86 hashing routines, which are now conditionally
complied from libsec) and instead creates a libmachdep directory with
a tas.c file. In tas.c it tries to use the fairly new builtin atomic
operations, and falls back to assemby if it's available. Also, the
getcallerpc function is replaced with other gcc builtin, but I haven't
made provisions for fallback to assembly here as I think that the
builtin I'm using is widely available and that would have made the
patch very complex.

This change enabled drawterm to compile on these architectures: i386
amd64 sparc64 mips mipsel alpha ia64 powerpc s390. But since most of
the porter machines I can use don't have X forwarding, I cannot test
that it works correctly. sparc64 and i386 were tested and it seems to
work ok.


[0] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508068
[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508100

--
Martín Ferrari



-- 
Martín Ferrari

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: replace-archspecific-with-gcc-extensions.patch --]
[-- Type: text/x-patch; name=replace-archspecific-with-gcc-extensions.patch, Size: 6909 bytes --]

Index: drawterm/include/lib.h
===================================================================
--- drawterm.orig/include/lib.h
+++ drawterm/include/lib.h
@@ -240,7 +240,12 @@ extern	int	fmtprint(Fmt*, char*, ...);
 extern	int	fmtvprint(Fmt*, char*, va_list);
 extern	void*	mallocz(ulong, int);
 
-extern	uintptr	getcallerpc(void*);
+/* Use a GCC extension that provides the same, without specifying the actual
+ * assembly code. In the code is always used to return the caller to the actual
+ * function, so I ignore the argument */
+/* extern	uintptr	getcallerpc(void*); */
+#define		getcallerpc(a) \
+		((uintptr)__builtin_return_address(0))
 extern	char*	cleanname(char*);
 extern	void	sysfatal(char*, ...);
 extern	char*	strecpy(char*, char*, char*);
Index: drawterm/Make.irix
===================================================================
--- drawterm.orig/Make.irix
+++ drawterm/Make.irix
@@ -20,5 +20,4 @@ MAKE=gmake
 
 all: default
 
-libmachdep.a:
-	(cd posix-mips && $(MAKE))
+ARCH=mips
Index: drawterm/Make.osx
===================================================================
--- drawterm.orig/Make.osx
+++ drawterm/Make.osx
@@ -15,6 +15,5 @@ AUDIO=none
 
 all: default
 
-libmachdep.a:
-	arch=`uname -m|sed 's/i.86/386/;s/Power Macintosh/power/'`; \
-	(cd posix-$$arch &&  make)
+ARCH := $(shell uname -m |\
+	sed 's/i.86/386/;s/Power Macintosh/power/;s/x86_64/amd64/' )
Index: drawterm/Make.osx-x11
===================================================================
--- drawterm.orig/Make.osx-x11
+++ drawterm/Make.osx-x11
@@ -16,6 +16,5 @@ AUDIO=none
 
 all: default
 
-libmachdep.a:
-	arch=`uname -m|sed 's/i.86/386/;s/Power Macintosh/power/'`; \
-	(cd posix-$$arch &&  make)
+ARCH := $(shell uname -m |\
+	sed 's/i.86/386/;s/Power Macintosh/power/;s/x86_64/amd64/' )
Index: drawterm/Make.sun
===================================================================
--- drawterm.orig/Make.sun
+++ drawterm/Make.sun
@@ -16,6 +16,5 @@ AUDIO=none
 
 all: default
 
-libmachdep.a:
-	arch=`uname -m|sed 's/i.86/386/;s/Power Macintosh/power/'`; \
-	(cd posix-$$arch &&  make)
+ARCH := $(shell uname -m |\
+	sed 's/i.86/386/;s/Power Macintosh/power/;s/x86_64/amd64/' )
Index: drawterm/Make.unix
===================================================================
--- drawterm.orig/Make.unix
+++ drawterm/Make.unix
@@ -18,6 +18,5 @@ AUDIO=unix
 
 all: default
 
-libmachdep.a:
-	arch=`uname -m|sed 's/i.86/386/;s/Power Macintosh/power/; s/x86_64/amd64/'`; \
-	(cd posix-$$arch &&  make)
+ARCH := $(shell uname -m |\
+	sed 's/i.86/386/;s/Power Macintosh/power/;s/x86_64/amd64/' )
Index: drawterm/Make.win32
===================================================================
--- drawterm.orig/Make.win32
+++ drawterm/Make.win32
@@ -33,10 +33,7 @@ XOFILES=9ball.$O
 
 all: default
 
-# for root
-libmachdep.a:
-	(cd win32-386; make)
-
 9ball.$O: 9ball.rc 9ball.ico
 	$(WINDRES) -i 9ball.rc -o 9ball.o
 
+ARCH=386
Index: drawterm/Makefile
===================================================================
--- drawterm.orig/Makefile
+++ drawterm/Makefile
@@ -25,7 +25,7 @@ LIBS1=\
 	libc/libc.a\
 
 # stupid gcc
-LIBS=$(LIBS1) $(LIBS1) $(LIBS1) libmachdep.a
+LIBS=$(LIBS1) $(LIBS1) $(LIBS1) libmachdep/libmachdep.a
 
 default: $(TARG)
 $(TARG): $(OFILES) $(LIBS)
@@ -69,3 +69,6 @@ libc/libc.a:
 
 gui-$(GUI)/libgui.a:
 	(cd gui-$(GUI); $(MAKE))
+
+libmachdep/libmachdep.a:
+	(cd libmachdep; $(MAKE))
Index: drawterm/libsec/Makefile
===================================================================
--- drawterm.orig/libsec/Makefile
+++ drawterm/libsec/Makefile
@@ -33,6 +33,7 @@ OFILES=\
 	hmac.$O\
 	md4.$O\
 	md5.$O\
+	_md5block.$O\
 	md5pickle.$O\
 	nfastrand.$O\
 	prng.$O\
@@ -45,6 +46,7 @@ OFILES=\
 	rsagen.$O\
 	rsaprivtopub.$O\
 	sha1.$O\
+	_sha1block.$O\
 	sha1pickle.$O\
 	smallprimes.$O
 
@@ -56,3 +58,23 @@ $(LIB): $(OFILES)
 %.$O: %.c
 	$(CC) $(CFLAGS) $*.c
 
+ifeq "$(ARCH)" "386"
+_sha1block.$O: sha1block.s
+	$(AS) -o $@ $<
+
+_md5block.$O: md5block.s
+	$(AS) -o $@ $<
+
+sha1block.s: ../posix-386/sha1block.spp
+	$(CC) -E - < $< > $@
+
+md5block.s: ../posix-386/md5block.spp
+	$(CC) -E - < $< > $@
+else
+_sha1block.$O: sha1block.c
+	echo -$(ARCH)-
+	$(CC) $(CFLAGS) -o $@ $<
+
+_md5block.$O: md5block.c
+	$(CC) $(CFLAGS) -o $@ $<
+endif
Index: drawterm/libmachdep/Makefile
===================================================================
--- /dev/null
+++ drawterm/libmachdep/Makefile
@@ -0,0 +1,14 @@
+ROOT=..
+include ../Make.config
+LIB=libmachdep.a
+
+OFILES=\
+	tas.$O
+
+default: $(LIB)
+$(LIB): $(OFILES)
+	$(AR) r $(LIB) $(OFILES)
+	$(RANLIB) $(LIB)
+
+%.$O: %.c
+	$(CC) $(CFLAGS) $*.c
Index: drawterm/libmachdep/tas.c
===================================================================
--- /dev/null
+++ drawterm/libmachdep/tas.c
@@ -0,0 +1,89 @@
+#include "u.h"
+#include "libc.h"
+
+#define GCC_VERSION (__GNUC__ * 10000 \
+		+ __GNUC_MINOR__ * 100 \
+		+ __GNUC_PATCHLEVEL__)
+
+long
+tas(long *x)
+{
+	long	v = 1;
+#if (! __arm__) && (! __m68k__) && ((GCC_VERSION > 40300) || \
+		(__alpha__ && GCC_VERSION > 40200) || \
+		(__sparc__ && GCC_VERSION > 40200) || \
+		(__s390__ && GCC_VERSION > 40100))
+	v = __sync_lock_test_and_set (x, 1);
+#else /* Old gcc */
+#ifdef __i386__
+	__asm__(	"xchgl  %%eax,(%%ecx)"
+			: "+a" (v)
+			: "c" (x));
+#else /* __i386__ */
+#ifdef __x86_64__
+	__asm__(	"xchgl  %%eax,(%%rcx)"
+			: "+a" (v)
+			: "c" (x));
+#else /* __x86_64__ */
+
+#ifdef __mips__
+#ifdef __linux__
+	__asm__(".set mips2\n");
+#endif
+	__asm__("loop:\n\t"
+			"li	$t1, 12345\n\t"	/* t1 = 12345 */
+			"ll	%0, (%1)\n\t"	/* v0 = *a0 */
+			"sc	$t1, (%1)\n\t"	/* *a0 = t1 if *a0 hasn't changed; t1=success */
+			"beqz	$t1, loop"	/* repeat if *a0 did change */
+			: "=r" (v)
+			: "d" (x)
+			: "t1");
+#ifdef __linux__
+	__asm__(".set mips0\n");
+#endif
+#else /* __mips__ */
+
+#ifdef __powerpc__
+	/*
+	 * this __asm__ works with gcc 2.95.2 (mac os x 10.1).
+	 * this assembly language destroys r0 (0), some other register (v),
+	 * r4 (x) and r5 (temp).
+	 */
+	__asm__("\n	sync\n"
+	"	li	r0,0\n"
+	"	mr	r4,%1		/* &l->val */\n"
+	"	lis	r5,0xdead	/* assemble constant 0xdeaddead */\n"
+	"	ori	r5,r5,0xdead	/* \" */\n"
+	"tas1:\n"
+	"	dcbf	r4,r0	/* cache flush; \"fix for 603x bug\" */\n"
+	"	lwarx	%0,r4,r0	/* v = l->val with reservation */\n"
+	"	cmp	cr0,0,%0,r0	/* v == 0 */\n"
+	"	bne	tas0\n"
+	"	stwcx.	r5,r4,r0   /* if (l->val same) l->val = 0xdeaddead */\n"
+	"	bne	tas1\n"
+	"tas0:\n"
+	"	sync\n"
+	"	isync\n"
+	: "=r" (v)
+	: "r"  (x)
+	: "cc", "memory", "r0", "r4", "r5"
+	);
+
+#else /* __powerpc__ */
+
+#ifdef __sparc__
+	__asm__(	"stbar\n\t"
+			"swap	[ %1 ], %0"
+			: "+g" (v)
+			: "g" (x));
+#else /* __sparc__ */
+#error "Arch not supported with this GCC version"
+#endif /* __sparc__ */
+#endif /* __powerpc__ */
+#endif /* __mips__ */
+#endif /* __x86_64__ */
+#endif /* __i386__ */
+#endif /* Old gcc */
+	return v != 0;
+}
+