The default bug view has changed. See this FAQ.

media/ - compiler warnings on mac

RESOLVED FIXED in mozilla9

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: joey, Assigned: Atul Aggarwal)

Tracking

(Blocks: 1 bug)

unspecified
mozilla9
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Whiteboard: [build_warnings]
Whiteboard: [build_warnings] → [build_warning]
Blocks: 187528
(Assignee)

Comment 1

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

Comment 3

6 years ago
Created attachment 557030 [details] [diff] [review]
Patch v2

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 5

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

Comment 6

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

Comment 8

6 years ago
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?).
Attachment #557030 - Attachment is obsolete: true
Attachment #557100 - Flags: review?(giles)
Attachment #557100 - Flags: review?(chris.double)
Attachment #557030 - Flags: review?(giles)
(Assignee)

Comment 9

6 years ago
Created attachment 557325 [details] [diff] [review]
Patch v4

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'.

Comment 12

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

Comment 16

6 years ago
Created attachment 557413 [details] [diff] [review]
Patch v5
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

Comment 18

6 years ago
Build bustage on Mac OS X from try server: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=001b40a7bee3
http://tbpl.allizom.org/php/getParsedLog.php?id=6224676#error0
(Assignee)

Updated

6 years ago
Duplicate of this bug: 458725
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

6 years ago
Oops, you're right. New try server attempt: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ad4b478ce996
(Assignee)

Comment 22

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

Comment 26

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

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

Updated

6 years ago
Keywords: checkin-needed
Thanks, Chris.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f61c120e00f2
Keywords: checkin-needed
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/f61c120e00f2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.