692922 - QCMS looks for HAS_POSIX_MEMALIGN s/b HAVE_POSIX_MEMALIGN

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Steve Snyder]

There is code in QCMS that is #ifdef'd to symbol HAS_POSIX_MEMALIGN.  This symbol isn't defined anywhere.

Maybe someone overlooked defining this symbol.  More likely is that the intent was to use HAVE_POSIX_MEMALIGN, which actually is defined and used elsewhere.

In any case, that call to posix_memalign() will never be enabled with the #ifdef in its current state.

Comment 1

[Benoit Girard (:BenWa)]

Thanks for the report. We should fix this. Willing to take a patch or mentor someone.

The problem is under this file:
http://mxr.mozilla.org/mozilla-central/source/gfx/qcms/transform.c#818

Comment 2

[Steve Snyder]

Attachment: Fix use of posix_memalign() when appropriate

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 565665 [details] [diff] [review]
Fix use of posix_memalign() when appropriate

>--- mozilla8/gfx/qcms/transform.c.original	2011-10-06 20:59:02.000000000 -0400
>+++ mozilla8/gfx/qcms/transform.c	2011-10-07 18:04:34.146821044 -0400
>@@ -27,10 +27,11 @@
> #include <string.h> //memcpy
> #include "qcmsint.h"
> #include "chain.h"
> #include "matrix.h"
> #include "transform_util.h"
>+#include "mozilla-config.h"

I'd rather this be included by the build system instead of directly into transform.c

