[MIPS] Adding validator for MIPS architecture.

Initial patch for MIPS architecture that includes implementation of the validator, as well as the test code and script changes that enable building MIPS binutils and test executables.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2275
TEST=pnacl/build.sh misc-tools ; src/trusted/validator_mips/testdata/compile_tests.sh

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=8780

[petarj, 2012-04-04 20:44:15]

Initial change for MIPS architecture. It includes implementation of the
validator and test nexes.

[Mark Seaborn, 2012-04-04 21:04:22]

Some initial comments.  I haven't looked at the validator code yet.

Please set BUG= and TEST= fields in the commit message.  BUG should contain the
URL of the issue you filed for MIPS support
(http://code.google.com/p/nativeclient/issues/detail?id=2275).

I notice you're adding .nexes for the test data.  Please don't do that.  The ARM
validator might do that, but we don't want to repeat that for any new code.  The
alternative to that are:

 1) Build nexes from the .S files at build time and run your MIPS validator
against those.  That is awkward without a MIPS cross-toolchain, however.

 2) Use the golden file approach that the x86 validator uses.

http://codereview.chromium.org/9979025/diff/64/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9979025/diff/64/SConstruct#newcode929
SConstruct:929: 'mips'        : { 'arch' : 'mips', 'subarch' : '32' },
Should this be called "mips32"?

http://codereview.chromium.org/9979025/diff/64/SConstruct#newcode1748
SConstruct:1748: or env.Bit('target_mips'))
Nit: Indent by 1 more to line up with '('

http://codereview.chromium.org/9979025/diff/64/SConstruct#newcode1961
SConstruct:1961: if env.Bit('target_mips') and env.UsingEmulator():
This is a duplicate of the previous block, right?  You could just remove the
env.Bit('target_arm') conditional on the previous block, because it's redundant.

http://codereview.chromium.org/9979025/diff/64/SConstruct#newcode2566
SConstruct:2566: if base_env.Bit('target_mips'):
Please make this unconditional: I want to ensure that the MIPS validator builds
OK everywhere (and the tests should pass everywhere too).  For background, see
http://code.google.com/p/nativeclient/issues/detail?id=574.

http://codereview.chromium.org/9979025/diff/64/SConstruct#newcode3008
SConstruct:3008: print 'placeholder for MIPS'
You can just replace this with "pass".  No need to print anything.

http://codereview.chromium.org/9979025/diff/64/src/trusted/service_runtime/bu...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/9979025/diff/64/src/trusted/service_runtime/bu...
src/trusted/service_runtime/build.scons:298: VALIDATOR_LIB= 'ncvalidate_mips'
Put spaces around '='

[petarj, 2012-04-12 00:35:21]

The patch has been changed in the way that:
- style issues reported in the initial code review have been corrected,
- nexes have been removed; instead of that, pnacl/build.sh has been modified to
build binutils for MIPS, so with this patch the test nexes for the validator can
be generated from the patch.
- related binutils patch to this change is:
http://codereview.chromium.org/10020059/

[Brad Chen, 2012-04-12 00:51:34]

Trying to get started on this.

Are you able to do trybot runs? We will need those before this can be committed.

http://codereview.chromium.org/9979025/diff/5001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9979025/diff/5001/SConstruct#newcode1962
SConstruct:1962: if env.UsingEmulator():
Have you determined that the env.Bit('target_arm') test was redundant? Please
explain.

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh#newcode52
pnacl/build.sh:52: # We don't yet support building ARM tools for mac or windows.
Please update this comment.

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh#newcode3267
pnacl/build.sh:3267: cp -a
"${BINUTILS_INSTALL_DIR}/bin/arm-${BINUTILS_TARGET}-readelf" \
Why do these names need to be changed? Where are the MIPS versions installed?

[petarj, 2012-04-12 13:21:01]

http://codereview.chromium.org/9979025/diff/5001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9979025/diff/5001/SConstruct#newcode1962
SConstruct:1962: if env.UsingEmulator():
On 2012/04/12 00:51:34, Brad Chen wrote:
> Have you determined that the env.Bit('target_arm') test was redundant? Please
> explain.
This has been changed per Mark's comment in the previous review. Check at:
http://codereview.chromium.org/9979025/diff/64/SConstruct#newcode1961

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh#newcode52
pnacl/build.sh:52: # We don't yet support building ARM tools for mac or windows.
On 2012/04/12 00:51:34, Brad Chen wrote:
> Please update this comment.
Ok.

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh#newcode3267
pnacl/build.sh:3267: cp -a
"${BINUTILS_INSTALL_DIR}/bin/arm-${BINUTILS_TARGET}-readelf" \
On 2012/04/12 00:51:34, Brad Chen wrote:
> Why do these names need to be changed? Where are the MIPS versions installed?
This patch tends to contain minimal (yet self-sufficient) set of changes that
are needed for MIPS validator testing. Thus, not all changes for this script
have been included.
With this change, binutils build produces '$ARCH-pc-nacl-executables', see the
changes at L61 and L1906:1924 in this file.

[(google.com) Derek Schuff, 2012-04-12 16:46:39]

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh#newcode1
pnacl/build.sh:1: #!/bin/bash
The build.sh changes appear to be partly focused on building binutils (which
belongs in a separate CL) and partly just adding the mips validator to the pnacl
package (which also does not need to be in this CL). So I think this whole file
should be removed for now.

