Closed Bug 1104663 Opened 10 years ago Closed 9 years ago

[gonk-l] libopus get compile error with -O3

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1056337

People

(Reporter: seinlin, Assigned: seinlin)

References

Details

Attachments

(2 files, 1 obsolete file)

      No description provided.
Hi Chris, Could you have a look to this patch? Thanks!
Attachment #8528890 - Flags: feedback?(cpeterson)
Comment on attachment 8528890 [details] [diff] [review]
bug-1104663-libopus.patch

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

::: media/libopus/moz.build
@@ +91,2 @@
>      if CONFIG['MOZ_OPTIMIZE']:
> +      if CONFIG['ANDROID_VERSION'] >= '21':

Is this gcc bug fixed in recent gcc versions? Can you can add a gcc version check so we don't have to deoptimize libopus forever? :)

@@ +91,3 @@
>      if CONFIG['MOZ_OPTIMIZE']:
> +      if CONFIG['ANDROID_VERSION'] >= '21':
> +        # -O3 hit gcc bug in gonk-l, fallback to use -Os

The comment should probably include a link to a bug report of the gcc bug.

@@ +95,5 @@
> +          '-Os',
> +        ]
> +        CXXFLAGS += [
> +          '-Os',
> +        ]

Is the -Os workaround needed for both CFLAGS and CXXFLAGS?
Attachment #8528890 - Flags: feedback?(cpeterson) → feedback+
Attached patch bug-1104663.patch (obsolete) — Splinter Review
Hi Chris, 

Could you review this patch? Thanks!

I checked and verified that we do not need to add Os in this makefile as it will be passed from above level.
Attachment #8529492 - Flags: review?(cpeterson)
Update the comment.
Attachment #8529492 - Attachment is obsolete: true
Attachment #8529492 - Flags: review?(cpeterson)
Attachment #8529499 - Flags: review?(cpeterson)
Please check that this issue isn't fixed by bug 1056337.
Depends on: 1056337
Comment on attachment 8529499 [details] [diff] [review]
bug-1104663.patch

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

Looks OK to me, but I am not a reviewer for this component. Ralph?

Since this is a gcc bug, it would be nice if we could check the compiler version < 4.8.2 instead of just Android API Level. (The gcc bug report suggests the bug was fixed in upstream gcc 4.8.2). When we start building with a future Android NDK's fixed gcc, then we will automatically benefit from -O3 without having to remember to revert this libopus change. :)
Attachment #8529499 - Flags: review?(giles)
Attachment #8529499 - Flags: review?(cpeterson)
Attachment #8529499 - Flags: feedback+
Comment on attachment 8529499 [details] [diff] [review]
bug-1104663.patch

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

Pretty much what cpeterson said. If the issue is a gcc internal compiler error, the issue is the the toolchain version, not the android version, so it would be better to check for that.

Also, per comment #6, have you checked that the patch from bug 1056337 does not address this problem?
Attachment #8529499 - Flags: review?(giles) → review-
Kai-Zhen, you are working on this bug, so...
Assignee: nobody → kli
This issue get fixed in bug 1056337.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: