subversion tests fail on ppc64el when built with -O3

the failing test is

START: random-test
PASS: lt-random-test 1: random delta test
svn_tests: E200006: mismatch at position 75752
FAIL: lt-random-test 2: random combine delta test
END: random-test
ELAPSED: random-test 0:00:00.134787

libsvn_delta/text_delta.c is the problem file causing failure of random-test. I haven't yet tracked down which function is being miscompiled at -O3 (or possibly contains a source error exposed by -O3).

Compiling libsvn_delta/text_delta.c with -O2 -ftree-loop-vectorize -fvect-cost-model=dynamic also causes failure (ie. those two options are the specific ones added by -O3 that cause failure). The problem is this code:

/* Copy LEN bytes from SOURCE to TARGET. Unlike memmove() or memcpy(),
 * create repeating patterns if the source and target ranges overlap.
 * Return a pointer to the first byte after the copied target range. */
static APR_INLINE char *
patterning_copy(char *target, const char *source, apr_size_t len)
{
  const char *end = source + len;

  /* On many machines, we can do "chunky" copies. */

#if SVN_UNALIGNED_ACCESS_IS_OK

  if (end + sizeof(apr_uint32_t) <= target)
    {
      /* Source and target are at least 4 bytes apart, so we can copy in
       * 4-byte chunks. */
      for (; source + sizeof(apr_uint32_t) <= end;
           source += sizeof(apr_uint32_t),
           target += sizeof(apr_uint32_t))
      *(apr_uint32_t *)(target) = *(apr_uint32_t *)(source);
    }

#endif

  /* fall through to byte-wise copy (either for the below-chunk-size tail
   * or the whole copy) */
  for (; source != end; source++)
    *(target++) = *source;

  return target;
}

Specifically the code inside #if SVN_UNALIGNED_ACCESS_IS_OK. That loop can't be done in any chunks bigger than the difference between "target" and "end", but it seems the loop vectorizer is not careful enough to handle overlapping target and source in this case.

Huh, looking at that code again, don't we have a bug in the svn source code? Seems to me that "if (target + sizeof(apr_uint32_t) <= source)" is the correct condition, not "if (end + sizeof(apr_uint32_t) <= target)"

I meant
if (source + sizeof(apr_uint32_t) <= target)

We still have the vectorizer bug though, even after fixing the condition..

Agree that the code is wrong, and the vectorizer appears to be out of line. Out of curiosity, does anything change if the "const" is taken off of const char* source? It shouldn't make a difference without __restrict__, but I'm grasping at straws for why the vectorizer thinks source and target can be dealiased. In any case there shouldn't be anything target-specific about this bug, so opening a GCC tree-optimization bug is called for... If you toss me a reduced test case I can also take a look at the vect-details dump to narrow it down a bit, but there have been a lot of changes to the vectorizer since I last poked my nose in there. (-fvect-cost-model=dynamic didn't even exist, so I don't know what that is.)

No, const doesn't make any difference.

On looking again at the source, I've come to the conclusion that this isn't a vectorizer bug after all. It is simply a coding error.

A compiler is allowed to assume that a pointer to a type is aligned suitably for the type. So in
 *(apr_uint32_t *)target = *(apr_uint32_t *)source;
gcc is allowed to assume that source and target are 4-byte aligned, but they are not..
Modifying the code to use an unaligned 4-byte type here cures the problem.

Quoting from ISO/IEC 9899:1999
6.3.2.3 Pointers
...
7 A pointer to an object or incomplete type may be converted to a pointer to a different
 object or incomplete type. If the resulting pointer is not correctly aligned for the
pointed-to type, the behavior is undefined.

Problem reported upstream
http://mail-archives.apache.org/mod_mbox/subversion-dev/201408.mbox/ajax/%3C20140809135509.GC7047%40bubble.grove.modra.org%3E

The bug described in comment #5 has already been fixed upstream

The attachment "patch for subversion-1.8.9" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

This bug was fixed in the package subversion - 1.8.10-1ubuntu1

---------------
subversion (1.8.10-1ubuntu1) utopic; urgency=medium

  * Merge with Debian; remaining changes:
    - debian/rules: Manually create the doxygen output directory, otherwise
      we get weird build failures when running parallel builds.
    - Build a python-subversion-dbg package.
    - Build-depend on python-dbg.
    - Build-depend on default-jre-headless/-jdk.
    - only build on requested python versions (X-Python-Versions:)
    - Do not apply java-build patch.
    - debian/patches/verbose-tests.diff: make tests verbose
    - debian/control: added ruby-test-unit to Build-Depends
    - Check for libtoolize instead of libtool, which is not used for
      the build.
  * debian/patches/lp1353142.patch: fix alignment issue causing test
    failures on ppc64el in subversion/libsvn_delta/text_delta.c.
    (LP: #1353142)

subversion (1.8.10-1) unstable; urgency=medium

  * New upstream release. Refresh patches.
    - Includes security fixes:
      + CVE-2014-3522: ra_serf improper validation of wildcards in SSL certs.
      + CVE-2014-3528: credentials cached with svn may be sent to wrong
        server.
  * debian/rules: Avoid an unnecessary call to dpkg-buildflags.
  * debian/control: Pre-Depend on ${misc:Pre-Depends} instead of hard-coding
    multiarch-support, as suggested by Lintian.
 -- Marc Deslauriers <email address hidden> Thu, 14 Aug 2014 14:39:07 -0400