>-#ifdef HAS_POSIX_MEMALIGN
>+#ifdef HAVE_POSIX_MEMALIGN
> static qcms_transform *transform_alloc(void)
> {
> 	qcms_transform *t;
>-	if (!posix_memalign(&t, 16, sizeof(*t))) {
>+	if (!posix_memalign((void **)&t, 16, sizeof(*t))) {
>+		memset(t, 0, sizeof(*t));

I'd rather we do something like:

void *allocated_memory;
posix_memalign(&allocated_memory,...)
t = allocated_memory

This lets us avoid the cast.

Comment 4

[Steve Snyder]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> I'd rather this be included by the build system instead of directly into
> transform.c

That's why I didn't submit a patch in the original report.  It's a judgement call as to where this symbol is defined and how to communicate its presence to the intended source file(s).
 
> I'd rather we do something like:
> 
> void *allocated_memory;
> posix_memalign(&allocated_memory,...)
> t = allocated_memory
> 
> This lets us avoid the cast.

Sounds reasonable to me.  I was trying to keep changes to a minimum to increase the chances of the patch being approved and applied.

I can modify the patch to avoid the casting, but I'm not sure how to get the build system to put HAVE_POSIX_MEMALIGN on the compiler command line.

Comment 5

[Steve Snyder]

One more thing.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> I'd rather we do something like:
> 
> void *allocated_memory;
> posix_memalign(&allocated_memory,...)
> t = allocated_memory
> 
> This lets us avoid the cast.

Your proposed change doesn't actually eliminate a cast, it just moves it from posix_memalign() to return().  Given the function's return type the last line in the function would end up being:

return (qcms_transform *)allocated_memory;

That's probably cleaner (as the patch probably should have put that "void *" cast on the memset() call as well), but it doesn't leave the function cast-free.

Comment 6

[Benoit Girard (:BenWa)]

Sorry Steve for the late response. I'm going to ping Jeff for a response for the second comment. The first problem should be fixed however before checking in.

Comment 7

[Steve Snyder]

Could someone please tear themselves away from the SocialAPI long enough to apply this 4-line fix (or equivalent) to an obvious bug?

Thanks.

Comment 8

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 9

[Steve Snyder]

(In reply to Mike Hoye [:mhoye] from comment #8)
> Can you confirm that you're still working on this bug?

I can confirm that I am *not* working on it.

Beyond the creation of the patch 18 months ago, I don't know what there is for me to do.  The last meaningful comment is comment #3 in which Jeff expresses the desire to have the build system provide the HAVE_POSIX_MEMALIGN def instead of including mozilla-config.h.  I don't know how to coax that behavior out of the build system.

Comment 10

[Steve Snyder]

(In reply to Benoit Girard (:BenWa) from comment #6)
> Sorry Steve for the late response. I'm going to ping Jeff for a response for
> the second comment. The first problem should be fixed however before
> checking in.

Twenty-nine months and counting.  This must set some sort of record for ping RTT.

Could someone please pull an intern off Social API to fix this single #ifdef?

Comment 11

[Shashank VRSN Sabniveesu]

Benoit Girard (:BenWa),

I have looked into the patch sublitted earlier by @Steve Snyder. I would work on this but share the same apprehension as @Steve Snyder --

"I'm not sure how to get the build system to put HAVE_POSIX_MEMALIGN on the compiler command line."

Are you willing to direct me or point me to someone else?

Comment 12

[Mike Hoye [:mhoye]]

Build system questions? Mr. Szorc, I choo-choo-choose you!

Comment 13

[Benoit Girard (:BenWa)]

Build command for transform.c:
clang -o transform.o -c -fvisibility=hidden -DNO_NSPR_10_SUPPORT -I/Users/bgirard/mozilla/mozilla-central/tree/gfx/qcms -I.  -I../../dist/include  -I/Users/bgirard/mozilla/mozilla-central/builds/obj-ff-64gdb/dist/include/nspr -I/Users/bgirard/mozilla/mozilla-central/builds/obj-ff-64gdb/dist/include/nss  -I/Users/bgirard/mozilla/mozilla-central/builds/obj-ff-64gdb/dist/include -I/Users/bgirard/mozilla/mozilla-central/tree/modules/zlib/src    -fPIC  -Qunused-arguments  -include ../../mozilla-config.h -DMOZILLA_CLIENT -MD -MP -MF .deps/transform.o.pp -Qunused-arguments  -Qunused-arguments -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Wsign-compare -Wno-unused -Wno-error=uninitialized -Wno-error=deprecated-declarations -std=gnu99 -fno-strict-aliasing -fno-math-errno -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fno-omit-frame-pointer  -Wno-missing-field-initializers    /Users/bgirard/mozilla/mozilla-central/tree/gfx/qcms/transform.c

Looks like mozilla-config.h is included for c files so that #include shouldn't be required.

Comment 14

[Gregory Szorc [:gps]]

The HAVE_* defines are typically provided by mozilla-config.h. And, mozilla-config.h is typically silently included as part of all compilations (via -include on the command line). Comment #13 implies this is working as expected! The underlying problem might be HAVE_POSIX_MEMALIGN not being defined in the build configuration? Is it in objdir/config.status or objdir/mozilla-config.h?

Comment 15

[Shashank VRSN Sabniveesu]

Attachment: Fix use of posix_memalign() avoiding typecasts

Comment 16

[Benoit Girard (:BenWa)]

Looks like chromium already patched this:
https://chromiumcodereview.appspot.com/10426010

Too bad they didn't upstream it :(

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 8419755 [details] [diff] [review]
Fix use of posix_memalign() avoiding typecasts

Review of attachment 8419755 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, I'll land the revised patch.

::: gfx/qcms/transform.c
@@ +929,5 @@
>  	qcms_transform *t;
> +
> +	void *allocated_memory;
> +	if (!posix_memalign(&allocated_memory, 16, sizeof(qcms_transform))) {
> +		t = allocated_memory;

Just one more thing. Notice how the non HAVE_POSIX_MEMALIGN version does a memset? The two version should be consistent here. They should either both return 0'ed memory or both not. Since this change is about fixing this we should make sure that for platforms that have memalign we don't suddenly start returning non 0'd memory. Please add a memset here and I think we're good to land the patch.

Thanks!

Comment 18

[Shashank VRSN Sabniveesu]

Attachment: Fix use of posix_memalign() avoiding typecasts with proper initialisations

Initialising memory block to all zeroes for consistency with non HAVE_POSIX_MEMALIGN verion of the code

Comment 19

[Shashank VRSN Sabniveesu]

Attachment: Fix use of posix_memalign() avoiding typecasts with proper initialisations

Corrected the previous Bug692922_2.patch

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 8419822 [details] [diff] [review]
Fix use of posix_memalign() avoiding typecasts with proper initialisations

Review of attachment 8419822 [details] [diff] [review]:
-----------------------------------------------------------------

It's fine but next time we don't need a comment to explain a memset. It would be useful however to have a comment 'memset here to be consistent with the other transform_alloc'.

Comment 21

[Benoit Girard (:BenWa)]

Attachment: patch

Fixed up the patch. Ready to land.

Comment 22

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36ba501f5585

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36ba501f5585