Closed Bug 666672 Opened 13 years ago Closed 13 years ago

media/ - compiler warnings on mac

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: joey, Assigned: atulagrwl)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(1 file, 4 obsolete files)

% uname -a
Darwin banshee.local 10.7.4 Darwin Kernel Version 10.7.4: Mon Apr 18 21:24:17 PDT 2011; root:xnu-1504.14.12~3/RELEASE_X86_64 x86_64

/mozilla/sandbox/gml/media/libvorbis/lib/vorbis_codebook.c:251: warning: suggest parentheses around + or - inside shift
/mozilla/sandbox/gml/media/libvorbis/lib/vorbis_floor1.c:1038: warning: suggest parentheses around + or - in operand of &
/mozilla/sandbox/gml/media/libvorbis/lib/vorbis_psy.c:1164:17: warning: "/*" within comment
/mozilla/sandbox/gml/media/libtheora/lib/state.c:717: warning: comparison of unsigned expression < 0 is always false
/mozilla/sandbox/gml/media/libtheora/lib/state.c:718: warning: comparison of unsigned expression < 0 is always false
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:141: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:183: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:190: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:202: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:225: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:276: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:328: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:375: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:433: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:437: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:449: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:477: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:518: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:616: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:627: warning: ISO C90 forbids mixed declarations and code
/mozilla/sandbox/gml/media/libsydneyaudio/src/sydney_audio_mac.c:664: warning: ISO C90 forbids mixed declarations and code
Whiteboard: [build_warnings]
Whiteboard: [build_warnings] → [build_warning]
Blocks: buildwarning
Attached patch Patch v1 (obsolete) — Splinter Review
Fixed all the issues except "warning: comparison of unsigned expression < 0 is always false". The reason being the check is there to ensure enum is within range by enum <0 and enum > MAX_LIMIT but gcc might have implemented enum as unsigned int and hence it is throwing warning that one check is always false but this might not be the case with other compilers and we don't want to miss an important check.
Assignee: nobody → atulagrwl
Status: NEW → ASSIGNED
One of the libvorbis issues are fixed slightly differently upstream. I suggest copying that patch to reduce noise at the next merge.

https://trac.xiph.org/changeset/17688

The code in question is #if 0'd out, so the uncommenting the related code isn't a functional change.
Attached patch Patch v2 (obsolete) — Splinter Review
Accommodating the changes done upstream to fix the warning.
Attachment #556993 - Attachment is obsolete: true
Attachment #557030 - Flags: review?(giles)
Comment on attachment 557030 [details] [diff] [review]
Patch v2

Thanks. Looks good. My only comment is that it would suit local style better if in the sydneyaudio code when you add a set of declarations at the start of a function, you follow it with a blank line, like in audio_callback().

Also, why doesn't sa_stream_get_volume_abs() warn about local_vol around sydney_audio_mac.c:677?
Comment on attachment 557030 [details] [diff] [review]
Patch v2

Drive by comments.

>diff --git a/media/libsydneyaudio/src/sydney_audio_mac.c b/media/libsydneyaudio/src/sydney_audio_mac.c
>--- a/media/libsydneyaudio/src/sydney_audio_mac.c
>+++ b/media/libsydneyaudio/src/sydney_audio_mac.c
> 
>+  sa_stream_t     * s;
...
>-  sa_stream_t     * s = arg;
>+  *s = arg;

This dereferences an uninitialized variable, s. The "*s = arg" is not the same as "sa_stream_t* s = arg".

+  *dst            = data->mBuffers[0].mData;

As above. 'dst' is uninitialized. Should be 'dst ='.

And here:

-      sa_buf  * next = s->bl_head->next;
+      *next = s->bl_head->next;

'next' is uninitialized.

>+          struct timespec ts;
>           pthread_mutex_unlock(&s->mutex);
>-          struct timespec ts = {0, 1000000};
>+          ts = {0, 1000000};

This can be:

>+          struct timespec ts = {0, 1000000};
>           pthread_mutex_unlock(&s->mutex);
>-          struct timespec ts = {0, 1000000};

There are a few places where you've moved code like this:

Foo* a = 0;

To be:

Foo* a;
...
a = 0;

Since you are assigning a constant you could just move the variable declaration and still initialize it in one.

