615615 - Android compiler flag review

Status: VERIFIED FIXED

(Tamarin Graveyard :: Build Config, defect, P3)

Description

[Brent Baker]

Need to review the compiler flags that are set in the xplatform compile script for android against what the player is currently using. The switches that are currently being used are from an old Makefile that was in use when android development started up and has not been kept up to date.

We should at least ensure that we are setting the same level of optimization and the warnings.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

just so its not missed: please also whether (and how to) propagate the commented declarations from Bug 609809, comment 6.

Comment 2

[Trevor Baker]

Isn't this done?

Comment 3

[James Sudduth]

No, this still needs to be done.

Comment 4

[James Sudduth]

We need to get some dev eyes on this. There is a wiki page/spreadsheet here:

https://zerowing.corp.adobe.com/display/FlashPlayer/Android+Compiler+Flag+Review

listing every -m, -f, -D, -O, -W and -Wl flag used by both the Flash team's and our make file(s).

As a first pass, as stated in the Description field above, we should make sure we're using the same levels of optimizations and warnings. But after comparing the avm shell make file with the Flash team's make files it appears there are some possibly significant differences in -f and -D flags that should be accounted for. Also, both our x-compile generated make file and Flash's make files were adapted from an older make file. The working assumption was that it was mostly correct (because the resulting shell passed all tests) but correctness hasn't been verified on a per-flag basis. 

