Am 06.08.2015 um 17:54 schrieb Rainer Jung:
I think the root cause is here (file subversion/libsvn_ra_svn/marshal.c):

   1082        /* Allocate an APR array with room for (initially) 4 items.
   1083         * We do this manually because lists are the most
frequent protocol
   1084         * element, often used to frame a single, optional value.
  We save
   1085         * about 20% of total protocol handling time. */
   1086        char *buffer = apr_palloc(pool, sizeof(apr_array_header_t)
   1087                                        + 4 *
sizeof(svn_ra_svn_item_t));
   1088        svn_ra_svn_item_t *data
   1089          = (svn_ra_svn_item_t *)(buffer +
sizeof(apr_array_header_t));
   1090
   1091        item->kind = SVN_RA_SVN_LIST;
   1092        item->u.list = (apr_array_header_t *)buffer;

"buffer" is not specifically aligned, the array members in "item->u.list
= (apr_array_header_t *)buffer" could be misaligned.

The following (ugly) workaround fixes it for me:

--- subversion/libsvn_ra_svn/marshal.c.kpdt_orig        Fri Feb 13
12:17:40 2015
+++ subversion/libsvn_ra_svn/marshal.c  Thu Aug  6 17:46:58 2015
@@ -1083,10 +1083,16 @@
         * We do this manually because lists are the most frequent protocol
         * element, often used to frame a single, optional value.  We save
         * about 20% of total protocol handling time. */
-      char *buffer = apr_palloc(pool, sizeof(apr_array_header_t)
+
+      /* Make sure the data part of the buffer has appropriate alignment
+         by prefixing it with a size that fits the needed
apr_array_header_t
+         but is itself highly aligned. */
+      size_t offset = sizeof(apr_array_header_t) / 8 * 8;
+
+      char *buffer = apr_palloc(pool, offset
                                        + 4 * sizeof(svn_ra_svn_item_t));
        svn_ra_svn_item_t *data
-        = (svn_ra_svn_item_t *)(buffer + sizeof(apr_array_header_t));
+        = (svn_ra_svn_item_t *)(buffer + offset);

        item->kind = SVN_RA_SVN_LIST;
        item->u.list = (apr_array_header_t *)buffer;

But of course its a bit rough, because it would apply on all platforms,
even if not needed. Also on some (future?) platforms, the alignment for
8 bytes might not always be correct.

It's a bit tragic that this code part is prefixed with:

* We do this manually because lists are the most frequent protocol
* element, often used to frame a single, optional value.  We save
* about 20% of total protocol handling time. */

and the trap is that doing it manually often is harder than expected.
Switching to apr_array_make() would have not introduced this bug, but of
course you did it for a reason.

The switch was introduced in r1485851. It is not part of 1.7 or 1.8.

Regards,

Rainer

Reply via email to