Last Comment Bug 666672 - media/ - compiler warnings on mac
: media/ - compiler warnings on mac
Status: RESOLVED FIXED
[build_warning]
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Atul Aggarwal
:
: Maire Reavy [:mreavy]
Mentors:
: 458725 (view as bug list)
Depends on:
Blocks: buildwarning 683823
  Show dependency treegraph
 
Reported: 2011-06-23 11:22 PDT by Joey Armstrong [:joey]
Modified: 2011-09-26 07:45 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (14.04 KB, patch)
2011-08-30 14:19 PDT, Atul Aggarwal
no flags Details | Diff | Splinter Review
Patch v2 (14.39 KB, patch)
2011-08-30 16:27 PDT, Atul Aggarwal
no flags Details | Diff | Splinter Review
Patch v3 (31.97 KB, patch)
2011-08-31 01:27 PDT, Atul Aggarwal
no flags Details | Diff | Splinter Review
Patch v4 (31.94 KB, patch)
2011-08-31 14:28 PDT, Atul Aggarwal
no flags Details | Diff | Splinter Review
Patch v5 (18.95 KB, patch)
2011-08-31 22:53 PDT, Atul Aggarwal
giles: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2011-06-23 11:22:54 PDT
% 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
Comment 1 Atul Aggarwal 2011-08-30 14:19:00 PDT
Created attachment 556993 [details] [diff] [review]
Patch v1

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.
Comment 2 Ralph Giles (:rillian) needinfo me 2011-08-30 15:25:50 PDT
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.
Comment 3 Atul Aggarwal 2011-08-30 16:27:37 PDT
Created attachment 557030 [details] [diff] [review]
Patch v2

Accommodating the changes done upstream to fix the warning.
Comment 4 Ralph Giles (:rillian) needinfo me 2011-08-30 21:34:22 PDT
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 5 cajbir (:cajbir) 2011-08-30 21:53:15 PDT
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.
Comment 6 Atul Aggarwal 2011-08-30 22:31:41 PDT
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.
Comment 7 Ralph Giles (:rillian) needinfo me 2011-08-30 22:34:39 PDT
Ah, you should have said it was untested; I could have tried it. Thanks, Chris, for catching the spurious dereferences.
Comment 8 Atul Aggarwal 2011-08-31 01:27:51 PDT
Created attachment 557100 [details] [diff] [review]
Patch v3

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?).
Comment 9 Atul Aggarwal 2011-08-31 14:28:52 PDT
Created attachment 557325 [details] [diff] [review]
Patch v4

Removed some whitespaces.
Comment 10 Ralph Giles (:rillian) needinfo me 2011-08-31 14:53:07 PDT
>    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 11 Ralph Giles (:rillian) needinfo me 2011-08-31 15:23:45 PDT
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'.
Comment 12 cajbir (:cajbir) 2011-08-31 21:54:49 PDT
(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.
Comment 13 Matthew Gregan [:kinetik] 2011-08-31 22:05:35 PDT
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.
Comment 14 Ralph Giles (:rillian) needinfo me 2011-08-31 22:13:03 PDT
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.
Comment 15 Ralph Giles (:rillian) needinfo me 2011-08-31 22:43:21 PDT
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.
Comment 16 Atul Aggarwal 2011-08-31 22:53:46 PDT
Created attachment 557413 [details] [diff] [review]
Patch v5
Comment 17 Ralph Giles (:rillian) needinfo me 2011-08-31 23:22:02 PDT
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.
Comment 19 Atul Aggarwal 2011-09-01 10:33:26 PDT
*** Bug 458725 has been marked as a duplicate of this bug. ***
Comment 20 Ralph Giles (:rillian) needinfo me 2011-09-01 13:04:42 PDT
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.
Comment 21 cajbir (:cajbir) 2011-09-01 22:28:42 PDT
Oops, you're right. New try server attempt: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ad4b478ce996
Comment 22 Atul Aggarwal 2011-09-04 11:45:14 PDT
Are test results look okay?
Comment 23 Ralph Giles (:rillian) needinfo me 2011-09-07 15:44:07 PDT
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?
Comment 24 Ralph Giles (:rillian) needinfo me 2011-09-08 12:54:39 PDT
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.
Comment 25 Dão Gottwald [:dao] 2011-09-08 13:23:14 PDT
There's an outstanding review?(chris.double).
Comment 26 Atul Aggarwal 2011-09-25 11:36:09 PDT
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 27 cajbir (:cajbir) 2011-09-25 16:16:37 PDT
Comment on attachment 557413 [details] [diff] [review]
Patch v5

If giles says it's fine, then that's review enough.
Comment 28 Ralph Giles (:rillian) needinfo me 2011-09-25 21:07:15 PDT
Thanks, Chris.
Comment 29 Matthew Gregan [:kinetik] 2011-09-25 21:18:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f61c120e00f2

Note You need to log in before you can comment on or make changes to this bug.