[petarj, 2012-04-12 17:38:17]

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh#newcode1
pnacl/build.sh:1: #!/bin/bash
On 2012/04/12 16:46:39, Derek Schuff wrote:
> The build.sh changes appear to be partly focused on building binutils (which
> belongs in a separate CL) and partly just adding the mips validator to the
pnacl
> package (which also does not need to be in this CL). So I think this whole
file
> should be removed for now.
Changes that enable building binutils for MIPS should land before or at the same
time with validator-mips changes, so validator tests can be compiled. I can
create a separate CL with just these changes for build.sh for binutils, if that
is the way it is preferred.

[(google.com) Derek Schuff, 2012-04-12 17:54:22]

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh#newcode1
pnacl/build.sh:1: #!/bin/bash
true, but I don't see any validator tests in this CL that require binutils.
edit: nevermind, I was looking for (and not finding) a nacl.scons file. Didn't
see the compile_tests.sh. Perhaps that should be fixed as well.

however, changes to build binutils should still be in a separate CL.
Unfortunately you also can't land build.sh changes in the same CL as the
binutils changes themselves (different repo), so they can just be reviewed as a
2-CL unit.

On 2012/04/12 17:38:17, petarj wrote:
> On 2012/04/12 16:46:39, Derek Schuff wrote:
> > The build.sh changes appear to be partly focused on building binutils (which
> > belongs in a separate CL) and partly just adding the mips validator to the
> pnacl
> > package (which also does not need to be in this CL). So I think this whole
> file
> > should be removed for now.
> Changes that enable building binutils for MIPS should land before or at the
same
> time with validator-mips changes, so validator tests can be compiled. I can
> create a separate CL with just these changes for build.sh for binutils, if
that
> is the way it is preferred.

[Brad Chen, 2012-04-12 17:55:01]

In general more smaller change lists would be appreciated. In particular,
if you mostly isolate changes to src/trusted from other CLs that would be
great.

Brad

On Thu, Apr 12, 2012 at 10:38 AM, <petarj@mips.com> wrote:

>
>
http://codereview.chromium.**org/9979025/diff/5001/pnacl/**build.sh<http://co...
> File pnacl/build.sh (right):
>
> http://codereview.chromium.**org/9979025/diff/5001/pnacl/**
>
build.sh#newcode1<http://codereview.chromium.org/9979025/diff/5001/pnacl/build.sh#newcode1>
> pnacl/build.sh:1: #!/bin/bash
> On 2012/04/12 16:46:39, Derek Schuff wrote:
>
>> The build.sh changes appear to be partly focused on building binutils
>>
> (which
>
>> belongs in a separate CL) and partly just adding the mips validator to
>>
> the pnacl
>
>> package (which also does not need to be in this CL). So I think this
>>
> whole file
>
>> should be removed for now.
>>
> Changes that enable building binutils for MIPS should land before or at
> the same time with validator-mips changes, so validator tests can be
> compiled. I can create a separate CL with just these changes for
> build.sh for binutils, if that is the way it is preferred.
>
>
http://codereview.chromium.**org/9979025/<http://codereview.chromium.org/9979...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Brad Chen, 2012-04-13 00:50:52]

Adding ncbray, mostly for python.

Can you explain a little about the structure of the test suite? Is there some
sense in which you believe these are complete/comprehensive? Or are they just
the tests that came together as you were debugging the compiler?

Apologize in advance if some of these comments seem trite. Next, the actual
validator implementation...

http://codereview.chromium.org/9979025/diff/5001/src/trusted/service_runtime/...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:231: ldr_inputs +=
['linux/nacl_signal_mips.c']
This file is not in the repo, right? Is there a reason not to hold this change
until nacl_signal_mips.c is ready?