For the vorbis changes you'll need to add a patch file, update the README_MOZILLA, and change update.sh to apply the patch.
Ralph, I don't have mac system and hence have relied upon the warning reported by the bug and fixed them. Due to this, I could not figure out the problem which Chris reported. Thanks a lot Chris for reporting this issue. I am feeling kind of stupid by committing blunder. Maybe I was half asleep when I prepared this patch. I will prepare another patch.
Ah, you should have said it was untested; I could have tried it. Thanks, Chris, for catching the spurious dereferences.
Attached patch Patch v3 (obsolete) — Splinter Review
I have tried to fix all the issue raised. Please see if there is still some problem with it.
Please note this is untested.

Also I think there is some problem in the update.sh file of libsydneyaudio. Initial lines of patches has -p4 argument which I guess needs to be corrected to -p3 (Maybe?).
Attachment #557030 - Attachment is obsolete: true
Attachment #557100 - Flags: review?(giles)
Attachment #557100 - Flags: review?(chris.double)
Attachment #557030 - Flags: review?(giles)
Attached patch Patch v4 (obsolete) — Splinter Review
Removed some whitespaces.
Attachment #557100 - Attachment is obsolete: true
Attachment #557325 - Flags: review?(giles)
Attachment #557325 - Flags: review?(chris.double)
Attachment #557100 - Flags: review?(giles)
Attachment #557100 - Flags: review?(chris.double)
>    ts = {0, 1000000};

This doesn't compile. The initializer syntax is only allowed on declarations. Despite making it harder to read, it's probably best to just move the initializer up to the declaration as well. Otherwise you need:

struct timespec ts;
...
ts.tv_sec = 0;
ts.tv_nsec = 1000000;
nanosleep(&ts, NULL);

which I don't think is any better.
Comment on attachment 557325 [details] [diff] [review]
Patch v4

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

BTW, I think you're right about the -p4 lines, but don't worry about fixing it as part of this bug. It will be obvious the next time the code is sync'd with 'upstream'.
(In reply to Atul Aggarwal from comment #8)
> Please note this is untested.

Can you push to try server to make sure this issue is fixed on Mac? I don't have a mac to test with.
There's no need to include bug666672_gccWarnings.patch for the sydneyaudio patches.  We stopped doing this some time ago, and the patches, update.sh, and readmes that remain in media/libsydneyaudio should be removed or updated to reflect the current practices.
I think it's worth maintaining the patch list while the code in tree is configured that way. If you want to remove the update mechanism, that should be an independent bug.
After further discussion, I now agree with Matthew.

Please remove the the separate patch and changes to update.sh and README_MOZILLA for libsyndeyaudio. The separate patch file and update.sh changes for libvorbis should still be included.
Attached patch Patch v5Splinter Review
Attachment #557325 - Attachment is obsolete: true
Attachment #557413 - Flags: review?(giles)
Attachment #557413 - Flags: review?(chris.double)
Attachment #557325 - Flags: review?(giles)
Attachment #557325 - Flags: review?(chris.double)
Comment on attachment 557413 [details] [diff] [review]
Patch v5

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

Compiles and plays sound on MacOS. Looks ok to me.
Attachment #557413 - Flags: review?(giles) → review+
Blocks: 683823
I'm told those links should be:

http://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=001b40a7bee3
http://tbpl.mozilla.org/php/getParsedLog.php?id=6224676#error0

Chris, are you sure you pushed v5 of the patch? That looks like the compile problem I reported against v4 in comment 10.
Oops, you're right. New try server attempt: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ad4b478ce996
Are test results look okay?
Excellent question. The red tests look spurious. I've requested respins of those and the purple.

Of the orange, most have intermittent bugs filed, and the rest look unrelated to me.

Chris? Can you offer some guidance in reading the report?
With the exception of Win64, which is still pending, all the respins I requested came back greener. I think the tryserver run shows no new issues at this point.
Keywords: checkin-needed
There's an outstanding review?(chris.double).
Keywords: checkin-needed
Chris, can you please review the fix? Fix is quite simple and it won't take long. However, if you don't have time to review the fix, can you please add somebody appropriate to review it.
Thanks
Comment on attachment 557413 [details] [diff] [review]
Patch v5

If giles says it's fine, then that's review enough.
Attachment #557413 - Flags: review?(chris.double)
Keywords: checkin-needed
Thanks, Chris.
https://hg.mozilla.org/mozilla-central/rev/f61c120e00f2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.