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

RESOLVED DUPLICATE of bug 1056337

Status

()

RESOLVED DUPLICATE of bug 1056337
4 years ago
4 years ago

People

(Reporter: seinlin, Assigned: seinlin)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 8528890 [details] [diff] [review]
bug-1104663-libopus.patch

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+
(Assignee)

Comment 3

4 years ago
Created attachment 8529492 [details] [diff] [review]
bug-1104663.patch

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)
(Assignee)

Comment 5

4 years ago
Created attachment 8529499 [details] [diff] [review]
bug-1104663.patch

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-

Comment 9

4 years ago
Kai-Zhen, you are working on this bug, so...
Assignee: nobody → kli
(Assignee)

Comment 10

4 years ago
This issue get fixed in bug 1056337.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1056337
You need to log in before you can comment on or make changes to this bug.