http://codereview.chromium.org/9979025/diff/5001/src/trusted/service_runtime/...
src/trusted/service_runtime/build.scons:298: VALIDATOR_LIB = 'ncvalidate_mips'
Ditto; I'm wondering if this is premature.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/o...
File src/trusted/validator_mips/optimize-table.py (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/o...
src/trusted/validator_mips/optimize-table.py:3: # Copyright 2009 The Native
Client Authors.  All rights reserved.
2012, everywhere. Make sure you run the presubmit checker.

I don't think the "Copyright 2009, Google Inc." should be there. Where did that
come from?

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
File src/trusted/validator_mips/testdata/compile_tests.sh (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
src/trusted/validator_mips/testdata/compile_tests.sh:5: # Copyright 2012 MIPS
Technologies / RT-RK
I don't think this MIPS copyright should be here.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
File src/trusted/validator_mips/testdata/test_forbidden_instructions.S (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
src/trusted/validator_mips/testdata/test_forbidden_instructions.S:4: # Copyright
2012 MIPS Technologies / RT-RK.
MIPS Copyright? Please clarify or remove from all files.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
File src/trusted/validator_mips/testdata/test_invalid_dest.S (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
src/trusted/validator_mips/testdata/test_invalid_dest.S:6: # With this test we
are tryig to branch on address above 256 MB
...trying to branch to an address...

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
File src/trusted/validator_mips/testdata/test_jmp_imm.S (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
src/trusted/validator_mips/testdata/test_jmp_imm.S:9: # 2. jmps into
0x10000-0x20000, trampoline code section, jmps need to be bundle aligned
Please try to stick with 80-columns unless syntactically impossible.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
src/trusted/validator_mips/testdata/test_jmp_imm.S:11: # beside destination
address we also check the position of jmp/b and link
Standard capitalization makes the comments more readable. I don't want to be a
grammar nanny, but we usually try to stick with standard English and
punctuation.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/t...
src/trusted/validator_mips/testdata/test_jmp_imm.S:22: nop
It looks like the convention you are using is an indented nop to indicate a
delay slot? This may be confusing to some of your reviewers, but if that is the
standard MIPS convention then you should probably stick with it. Maybe you could
comment the first such nop just so that a new reader might understand more
quickly.

[Nick Bray (chromium), 2012-04-13 20:25:26]

A quick pass over they Python code...

http://codereview.chromium.org/9979025/diff/5001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9979025/diff/5001/SConstruct#newcode1021
SConstruct:1021: if env.Bit('target_mips32') and not env.Bit('bitcode'):
Optional:

if env.Bit('target_arm') or env.Bit('target_mips32'):
  if not env.Bit('bitcode'):
    env.SetBits('bitcode')

It's the same issue, so fusing the statement makes that clearer.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/b...
File src/trusted/validator_mips/build.scons (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/b...
src/trusted/validator_mips/build.scons:10: 
I'd kill the empty line after the "if" - it's not our style.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/b...
src/trusted/validator_mips/build.scons:47: 
Single empty line, except between toplevel functions and classes.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/b...
src/trusted/validator_mips/build.scons:59: # TODO(cbiffle): get this wrapped in
QEMU.
TODOs list the person placing the TODO.  TODO yourself. cbiffle is no longer
working on the project.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/b...
src/trusted/validator_mips/build.scons:80: stdout_golden = env.File('testdata/'
+ test + '.out'),
I realize you're following an established pattern, but everything being equal,
I'd rather have fewer golden file tests.  The end up being fragile and make
refactoring hard.

You can commit as-is, but golden files can be a maintenance issue, and
maintenance issues for externally contributed code may get ugly down the road. 
If it would be easy to make the tests more unit-testy (either now or down the
road), you'd be more futureproof.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/b...
src/trusted/validator_mips/build.scons:99: 'run_validator_tests')
run_mips_validator_tests?

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/d...
File src/trusted/validator_mips/dgen_core.py (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/d...
src/trusted/validator_mips/dgen_core.py:1: #!/usr/bin/python
Putting the dgen_* files into a module might help cleanup your directory
structure?  It wouldn't be worthwhile if importing the module get complicated,
however.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/d...
src/trusted/validator_mips/dgen_core.py:2: #
No empty line.  The file you're copying this from is non-standard.

Looking at validator_arm, it appears that the dgen_* files has been copied
almost verbatim.  Please eliminate code duplication, to whatever extent you can
- this might involve a CL to lift this code out of validator_arm into a common
directory as an intermediate change.

This will help me review what you actually changed, too.  Punting on the dgen_*
files until then.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/d...
src/trusted/validator_mips/dgen_core.py:3: # Copyright 2009 The Native Client
Authors.  All rights reserved.
Update all copyrights to 2012 in the files you've added and modified.  If you
don't, presubmit checks will stop you from commiting.

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/v...
File src/trusted/validator_mips/validation-report.py (right):

http://codereview.chromium.org/9979025/diff/5001/src/trusted/validator_mips/v...
src/trusted/validator_mips/validation-report.py:25: for line in Popen(args,
stdout=PIPE).stdout.read().split('\n'):
p.communicate() is preferred in most cases.

[petarj, 2012-04-26 18:26:30]

Hi All,

here is an updated patch. The changes include:

- the patch has been rebased to the latest revision.
- parts of the original change have been uploaded as separate CLs (changes for
building binutils can be seen at http://codereview.chromium.org/10020059/)
- comments in the test files have been corrected.
- copyright issues have been corrected.
- golden files (.out) for validator tests have been removed.
- python files are moved into dgen folder and are left as-is until ARM
counterparts are sorted out. (as agreed on
http://codereview.chromium.org/10191011/)
- updates to SConstruct and build.scons files per code review.

Looking forward to getting your comments.

Petar

[Nick Bray (chromium), 2012-04-27 19:41:33]

Nothing but nits on the scripts.

http://codereview.chromium.org/9979025/diff/19001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9979025/diff/19001/SConstruct#newcode2914
SConstruct:2914: pass
Nit: are there TODOs?

http://codereview.chromium.org/9979025/diff/19001/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/19001/pnacl/build.sh#newcode49
pnacl/build.sh:49: # PNACL_BUILD_MIPS=true
Nit: I haven't ever looked at this script before, but adding a commented line
raises the question of what you want and if there's a better solution.

http://codereview.chromium.org/9979025/diff/19001/src/trusted/service_runtime...
File src/trusted/service_runtime/build.scons (right):

http://codereview.chromium.org/9979025/diff/19001/src/trusted/service_runtime...
src/trusted/service_runtime/build.scons:226: pass
Nit: Are there TODOs?

http://codereview.chromium.org/9979025/diff/19001/src/trusted/validator_mips/...
File src/trusted/validator_mips/validation-report.py (right):

http://codereview.chromium.org/9979025/diff/19001/src/trusted/validator_mips/...
src/trusted/validator_mips/validation-report.py:8: 
One newline here.

http://codereview.chromium.org/9979025/diff/19001/src/trusted/validator_mips/...
src/trusted/validator_mips/validation-report.py:14: 
Two newlines around functions and classes at module scope.

http://codereview.chromium.org/9979025/diff/19001/src/trusted/validator_mips/...
src/trusted/validator_mips/validation-report.py:103: for line in sys.stdin:
Nit: Wrap inside a main() function, prevents the code from poluting the global
namespace.

http://codereview.chromium.org/9979025/diff/19001/src/trusted/validator_mips/...
src/trusted/validator_mips/validation-report.py:106:
_explain_problem(sys.argv[1], *_parse_report(line))
Nit: A little input validation here (len(sys.argv) == 2 and possibly
sys.path.exists(sys.argv[1])) might save a headache down the road.

[petarj, 2012-04-30 19:16:36]

Here is a new patch set per Nick's code review.

The changes are:

- TODOs have been added to the scripts
- unnecessary variable PNACL_BUILD_MIPS has been temporarily removed (it will
resurrect in some of the next CLs)
- validation-report.py has been changed to wrap the code in a main function and
recommended validation checks have been added as well.

[Nick Bray, 2012-05-01 17:34:06]

Embarrassingly minor nits.  (Don't bother uploading them, just wait for other
reviews.)  Consider me signed off on the Python stuff.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/build.scons (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/build.scons:57:
#env.AddNodeToTestSuite(address_set_test, ['small_tests'], 'address_set_test')
Change the test name to mips_address_set_test and it should be OK?

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/validation-report.py (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/validation-report.py:111: print >>sys.stderr,
('Error, provide one parameter to the script')
">>" syntax is discouraged.  (Nonstandard for our code base, deprecated in
Python 3.0.)  Prefer sys.stderr.write(message + '\n') or just printing to
stdout.

[Brad Chen, 2012-05-04 20:16:10]

Adding Robert for more PNaCl perspective.

http://codereview.chromium.org/9979025/diff/27099/SConstruct
File SConstruct (right):

http://codereview.chromium.org/9979025/diff/27099/SConstruct#newcode1717
SConstruct:1717: # Disabled for ARM because Chrome binaries for ARM are not
available.
Add a comment for MIPS.

http://codereview.chromium.org/9979025/diff/27099/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/27099/pnacl/build.sh#newcode1834
pnacl/build.sh:1834: cp
scons-out/opt-linux-x86-32-to-mips32/obj/src/trusted/validator_mips/\
${SCONS-OUT}, as in the clause above for ARM? Also, is the rm/mkdir not
necessary? If so why?

[robertm, 2012-05-04 20:52:43]

http://codereview.chromium.org/9979025/diff/27099/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/27099/pnacl/build.sh#newcode1834
pnacl/build.sh:1834: cp
scons-out/opt-linux-x86-32-to-mips32/obj/src/trusted/validator_mips/\
On 2012/05/04 20:16:11, Brad Chen wrote:
> ${SCONS-OUT}, as in the clause above for ARM? Also, is the rm/mkdir not
> necessary? If so why?

I think it would be good to factor the code into a new function and then have
something like

 rm -rf  "${INSTALL_ROOT}/tools-x86"
 mkdir "${INSTALL_ROOT}/tools-x86"
 build-arm-validator <args>
 build-mips-validort <args>

[Brad Chen, 2012-05-04 22:49:50]

Here's some feedback on the validator implementation. I will probably want to do
some of this review via phone/VC. Things that are hard for me to get out of
reading:
- structure/architecture of validator
- structure/architecture of tests

In a few places I note style things that need to be corrected throughout: mixed
case for method names, avoid operating overloading, etc. Hopefully you
understand that, although it might seem like they negatively impact the
readability of these files, by making them consistent with the rest of the
codebase the overall impact on readability and maintainability is positive.

Another tedious thing I'm asking you to do is generate your decoder source into
a checked-in file, instead of ${DESTINATION_ROOT}. Understand there is quite a
bit of pain and suffering behind that particular request.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/address_set.cc (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.cc:15: : base_(base), size_(size),
bits_(new uint32_t[(size + 3) / 4]) {
Please declare const int ... for 3 and 4 here.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.cc:27: bits_[word_address / 32] |= 1 <<
(word_address % 32);
Here too. const unsigned int kWordSize = 32 ???

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.cc:78: // A meager optimization for
sparse words
A little more explanation would be nice.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/address_set.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.h:41: void add(uint32_t address);
Please use mixed case for function/method names (throughout).

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.h:53: Iterator &operator++();
In general we prefer to avoid using operator overloading in Google code. (see
google-styleguide.googlecode.com). Do you think you can live without overloading
here?

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/address_set_test.cc (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set_test.cc:19: using
nacl_mips_val::AddressSet;
Please avoid using "using", as per the style guide.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set_test.cc:42: abort();
Please add a TODO(petarj) comment someplace that says these tests need to be
rewritten to use a proper unit testing framework once Native Client has put a
proper unit testing framework in place.

Are you familiar with GTest? Maybe from your V8 work you've seen better unit
testing infrastructure. I wish I could point you at an example from the NaCl
code base of a validator test that used a proper unit-testing framework. If you
have any thoughts on the subject feel free to express them in the code.

In any case, imitating the style of error messages from Chromium bots would
probably be an improvement. Also, instead of calling abort() after the first
error, maybe life would be better if you ran all the tests first and then called
abort() if any of them failed.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/build.scons (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/build.scons:16:
gen_dec=('${DESTINATION_ROOT}/gen/native_client/src/trusted/validator_mips/'
If you look at the NaCl x86 validator you may recognize that we generate our
decoder from a table, but check the generated validator into our source repo.
This was done to avoid some nasty issues with generating the validator during
the build. Targets valclean and valgen. We did this because Chromium builds use
"gyp" not "scons", and gyp makes it harder to do this kind of thing. 

Can you please do something similar here? I suggest scons targets "mipsvalclean"
and "mipsvalgen".

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/dgen/generate_decoder.py (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/dgen/generate_decoder.py:11: Usage:
generate-decoder.py <table-file> <output-cc-file>
So ... I understand this was copied from old ARM decoder tablegen without mods,
correct?

You should understand that Karl is making a bunch of changes/improvements on the
ARM side, and when those are stable we will likely want to drop this old
tablegen and start using his new tablegen.

Can you please add a TODO comment to that effect?

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/inst_classes.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:24: * and even those may be rejected
by the validator.
Why the holes in the enumeration? Why not consecutive values?

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:27: // The initial value of
uninitialized SafetyLevels -- treat as unsafe.
Please don't mix comment styles. For C stick with /* */.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:67: virtual SafetyLevel
safety(Instruction i) const = 0;
Mixed case for function/method names. Possible exception if you are capitalizing
to be consistent with a large bode of existing code.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:75: virtual bool is_mask(Instruction
i, nacl_mips_dec::Register dest,
Can functions like these could use more const qualifies on parameters? Is the
Instruction ever modified?

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:214: virtual Register
dest_gpr_reg(Instruction i) const {
I would be grateful if you could think of a better name for the parameter "i".
"instr"?

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/model-inl.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/model-inl.h:23: bool Register::operator==(const
Register &other) const {
Avoid operator overloading.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/model.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/model.h:56: inline bool operator==(const Register &)
const;
avoid operator overloading

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/validator.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/validator.h:23: #include
"native_client/src/include/portability.h"
includes should generally be in alphabetical order.

[petarj, 2012-05-08 14:54:19]

Hi All,

thank you for the feedback. Before we make some changes, I would like to discuss
them just briefly. In short, I am puzzled by some previous and recent comments
(esp. in which the code style was criticized). The truth is that we tried to
reuse some of your existing code (to be inline with code style), and thus the
code snippets you are making comments about are actually copied from the already
existing code in Native Client. In some of the cases, if we try to change the
code per your reviews, it will diverge more from the rest of the code (mostly to
ARM implementation). I responded to some of those.

How do you want to treat those style issues?

Let me know. Thanks.

Petar

http://codereview.chromium.org/9979025/diff/27099/pnacl/build.sh
File pnacl/build.sh (right):

http://codereview.chromium.org/9979025/diff/27099/pnacl/build.sh#newcode1834
pnacl/build.sh:1834: cp
scons-out/opt-linux-x86-32-to-mips32/obj/src/trusted/validator_mips/\
On 2012/05/04 20:52:44, robertm wrote:
> On 2012/05/04 20:16:11, Brad Chen wrote:
> > ${SCONS-OUT}, as in the clause above for ARM? Also, is the rm/mkdir not
> > necessary? If so why?
> 
> I think it would be good to factor the code into a new function and then have
> something like
> 
>  rm -rf  "${INSTALL_ROOT}/tools-x86"
>  mkdir "${INSTALL_ROOT}/tools-x86"
>  build-arm-validator <args>
>  build-mips-validort <args>

Ok, we will refactor it this way.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/address_set.cc (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.cc:15: : base_(base), size_(size),
bits_(new uint32_t[(size + 3) / 4]) {
On 2012/05/04 22:49:50, Brad Chen wrote:
> Please declare const int ... for 3 and 4 here. 

Copied as-is from validator_arm/address_set.cc. We can change it though.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.cc:27: bits_[word_address / 32] |= 1 <<
(word_address % 32);
On 2012/05/04 22:49:50, Brad Chen wrote:
> Here too. const unsigned int kWordSize = 32 ???

Copied as-is from validator_arm/address_set.cc. We can change it though.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.cc:78: // A meager optimization for
sparse words
On 2012/05/04 22:49:50, Brad Chen wrote:
> A little more explanation would be nice.

Ditto.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/address_set.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.h:41: void add(uint32_t address);
On 2012/05/04 22:49:50, Brad Chen wrote:
> Please use mixed case for function/method names (throughout).

Copied as-is from validator_arm/address_set.h. We can change it though.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set.h:53: Iterator &operator++();
On 2012/05/04 22:49:50, Brad Chen wrote:
> In general we prefer to avoid using operator overloading in Google code. (see
> http://google-styleguide.googlecode.com). Do you think you can live without
overloading
> here?
Copied as-is from validator_arm/address_set.h. We can change it though.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/address_set_test.cc (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set_test.cc:19: using
nacl_mips_val::AddressSet;
On 2012/05/04 22:49:50, Brad Chen wrote:
> Please avoid using "using", as per the style guide.
validator_arm/address_set_test.cc has it, so we assumed it is ok for us to use
it as well.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/build.scons (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/build.scons:57:
#env.AddNodeToTestSuite(address_set_test, ['small_tests'], 'address_set_test')
On 2012/05/01 17:34:07, Nick Bray wrote:
> Change the test name to mips_address_set_test and it should be OK?

Sure.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/dgen/generate_decoder.py (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/dgen/generate_decoder.py:11: Usage:
generate-decoder.py <table-file> <output-cc-file>
On 2012/05/04 22:49:50, Brad Chen wrote:
> So ... I understand this was copied from old ARM decoder tablegen without
mods,
> correct?
> 
> You should understand that Karl is making a bunch of changes/improvements on
the
> ARM side, and when those are stable we will likely want to drop this old
> tablegen and start using his new tablegen.
> 
> Can you please add a TODO comment to that effect?

Sure, will do.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/inst_classes.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:24: * and even those may be rejected
by the validator.
On 2012/05/04 22:49:50, Brad Chen wrote:
> Why the holes in the enumeration? Why not consecutive values?

We used the same values as ARM implementation does, and we wanted to have the
(same) missing values still available for future extensions/rework. We can
change it to consecutive values if you prefer it that way.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:27: // The initial value of
uninitialized SafetyLevels -- treat as unsafe.
On 2012/05/04 22:49:50, Brad Chen wrote:
> Please don't mix comment styles. For C stick with /* */.
Copied as-is from validator_arm/inst_classes.h

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:67: virtual SafetyLevel
safety(Instruction i) const = 0;
On 2012/05/04 22:49:50, Brad Chen wrote:
> Mixed case for function/method names. Possible exception if you are
capitalizing
> to be consistent with a large bode of existing code.

Copied as-is from validator_arm/inst_classes.h.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:214: virtual Register
dest_gpr_reg(Instruction i) const {
On 2012/05/04 22:49:50, Brad Chen wrote:
> I would be grateful if you could think of a better name for the parameter "i".
> "instr"?
We would gladly replace it with 'instr', sure, just the same name 'i' is used in
validator_arm/inst_classes.h. It made sense to us to use the same variable name
as you did, since we copied the class.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/model-inl.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/model-inl.h:23: bool Register::operator==(const
Register &other) const {
On 2012/05/04 22:49:50, Brad Chen wrote:
> Avoid operator overloading.

Taken from validator_arm/model-inl.h

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/model.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/model.h:56: inline bool operator==(const Register &)
const;
On 2012/05/04 22:49:50, Brad Chen wrote:
> avoid operator overloading
Seen in validator_arm/model.h

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/validation-report.py (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/validation-report.py:111: print >>sys.stderr,
('Error, provide one parameter to the script')
On 2012/05/01 17:34:07, Nick Bray wrote:
> ">>" syntax is discouraged.  (Nonstandard for our code base, deprecated in
> Python 3.0.)  Prefer sys.stderr.write(message + '\n') or just printing to
> stdout.

Sure, it will be changed. This seemed to me like a standard to your code base,
this is why I used that. Check for instance these files:

pyauto_nacl.py, process_oprofile_x86_64.py, win_as.py, test_lib.py,
syscall_stats.py, test_timing.py, elf_fuzzer.py, elf_checker.py, enum_gen.py,
nacl_syscall_handlers_gen.py, export_header.py, parse_llvm_testsuite_report.py,
make_expectations.py, run_ncval_tests.py, run_objdump_test.py, etc.

[Derek Schuff, 2012-05-08 15:59:15]

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/address_set_test.cc (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/address_set_test.cc:19: using
nacl_mips_val::AddressSet;
According to the style guide, 'using' is ok for a single class, just not for a
whole namespace, so this should be ok.

On 2012/05/08 14:54:19, petarj wrote:
> On 2012/05/04 22:49:50, Brad Chen wrote:
> > Please avoid using "using", as per the style guide.
> validator_arm/address_set_test.cc has it, so we assumed it is ok for us to use
> it as well.

[Nick Bray, 2012-05-08 17:36:53]

> Hi All,
> 
> thank you for the feedback. Before we make some changes, I would like to
discuss
> them just briefly. In short, I am puzzled by some previous and recent comments
> (esp. in which the code style was criticized). The truth is that we tried to
> reuse some of your existing code (to be inline with code style), and thus the
> code snippets you are making comments about are actually copied from the
already
> existing code in Native Client. In some of the cases, if we try to change the
> code per your reviews, it will diverge more from the rest of the code (mostly
to
> ARM implementation). I responded to some of those.
> 
> How do you want to treat those style issues?


I tried to avoid calling you on style issues you didn't create yourself - I was
painfully aware you were adapting existing code much of the time.  We do have
existing style issues in our code base, however, particularly in the less
traveled parts of our code base (such as the ARM validator, for example.)  This
is rather embarrassing.  Currently we're doing to boy scout thing: leave the
code base better than when you found it.  Often this requires everyone to fix
problems they did not create.  Occasionally I have wanted to throttle my
reviewer when issues like this pop up - you aren't alone.  It's all about
maintainability, however.

In general, we try to adhere to these guides:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html
(Unless the entire file is out of wack, then we try to be consistent.  I think
you could easily argue consistency in many cases.  Sadly.)
The internal Google style guide does differ in some places, but not many.

[Brad Chen, 2012-05-08 23:09:26]

Thank you for calling out these issues, and apologies about the style
issues from copied code already in the repo. Nick got it right with the
'boy scout' thing. Please feel free (but not obligated) to file bugs when
you see style issues, and to push-back when reviewers are asking for things
that seem inconsistent with the style guide.

Brad

On Tue, May 8, 2012 at 10:36 AM, <ncbray@google.com> wrote:

> Hi All,
>>
>
>  thank you for the feedback. Before we make some changes, I would like to
>>
> discuss
>
>> them just briefly. In short, I am puzzled by some previous and recent
>> comments
>> (esp. in which the code style was criticized). The truth is that we tried
>> to
>> reuse some of your existing code (to be inline with code style), and thus
>> the
>> code snippets you are making comments about are actually copied from the
>>
> already
>
>> existing code in Native Client. In some of the cases, if we try to change
>> the
>> code per your reviews, it will diverge more from the rest of the code
>> (mostly
>>
> to
>
>> ARM implementation). I responded to some of those.
>>
>
>  How do you want to treat those style issues?
>>
>
>
> I tried to avoid calling you on style issues you didn't create yourself -
> I was
> painfully aware you were adapting existing code much of the time.  We do
> have
> existing style issues in our code base, however, particularly in the less
> traveled parts of our code base (such as the ARM validator, for example.)
>  This
> is rather embarrassing.  Currently we're doing to boy scout thing: leave
> the
> code base better than when you found it.  Often this requires everyone to
> fix
> problems they did not create.  Occasionally I have wanted to throttle my
> reviewer when issues like this pop up - you aren't alone.  It's all about
> maintainability, however.
>
> In general, we try to adhere to these guides:
>
http://google-styleguide.**googlecode.com/svn/trunk/**cppguide.xml<http://goo...
>
http://google-styleguide.**googlecode.com/svn/trunk/**pyguide.html<http://goo...
> (Unless the entire file is out of wack, then we try to be consistent.  I
> think
> you could easily argue consistency in many cases.  Sadly.)
> The internal Google style guide does differ in some places, but not many.
>
>
http://codereview.chromium.**org/9979025/<http://codereview.chromium.org/9979...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[petarj, 2012-05-17 17:15:25]

New patch set has just been uploaded. Here is a brief overview of the changes
inside:

- the generated decoder (decode.cc) is now checked in the source, and
build.scons has been changed to generate that file to the already source repo
location. Targets "mipsvalclean" and "mipsvalgen" have been added.
- patch for pnacl/build.sh has been changed, so now the same function is used to
build validator for ARM and MIPS.
- address_set has have been rewritten to be cleaner. The tests have been
modified with the given suggestions.
- function names changed to use MixedCases where applicable.
- operator overloading removed.
- output from Python script has been changed to use sys.stderr.write.
- other reported code style issues have been corrected.
- the code has been rebased against commit
31b6ba2b1675ff08af8894625249d8eaf3314dff (May 17)

Looking forward to getting your feedback.

[Brad Chen, 2012-05-29 16:20:39]

Thank you for making all these changes. It looks much better. There are two
things I will ask of you before you commit this. One is that we ask Karl to take
a quick look towards keeping this in synch with his improvements to the ARM
validator. The second is that you add a TODO(peterj) comment indicating that the
MIPS validator and runtime port need an external security review before they are
delivered in products. An external review is something we ought talk about in
our next F2F meeting. It's something we've done for our own validators and other
TCB components.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/inst_classes.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/inst_classes.h:24: * and even those may be rejected
by the validator.
What you will probably need to do is watch the source for the ARM validator,
which is being rewritten/improved, and follow with changes to your MIPS
implementation. That's probably the best we can do. I appreciate your efforts to
make the two implementations as similar as possible.

On 2012/05/08 14:54:19, petarj wrote:
> On 2012/05/04 22:49:50, Brad Chen wrote:
> > Why the holes in the enumeration? Why not consecutive values?
> 
> We used the same values as ARM implementation does, and we wanted to have the
> (same) missing values still available for future extensions/rework. We can
> change it to consecutive values if you prefer it that way.

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
File src/trusted/validator_mips/model-inl.h (right):

http://codereview.chromium.org/9979025/diff/27099/src/trusted/validator_mips/...
src/trusted/validator_mips/model-inl.h:23: bool Register::operator==(const
Register &other) const {
I guess we could get a second opinion from Karl or David, but ... I hate
operator overloading.

On 2012/05/08 14:54:19, petarj wrote:
> On 2012/05/04 22:49:50, Brad Chen wrote:
> > Avoid operator overloading.
> 
> Taken from validator_arm/model-inl.h

[petarj, 2012-05-29 16:39:25]

On 2012/05/29 16:20:39, Brad Chen wrote:
> What you will probably need to do is watch the source for the ARM validator,
> which is being rewritten/improved, and follow with changes to your MIPS
> implementation. That's probably the best we can do. I appreciate your efforts
to
> make the two implementations as similar as possible.

Sure. As of your comment for enum values, I have already changed it in the
previous patch to use consecutive values, if that matters.

> I guess we could get a second opinion from Karl or David, but ... I hate
> operator overloading.

I changed the code in the patch so it does not use operator overloading, so no
more operator overloading.

After Karl does the review, I will update the validator with TODO with the need
for an external review.

[Karl, 2012-05-29 19:48:33]

I've looked at the code, and it is fairly consistent with the ARM validator
(other than current changes being made). I see no underlying problems with the
code.

I think the code is acceptable.

[Brad Chen, 2012-05-29 19:53:12]

On 2012/05/29 19:48:33, Karl wrote:
> I've looked at the code, and it is fairly consistent with the ARM validator
> (other than current changes being made). I see no underlying problems with the
> code.
> 
> I think the code is acceptable.

Therefore LGTM

[Derek Schuff, 2012-05-29 21:02:36]

FYI petarj: you can commit this with the commit queue; to commit you can check
the "commit" box at the bottom of the patch set below all the diffs. This will
kick off a try job and commit automatically when they pass.

On 2012/05/29 19:53:12, Brad Chen wrote:
> On 2012/05/29 19:48:33, Karl wrote:
> > I've looked at the code, and it is fairly consistent with the ARM validator
> > (other than current changes being made). I see no underlying problems with
the
> > code.
> > 
> > I think the code is acceptable.
> 
> Therefore LGTM

[Brad Chen, 2012-05-29 21:09:01]

BTW please remember to add the TODO() comment about an external security review.

On 2012/05/29 21:02:36, dschuff wrote:
> FYI petarj: you can commit this with the commit queue; to commit you can check
> the "commit" box at the bottom of the patch set below all the diffs. This will
> kick off a try job and commit automatically when they pass.
> 
> On 2012/05/29 19:53:12, Brad Chen wrote:
> > On 2012/05/29 19:48:33, Karl wrote:
> > > I've looked at the code, and it is fairly consistent with the ARM
validator
> > > (other than current changes being made). I see no underlying problems with
> the
> > > code.
> > > 
> > > I think the code is acceptable.
> > 
> > Therefore LGTM

[commit-bot: I haz the power, 2012-05-30 00:03:28]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/9979025/77004

[petarj, 2012-05-30 00:07:54]

An updated patch has just been uploaded.
The patch has been rebased to commit b19ffd2741b22fac5f9e9398967ec9c0afafd448
(robertm@google.com, Fix a setup related bug..., May 29), and TODO comment about
an external security review has been added to validator.

I have also checked the "commit" box, hopefully this triggers the commit/trybot
mechanism.

[commit-bot: I haz the power, 2012-05-30 00:30:09]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/9979025/71009

[commit-bot: I haz the power, 2012-05-30 01:54:37]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is nacl-lucid32_glibc_opt, revision is 8712, job
name
was 9979025-71009 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2012-05-30 13:54:03]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/9979025/76010

[commit-bot: I haz the power, 2012-05-30 14:58:34]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is nacl-lucid-64-32-newlib-coverage, revision is
8733, job name
was 9979025-76010 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2012-05-30 18:03:32]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/9979025/81010

[commit-bot: I haz the power, 2012-05-31 00:16:39]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is nacl-mac10.6-newlib-coverage, revision is 8733,
job name
was 9979025-81010 (retry) (retry) (retry) (retry).

[petarj, 2012-05-31 00:27:33]

Derek, Brad,
can anyone give me a hint what is wrong with nacl-mac10.6-newlib-coverage
builder?

Some other builders fail with issues not related to this patch, for instance
nacl-mac10.7_newlib_opt fails on run_ppapi_example_audio_test, and
nacl-lucid64_newlib_opt fails on run_pyauto_surfaway_test and
run_pyauto_reload_test.

On 2012/05/31 00:16:39, I haz the power (commit-bot) wrote:
> The commit queue went berserk retrying too often for a
> seemingly flaky test. Builder is nacl-mac10.6-newlib-coverage, revision is
8733,
> job name
> was 9979025-81010 (retry) (retry) (retry) (retry).

[Brad Chen, 2012-05-31 17:03:20]

Sorry about the CQ difficulties. We are cleaning up a few things in the CQ
(dropping coverage bots among other changes) and hopefully will have it
restarted soon.

Brad

On Wed, May 30, 2012 at 5:27 PM, <petarj@mips.com> wrote:

> Derek, Brad,
> can anyone give me a hint what is wrong with nacl-mac10.6-newlib-coverage
> builder?
>
> Some other builders fail with issues not related to this patch, for
> instance
> nacl-mac10.7_newlib_opt fails on run_ppapi_example_audio_test, and
> nacl-lucid64_newlib_opt fails on run_pyauto_surfaway_test and
> run_pyauto_reload_test.
>
>
> On 2012/05/31 00:16:39, I haz the power (commit-bot) wrote:
>
>> The commit queue went berserk retrying too often for a
>> seemingly flaky test. Builder is nacl-mac10.6-newlib-coverage, revision is
>>
> 8733,
>
>> job name
>> was 9979025-81010 (retry) (retry) (retry) (retry).
>>
>
>
http://codereview.chromium.**org/9979025/<http://codereview.chromium.org/9979...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[commit-bot: I haz the power, 2012-05-31 17:24:09]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/9979025/81010

[Brad Chen, 2012-05-31 17:24:33]

The CQ should be operational again. I clicked the "Commit" box on the CL;
hopefully this time it will make it through.

Brad

On Thu, May 31, 2012 at 10:03 AM, Brad Chen <bradchen@google.com> wrote:

> Sorry about the CQ difficulties. We are cleaning up a few things in the CQ
> (dropping coverage bots among other changes) and hopefully will have it
> restarted soon.
>
> Brad
>
> On Wed, May 30, 2012 at 5:27 PM, <petarj@mips.com> wrote:
>
>> Derek, Brad,
>> can anyone give me a hint what is wrong with nacl-mac10.6-newlib-coverage
>> builder?
>>
>> Some other builders fail with issues not related to this patch, for
>> instance
>> nacl-mac10.7_newlib_opt fails on run_ppapi_example_audio_test, and
>> nacl-lucid64_newlib_opt fails on run_pyauto_surfaway_test and
>> run_pyauto_reload_test.
>>
>>
>> On 2012/05/31 00:16:39, I haz the power (commit-bot) wrote:
>>
>>> The commit queue went berserk retrying too often for a
>>> seemingly flaky test. Builder is nacl-mac10.6-newlib-coverage, revision
>>> is
>>>
>> 8733,
>>
>>> job name
>>> was 9979025-81010 (retry) (retry) (retry) (retry).
>>>
>>
>>
http://codereview.chromium.**org/9979025/<http://codereview.chromium.org/9979...
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[commit-bot: I haz the power, 2012-06-01 03:53:54]

Failed to apply patch for SConstruct:
While running patch -p1 --forward --force;
patching file SConstruct
Hunk #1 succeeded at 878 (offset 2 lines).
Hunk #2 succeeded at 893 (offset 2 lines).
Hunk #3 succeeded at 922 (offset 2 lines).
Hunk #4 succeeded at 933 (offset 2 lines).
Hunk #5 FAILED at 979.
Hunk #6 succeeded at 1129 (offset 6 lines).
Hunk #7 succeeded at 1763 (offset 6 lines).
Hunk #8 succeeded at 1979 (offset 6 lines).
Hunk #9 succeeded at 2623 (offset 53 lines).
Hunk #10 succeeded at 3011 (offset 53 lines).
1 out of 10 hunks FAILED -- saving rejects to file SConstruct.rej

[commit-bot: I haz the power, 2012-06-01 14:33:30]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/petarj@mips.com/9979025/82018

[commit-bot: I haz the power, 2012-06-01 15:59:33]

Change committed as 8780