Please update the spreadsheet as needed (use IE or Firefox 3.x - 4.x doesn't currently work). If you can't edit the spreadsheet in Confluence please download then reattach.

Comment 5

[James Sudduth]

To make collaborative editing easier it's been suggested that I post the spreadsheet on Google Docs. A group of devs and managers will be receiving an invitation from me soon. If you don't receive an invitation and would like to assist in this effort please ping me and I'll be happy to add you.

Comment 6

[James Sudduth]

You might receive two invitations for this. Use the one from [e-mail].

Comment 7

[James Sudduth]

Because of problems with Adobe rejecting the invitations from [e-mail] I re-posted the updated spreadsheet at my other account and sent new invitations. If you can find time please take a look at the spreadsheet and use comments in the cells for annotations.

In answer to a query about the current settings for each flag: the 'X's in the Flash, AVM and Needs columns are only meant to indicate whether the Flash or AVM build uses a particular flag and if AVM needs to use it also. Current settings are given in the Notes column, although not for every flag.

Comment 8

[James Sudduth]

Attachment: Android compiler flag review.

This patch brings us to within about a 98% match with what the Flash team uses to build their avm shell. It passed a sandbox run.

Comment 9

[Brent Baker]

Comment on attachment 531185 [details] [diff] [review]
Android compiler flag review.

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

::: configure.py
@@ +149,5 @@
>  MMGC_CPPFLAGS = "-DAVMSHELL_BUILD "
>  AVMSHELL_CPPFLAGS = ""
>  AVMSHELL_LDFLAGS = ""
> +#MMGC_DEFINES = {'SOFT_ASSERTS': None}
> +MMGC_DEFINES = {}

Please remove the commented out line. I did a little research into the SOFT_ASSETS, since I didn't know what it was, and it was a debugging feature in MMgc that was removed back in 11/2007

http://hg.mozilla.org/tamarin-redux/rev/ab93ffeb211f

Comment 10

[Brent Baker]

Comment on attachment 531185 [details] [diff] [review]
Android compiler flag review.

Actually changing to review-, can you explain why we are still setting preprocessor values that are NOT used in the avm/MMgc/nanojit at all?

For example in the tamarin configure script we are setting these values:
-DSDK_ON2_OPT -DSDK_ON2_OPT_ARM11 -DFP_ON2_USE_C_FILTERING_FUNCTIONS

These surely mean absolutely nothing to any of the code in tamarin.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 531185 [details] [diff] [review]
Android compiler flag review.

(removing self from review; I have no comment beyond what eagle-eyed Brent already caught.)

Comment 12

[James Sudduth]

(In reply to comment #10)
> Comment on attachment 531185 [details] [diff] [review] [review]
> Android compiler flag review.
> 
> Actually changing to review-, can you explain why we are still setting
> preprocessor values that are NOT used in the avm/MMgc/nanojit at all?
> 
> For example in the tamarin configure script we are setting these values:
> -DSDK_ON2_OPT -DSDK_ON2_OPT_ARM11 -DFP_ON2_USE_C_FILTERING_FUNCTIONS
> 
> These surely mean absolutely nothing to any of the code in tamarin.

Arrgghh - good catch. I'll pull those out.

Comment 13

[James Sudduth]

Attachment: Remove unneeded -D options.

Comment 14

[Brent Baker]

Comment on attachment 531499 [details] [diff] [review]
Remove unneeded -D options.

My earlier comment (comment #10) was just citing unused options that I was able to quickly see and was not meant as a definitive list of unnecessary list of -D args that are currently still being used.... Took some time to go through the remainder of the -D args:

I couldn't find the google doc that was tracking this so I am only basing this off of the wiki link from comment #4

NO_SYS_SIGNAL: looks like the player is not setting this so why is the shell? Not even sure what it is for, not in the tamarin source.

NO_CONSOLE_FWRITE: looks like the player is not setting this so why is the shell? Not even sure what it is for, not in the tamarin source.

RTMFPUTIL_OVERRIDE_OPERATOR_NEW: RTMP is not in the shell

AVMFEATURE_OVERRIDE_GLOBAL_NEW: this could be an old artifact, we are setting this to 0, but that is the default value, this really isn't needed.

DISABLE_DRM: used in player but not in tamarin
GENERIC_PLATFORM: used in player but not in tamarin
NEEDS_IN6_H: used in player but not in tamarin

Why do we have this in the configure.py? This should be removed.

    if DISABLE_RTMPE:
        BASE_D_FLAGS += "-DDISABLE_RTMPE "

Comment 15

[Brent Baker]

Seems like one other thing that must be resolved here is the -mfpu setting.

Comment 16

[Brent Baker]

(In reply to comment #15)
> Seems like one other thing that must be resolved here is the -mfpu setting.

Looks like this was resolved in the spreadsheet. Should we be compiling and testing with both neon and vfpv3-d16? At least neon in the main part of the build and then vfpv3-d16 in deep?

Comment 17

[James Sudduth]

(In reply to comment #14)
> Comment on attachment 531499 [details] [diff] [review] [review]
> Remove unneeded -D options.
> 
> My earlier comment (comment #10) was just citing unused options that I was
> able to quickly see and was not meant as a definitive list of unnecessary
> list of -D args that are currently still being used.... Took some time to go
> through the remainder of the -D args:
> 
> I couldn't find the google doc that was tracking this so I am only basing
> this off of the wiki link from comment #4
> 
> NO_SYS_SIGNAL: looks like the player is not setting this so why is the
> shell? Not even sure what it is for, not in the tamarin source.
> 
> NO_CONSOLE_FWRITE: looks like the player is not setting this so why is the
> shell? Not even sure what it is for, not in the tamarin source.
> 
> RTMFPUTIL_OVERRIDE_OPERATOR_NEW: RTMP is not in the shell
> 
> AVMFEATURE_OVERRIDE_GLOBAL_NEW: this could be an old artifact, we are
> setting this to 0, but that is the default value, this really isn't needed.
> 
> DISABLE_DRM: used in player but not in tamarin
> GENERIC_PLATFORM: used in player but not in tamarin
> NEEDS_IN6_H: used in player but not in tamarin
> 
> Why do we have this in the configure.py? This should be removed.
> 
>     if DISABLE_RTMPE:
>         BASE_D_FLAGS += "-DDISABLE_RTMPE "
All of these options were left in out of a (perhaps hyper) sense of caution because I got no response in the compiler review spreadsheet about what they do and/or could find little to no helpful info on them. I'm happy to remove them if there are no objections.

Comment 18

[James Sudduth]

(In reply to comment #16)
> (In reply to comment #15)
> > Seems like one other thing that must be resolved here is the -mfpu setting.
> 
> Looks like this was resolved in the spreadsheet. Should we be compiling and
> testing with both neon and vfpv3-d16? At least neon in the main part of the
> build and then vfpv3-d16 in deep?

Here are the responses from Kaiwalya Kher and Rupen Chanda about this:
Kaiwalya: "Because of the requirement to have one universal binary on the Android Market place we are forced to go the vfpv3 route for most of the code. We have some files compiled twice once with neon and once without. We also have some code having alternate neon assembly implementations. We then make the decision at runtime."

Rupen:
"you should define neon, unless target doesn't support neon like tegra."

Judging from these comments we might be able to find problems with vfpv3 before the Flash team runs into them. Do we want to always run deep with a different setting? Perhaps we could somehow alternate deep runs between neon and vfpv3?

Comment 19

[James Sudduth]

Attachment: Remove more unneeded -D options.

Comment 20

[Tamarin Bot]

changeset: 6319:332fb0b03435
user:      James Sudduth <[e-mail]>
summary:   Bug 615615 - Android compiler flag review (r=brbaker)

http://hg.mozilla.org/tamarin-redux/rev/332fb0b03435