Allow PNaCl NMF to set translator optimization options for experimentation.

Allow PNaCl NMF to set translator optimization options for experimentation.

This adds two fields:
(1) "-O": i
(2) "experimentation_flags": "arbitrary flags"

We may want to remove (2) soon... or just omit it.
The optimization flag is separated and made a uint so that
it can be tracked as a UMA stat more easily.

Also set default to -O0 for now.



TEST= browser_tests --gtest_filter=*Pnacl*Options*

This only tests that the plugin doesn't barf on those
new NMF options. To really test it we'd need to instrument
LLC to know that it's really getting these flags and get
output from LLC confirming that.  For now I have a manual
test:
http://www/~jvoung/IrrLicht/testirrnacl_portable.html
showing that it works though.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=3325

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187764

[Derek Schuff, 2013-03-08 21:34:55]

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_options.cc (right):

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:16:
PnaclOptions::PnaclOptions(const PnaclOptions& opt) {
do we really need a copy constructor? is there some reason the default
compiler-generated one wont' work?

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:37: void
PnaclOptions::set_opt_level(int8_t l) {
could we use uint8_t here or is that still against Google Style(TM)?

[jvoung (off chromium), 2013-03-08 23:21:41]

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_options.cc (right):

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:16:
PnaclOptions::PnaclOptions(const PnaclOptions& opt) {
On 2013/03/08 21:34:55, Derek Schuff wrote:
> do we really need a copy constructor? is there some reason the default
> compiler-generated one wont' work?

Yeah, I think the default works.  Removed.

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:37: void
PnaclOptions::set_opt_level(int8_t l) {
On 2013/03/08 21:34:55, Derek Schuff wrote:
> could we use uint8_t here or is that still against Google Style(TM)?

Well, I made the field a int8_t, so that I could use "-1" as "not explicitly
set" so I wanted the types to match.

What was the original Google Style rule?  There's still a blob about not using
unsigned int for loops, in case you accidentally only terminate when < 0.

I also thought about just using a plain int to avoid the problem of
string_streaming a char accidentally...

[Derek Schuff, 2013-03-08 23:37:51]

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_options.cc (right):

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:16:
PnaclOptions::PnaclOptions(const PnaclOptions& opt) {
for that matter, where are we actually copying it? can we get away with
DISALLOW_COPY_AND_ASSIGN?
On 2013/03/08 23:21:41, jvoung (cr) wrote:
> On 2013/03/08 21:34:55, Derek Schuff wrote:
> > do we really need a copy constructor? is there some reason the default
> > compiler-generated one wont' work?
> 
> Yeah, I think the default works.  Removed.

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:37: void
PnaclOptions::set_opt_level(int8_t l) {
oh, i forgot we were using -1.
int8_t is probably fine, and this check should be fine too.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege...
*4* bytes? Won't someone *please* think of the swapfile?!

[jvoung (off chromium), 2013-03-08 23:49:47]

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_options.cc (right):

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:16:
PnaclOptions::PnaclOptions(const PnaclOptions& opt) {
On 2013/03/08 23:37:51, Derek Schuff wrote:
> for that matter, where are we actually copying it? can we get away with
> DISALLOW_COPY_AND_ASSIGN?


We make a version that is a stack variable, which the json parser fills in, then
if options.translate() is true, we create a pnacl coordinator and copy the data
over to pnacl coordinator.

[Derek Schuff, 2013-03-11 16:08:24]

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_options.cc (right):

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:16:
PnaclOptions::PnaclOptions(const PnaclOptions& opt) {
I see. maybe we should add a comment somewhere in the header saying that, so if
we ever add any fields for which we need a deep copy, we'll remember
On 2013/03/08 23:49:47, jvoung (cr) wrote:
> On 2013/03/08 23:37:51, Derek Schuff wrote:
> > for that matter, where are we actually copying it? can we get away with
> > DISALLOW_COPY_AND_ASSIGN?
> 
> 
> We make a version that is a stack variable, which the json parser fills in,
then
> if options.translate() is true, we create a pnacl coordinator and copy the
data
> over to pnacl coordinator.

[jvoung (off chromium), 2013-03-11 17:29:22]

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/pnacl_options.cc (right):

https://codereview.chromium.org/12623004/diff/25001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/pnacl_options.cc:16:
PnaclOptions::PnaclOptions(const PnaclOptions& opt) {
Okay, I added a note by the private fields where people usually put the
DISABLE_COPY_AND_ASSIGN macro.

On 2013/03/11 16:08:24, Derek Schuff wrote:
> I see. maybe we should add a comment somewhere in the header saying that, so
if
> we ever add any fields for which we need a deep copy, we'll remember
> On 2013/03/08 23:49:47, jvoung (cr) wrote:
> > On 2013/03/08 23:37:51, Derek Schuff wrote:
> > > for that matter, where are we actually copying it? can we get away with
> > > DISALLOW_COPY_AND_ASSIGN?
> > 
> > 
> > We make a version that is a stack variable, which the json parser fills in,
> then
> > if options.translate() is true, we create a pnacl coordinator and copy the
> data
> > over to pnacl coordinator.
>

[Derek Schuff, 2013-03-11 17:42:32]

LGTM

[Nick Bray (chromium), 2013-03-11 22:34:08]

Quick owners LGTM, if needed.

[jvoung (off chromium), 2013-03-11 23:04:13]

Thanks Nick!  I do need to wait for a NaCl DEPS bump to pull in the new
translator though, for these tests to pass.

[commit-bot: I haz the power, 2013-03-12 22:19:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/12623004/42001

[commit-bot: I haz the power, 2013-03-13 03:24:14]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-03-13 03:42:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/12623004/42001

[commit-bot: I haz the power, 2013-03-13 03:52:10]

Change committed as 187764