unaligned access on mips/mipsel Linux with 2.6

[thomas-weber]

Hi Marti,
this is Debian bug #749975, https://bugs.debian.org/749975, which I copy in full below. The issue was found using colord's testsuite, that's why colord is mentioned.

colord fails to build from source on both mips/mipsel due to an unaligned access (SIGBUS):
https://buildd.debian.org/status/fetch.php?pkg=colord&arch=mips&ver=1.2.0-3&stamp=1401475533
https://buildd.debian.org/status/fetch.php?pkg=colord&arch=mipsel&ver=1.2.0-3&stamp=1401374635

The MIPS architecture requires that load/stores are done with an alignment corresponding to the type, so even if it is a 32-bit architecture accesses to double values (8-bytes) require a 64-bit alignment.

Currently the lcms2 memory allocator only guarantee an alignment corresponding the size of (void *). Since version 2.6, lcms2 now also store double values (called cmsFloat64Number), causing some random issues on mips/mipsel, depending on how lucky we are about the alignement. SPARC is likely to also be affected, as the same alignment requirement applies.

There are two ways of fixing that, either increasing the alignment to 64-bit on mips/mipsel/sparc:

--- a/src/lcms2_internal.h
+++ b/src/lcms2_internal.h
@@ -56,8 +56,13 @@
// Alignment of ICC file format uses 4 bytes (cmsUInt32Number)
#define _cmsALIGNLONG(x) (((x)+(sizeof(cmsUInt32Number)-1)) & ~(sizeof(cmsUInt32Number)-1))

-// Alignment to memory pointer
-#define _cmsALIGNMEM(x) (((x)+(sizeof(void *) - 1)) & ~(sizeof(void *) - 1))
+// Alignment to maximum required alignment
+// On MIPS and SPARC, access to double values need to be 8-bytes aligned
+#if defined(mips) || defined(sparc)
+# define _cmsALIGNMEM(x) ((x + 7) & ~7)
+#else
+# define _cmsALIGNMEM(x) (((x)+(sizeof(void *) - 1)) & ~(sizeof(void *) - 1))
+#endif

// Maximum encodeable values in floating point
#define MAX_ENCODEABLE_XYZ (1.0 + 32767.0/32768.0)

As it seems storing 64-bit values is not the common case, the otheralternative is to tell GCC that the stored double values are aligned to (void *), so that it generate code to do unaligned access if needed. This can be specified independently of the architecture as this isalways true, and might even help GCC to generate better code. This is what the patch below does:

--- a/src/lcms2_internal.h
+++ b/src/lcms2_internal.h
@@ -479,11 +479,13 @@
const struct _cmsContext_struct* src);

// Container for adaptation state -- not a plug-in
+// The chunks are only guaranteed to be aligned to void *, tell that to the
+// compiler so that it can generate unaligned access instructions if needed.
typedef struct {

 cmsFloat64Number  AdaptationState;

-} _cmsAdaptationStateChunkType;
+} _cmsAdaptationStateChunkType attribute ((aligned(alignof(void *))));

// The global Context0 storage for adaptation state
extern _cmsAdaptationStateChunkType _cmsAdaptationStateChunk;

[mmoll]

@thomas-weber could you open a PR for this patch, and ideally also for the patch in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=644473 ? We plan to pull that into FreeBSD soon, as it helps also there: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=201226

[mm2]

Michael,

Thanks for reporting. I have uploaded a fixed version to the GIT server, hope that will address the problem. If I understood correctly, the compiler does not align strings at all, so accessing the begin of string as an array of uint16 makes it fail.

• Actually lcms2 is really bad with regards memory alignment in general.

Please report all issues to upstream (me). I will try to fix them ASAP. Until now I was not aware of any alignment problems.

Regards

Marti Maria

The LittleCMS project

http://www.littlecms.com

From: Michael Moll [mailto:[email protected]]
Sent: sábado, 28 de noviembre de 2015 14:43
To: mm2/Little-CMS [email protected]
Subject: Re: [Little-CMS] unaligned access on mips/mipsel Linux with 2.6 (#32)

@thomas-weber https://github.com/thomas-weber could you open a PR for this patch, and ideally also for the patch in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=644473 ? We plan to pull that into FreeBSD soon, as it helps also there: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=201226

—
Reply to this email directly or view it on GitHub #32 (comment) . https://github.com/notifications/beacon/AAMLwuQogVPj09FSgeJB9CljjNJDkgqAks5pKabqgaJpZM4CADym.gif

[mmoll]

Thank you very much, it seems to me that the patch for https://bugs.debian.org/644473 is in master now. Is the issue in https://bugs.debian.org/749975 also fixed with these changes?

When patching FreeBSD ports I try to get people also to open PRs to upstream projects, but it's not always easy...

[thomas-weber]

Hi Michael,
sorry for not following up on this sooner, but I am mostly MIA these days ...
if I understand the code correctly, the new preprocessor variable "CMS_PTR_ALIGNMENT" can be used to force an alignment according to the current platform. Thus, https://bugs.debian.org/749975 should be fixed in current git, as well as https://bugs.debian.org/644473 (the latter is fixed by 40c6403).

Marti, I will try to contact the original bug reporter in Debian about further memory alignment issues. He is far more knowledgeable than me about this.