64-bit alignment problems.

[Andy Fiddaman]

On Fri, 14 Sep 2007, Ken Murchison wrote:
; This patch tries to force the entire buffer to be aligned, rather than
; aligning each 64-bit field individually.  Don't know if this will work or not.

The concept seems to work fine. There are some other places that
the same trick needs to be done - mailbox_expunge() and restore_expunged()
for example.

I adapted it slightly (see attachment) to use a union to conserve heap
space and a macro given the number of places it was proving necessary.
I also tried using the GNUC extension of specifying alignment but
it doesn't seem to work at all for me so I commented it out.

The following two bits aren't required - the pointer doesn't need to be
aligned, just the memory to which it points. That is already
done by malloc(). Also I don't think that anonymous unions are standard C.

-    unsigned char *buf  = NULL;
+    struct {
+#ifdef HAVE_LONG_LONG_INT
+       bit64 align8;
+#endif
+       unsigned char *buf;
+    } align8buf;
+    unsigned char *buf = align8buf.buf = NULL;

&

-    const char *index_base;
+    union {
+#ifdef HAVE_LONG_LONG_INT
+       bit64 align8;
+#endif
+       const char *index_base;
+    };


I haven't had much time to look at split_attribs() properly so I just
left that the way my previous patch worked for now.

Andy
-------------- next part --------------
diff -ur cyrus-imapd-2.3.9.dist/imap/annotate.c cyrus-imapd-2.3.9/imap/annotate.c
--- cyrus-imapd-2.3.9.dist/imap/annotate.c	2007-08-15 17:20:55.000000000 +0000
+++ cyrus-imapd-2.3.9/imap/annotate.c	2007-09-15 19:20:10.749460641 +0000
@@ -310,7 +310,8 @@
 
     /* xxx use datalen? */
     /* xxx sanity check the data? */
-    attrib->size = (size_t) ntohl(*(unsigned long *) data);
+    memcpy(&tmp, data, sizeof(unsigned long));
+    attrib->size = (size_t) ntohl(tmp);
     data += sizeof(unsigned long); /* skip to value */
 
     attrib->value = data;
