QCMS looks for HAS_POSIX_MEMALIGN s/b HAVE_POSIX_MEMALIGN

RESOLVED FIXED in mozilla32

Status

()

Core
GFX: Color Management
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Steve Snyder, Assigned: Shashank VRSN Sabniveesu)

Tracking

8 Branch
mozilla32
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=bgirard@mozilla.com])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
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
Whiteboard: [good first bug][mentor=bgirard@mozilla.com]
(Reporter)

Comment 2

6 years ago
Created attachment 565665 [details] [diff] [review]
Fix use of posix_memalign() when appropriate
Assignee: nobody → swsnyder
Attachment #565665 - Flags: review?(jmuizelaar)
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.
Attachment #565665 - Flags: review?(jmuizelaar) → review-
(Reporter)

Comment 4

6 years ago
(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.
(Reporter)

Comment 5

6 years ago
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.
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.
(Reporter)

Comment 7

5 years ago
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

5 years ago
Can you confirm that you're still working on this bug?
Flags: needinfo?(swsnyder)
(Reporter)

Comment 9

5 years ago
(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.
Flags: needinfo?(swsnyder)

Updated

4 years ago
Assignee: swsnyder → nobody
(Reporter)

Comment 10

4 years ago
(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?
(Assignee)

Comment 11

4 years ago
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?
Flags: needinfo?(bgirard)

Comment 12

4 years ago
Build system questions? Mr. Szorc, I choo-choo-choose you!
Flags: needinfo?(gps)
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.
Flags: needinfo?(bgirard)
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?
Flags: needinfo?(gps)
(Assignee)

Comment 15

4 years ago
Created attachment 8419755 [details] [diff] [review]
Fix use of posix_memalign() avoiding typecasts
Looks like chromium already patched this:
https://chromiumcodereview.appspot.com/10426010

Too bad they didn't upstream it :(
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!
Attachment #8419755 - Flags: review+
(Assignee)

Comment 18

4 years ago
Created attachment 8419804 [details] [diff] [review]
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
Attachment #8419755 - Attachment is obsolete: true
Attachment #8419804 - Flags: review?(bgirard)
(Assignee)

Comment 19

4 years ago
Created attachment 8419822 [details] [diff] [review]
Fix use of posix_memalign() avoiding typecasts with proper initialisations

Corrected the previous Bug692922_2.patch
Attachment #8419804 - Attachment is obsolete: true
Attachment #8419804 - Flags: review?(bgirard)
Attachment #8419822 - Flags: review?(bgirard)
Flags: needinfo?(jmuizelaar)
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'.
Attachment #8419822 - Flags: review?(bgirard) → review+
Created attachment 8419829 [details] [diff] [review]
patch

Fixed up the patch. Ready to land.
Attachment #565665 - Attachment is obsolete: true
Attachment #8419822 - Attachment is obsolete: true
Attachment #8419829 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/36ba501f5585
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Assignee)

Updated

4 years ago
Flags: needinfo?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.