diff -ur cyrus-imapd-2.3.9.dist/imap/mailbox.c cyrus-imapd-2.3.9/imap/mailbox.c
--- cyrus-imapd-2.3.9.dist/imap/mailbox.c	2007-07-20 14:21:57.000000000 +0000
+++ cyrus-imapd-2.3.9/imap/mailbox.c	2007-09-16 09:01:24.869639877 +0000
@@ -1283,7 +1283,7 @@
  */
 int mailbox_write_index_header(struct mailbox *mailbox)
 {
-    char buf[INDEX_HEADER_SIZE];
+    ALIGNBUF(buf, INDEX_HEADER_SIZE);
     unsigned long header_size = sizeof(buf);
     int n;
     
@@ -1388,7 +1388,7 @@
 			   int sync)
 {
     int n;
-    char buf[INDEX_RECORD_SIZE];
+    ALIGNBUF(buf, INDEX_RECORD_SIZE);
 
     mailbox_index_record_to_buf(record, buf);
 
@@ -1522,8 +1522,8 @@
     unsigned long exists;
     unsigned msgno;
     bit32 oldstart_offset, oldrecord_size, recsize_diff;
-    char buf[INDEX_HEADER_SIZE > INDEX_RECORD_SIZE ?
-	     INDEX_HEADER_SIZE : INDEX_RECORD_SIZE];
+    ALIGNBUF(buf, INDEX_HEADER_SIZE > INDEX_RECORD_SIZE ?
+	     INDEX_HEADER_SIZE : INDEX_RECORD_SIZE);
     char *bufp;
     int quota_offset = 0;
     int calculate_flagcounts = 0;
@@ -1851,8 +1851,8 @@
 			   mailbox_decideproc_t *decideproc, void *deciderock,
 			   int expunge_flags)
 {
-    char buf[INDEX_HEADER_SIZE > INDEX_RECORD_SIZE ?
-	     INDEX_HEADER_SIZE : INDEX_RECORD_SIZE];
+    ALIGNBUF(buf, INDEX_HEADER_SIZE > INDEX_RECORD_SIZE ?
+	     INDEX_HEADER_SIZE : INDEX_RECORD_SIZE);
     unsigned msgno;
     unsigned newexpunged;
     unsigned newexists;
@@ -2034,8 +2034,8 @@
     unsigned newanswered;
     unsigned newflagged; 
     
-    char buf[INDEX_HEADER_SIZE > INDEX_RECORD_SIZE ?
-	     INDEX_HEADER_SIZE : INDEX_RECORD_SIZE];
+    ALIGNBUF(buf, INDEX_HEADER_SIZE > INDEX_RECORD_SIZE ?
+	     INDEX_HEADER_SIZE : INDEX_RECORD_SIZE);
     unsigned msgno;
     struct stat sbuf;
     struct txn *tid = NULL;
diff -ur cyrus-imapd-2.3.9.dist/imap/mailbox.h cyrus-imapd-2.3.9/imap/mailbox.h
--- cyrus-imapd-2.3.9.dist/imap/mailbox.h	2006-11-30 17:11:19.000000000 +0000
+++ cyrus-imapd-2.3.9/imap/mailbox.h	2007-09-16 18:00:00.082289457 +0000
@@ -74,6 +74,23 @@
 #define MODSEQ_FMT "%lu"
 #endif
 
+#ifdef HAVE_LONG_LONG_INT
+#if 0 /* __GNUC__ */
+#define ALIGNBUF(buf, size) char buf[(size)] __attribute__ ((aligned (8)))
+#else
+#define ALIGNBUF(buf, size) \
+	struct { \
+	    union { \
+		bit64 align8; \
+		char buf[(size)]; \
+	    } u; \
+	} align8buf; \
+	char *buf = align8buf.u.buf
+#endif
+#else
+#define ALIGNBUF(buf, size) char buf[(size)]
+#endif
+
 #define MAX_MAILBOX_NAME 490
 #define MAX_MAILBOX_PATH 4096
 
diff -ur cyrus-imapd-2.3.9.dist/imap/reconstruct.c cyrus-imapd-2.3.9/imap/reconstruct.c
--- cyrus-imapd-2.3.9.dist/imap/reconstruct.c	2007-04-03 13:01:12.000000000 +0000
+++ cyrus-imapd-2.3.9/imap/reconstruct.c	2007-09-16 09:01:51.717884602 +0000
@@ -439,8 +439,8 @@
     char newfnamebuf[MAX_MAILBOX_PATH+1];
     struct stat sbuf;
 
-    char buf[((INDEX_HEADER_SIZE > INDEX_RECORD_SIZE) ?
-             INDEX_HEADER_SIZE : INDEX_RECORD_SIZE)];
+    ALIGNBUF(buf, ((INDEX_HEADER_SIZE > INDEX_RECORD_SIZE) ?
+             INDEX_HEADER_SIZE : INDEX_RECORD_SIZE));
 
     int expunge_fd;
     FILE *fexpunge;
@@ -693,8 +693,8 @@
  */
 int reconstruct(char *name, struct discovered *found)
 {
-    char buf[((INDEX_HEADER_SIZE > INDEX_RECORD_SIZE) ?
-	     INDEX_HEADER_SIZE : INDEX_RECORD_SIZE)];
+    ALIGNBUF(buf, ((INDEX_HEADER_SIZE > INDEX_RECORD_SIZE) ?
+	     INDEX_HEADER_SIZE : INDEX_RECORD_SIZE));
     char quota_root[MAX_MAILBOX_PATH+1];
     bit32 valid_user_flags[MAX_USER_FLAGS/32];
 
diff -ur cyrus-imapd-2.3.9.dist/perl/imap/IMAP.xs cyrus-imapd-2.3.9/perl/imap/IMAP.xs
--- cyrus-imapd-2.3.9.dist/perl/imap/IMAP.xs	2006-11-30 17:11:23.000000000 +0000
+++ cyrus-imapd-2.3.9/perl/imap/IMAP.xs	2007-09-15 19:21:00.047555344 +0000
@@ -320,7 +320,7 @@
 	struct xscb *nx;
 CODE:
 /* fprintf(stderr, "!DESTROY %p %d\n", client, client->cnt); */
-	if (!--client->cnt) {
+	if (!client->cnt--) {
 /* printf("closing\n"); */
 	  imclient_close(client->imclient);
 	  while (client->cb) {