ARM validator: continue with extra_load_store_instructions.

ARM validator: continue with extra_load_store_instructions.


BUG=validation
TEST=none
R=kschimpf@google.com

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=9598

[Please use jfb - chromium.org, 2012-08-28 02:37:50]

Please review again. git cl upload at my description:

Go through miscellaneous_instructions and update it so that it follows the ARM
ARM revC: it has new instructions and uses one more bit for decoding. Also check
that breakpoints aren't conditional, which would be UNDEFINED.

Go through msr_immediate_and_hints.

Get through table synchronization_primitives.

Move a few more ctors under protected:, disallow more default ctors, and make
friends with more testers.

Add ignores to some pnacl/ sub-directories.

[Karl, 2012-08-28 19:32:35]

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
File src/trusted/validator_arm/armv7.table (right):

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/armv7.table:659: | 0x00      = Deprecated # SWP/SWPB
Note: This probably doesn't parse the way you expect. The current parser does
not recognize a pattern if the rule name is omitted. Use "None" if you aren't
going to specify a rule.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/armv7.table:721: cccc0011001001001111iiiiiiiiiiii
This action can be repeated with " (on the next line) if you replace the pattern
with cccc00110010mm001111iiiiiiiiiiii. This is now allowable due to the recent
change I added.

I assume the issue you have is with the fact that mask 00 is not allowed. We
should consider this a "safety" issue that applies to both rows, rather than
separate patterns. However, our tables are not there yet to allow (simple)
specification of safety rules. Hence, if you are going to leave the patterns the
way you have specified, add a TODO to explain this.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/armv7.table:744: # MSR (Banked register)
Warning: This isn't probably parsing the way you think. The current parser will
only recognize a pattern if a rule name is specified (or None is used as a
placeholder to communicate that there isn't a rule name).

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/armv7.table:747: # MSR (Banked register)
Same here.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
File src/trusted/validator_arm/baseline_classes.h (right):

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/baseline_classes.h:695:
NACL_DISALLOW_DEFAULT_COPY_AND_ASSIGN(LoadStore2RegisterImm8DoubleOp);
Again, the Google C++ style guide suggests using DISALLOW_COPY_AND_ASSIGN, and
this implicitly also forces the omission of the default. Don't use the default
version.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/baseline_classes.h:788:
NACL_DISALLOW_DEFAULT_COPY_AND_ASSIGN(LoadStore2RegisterImm12Op);
Same here.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/baseline_classes.h:1203:
NACL_DISALLOW_DEFAULT_COPY_AND_ASSIGN(LoadStore3RegisterOp);
Same here.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/baseline_classes.h:1245:
NACL_DISALLOW_DEFAULT_COPY_AND_ASSIGN(LoadStore3RegisterDoubleOp);
Same here.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/baseline_classes.h:1377:
NACL_DISALLOW_DEFAULT_COPY_AND_ASSIGN(LoadStore3RegisterImm5Op);
Same here.

[Please use jfb - chromium.org, 2012-08-28 22:19:20]

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
File src/trusted/validator_arm/armv7.table (right):

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/armv7.table:659: | 0x00      = Deprecated # SWP/SWPB
On 2012/08/28 19:32:35, Karl wrote:
> Note: This probably doesn't parse the way you expect. The current parser does
> not recognize a pattern if the rule name is omitted. Use "None" if you aren't
> going to specify a rule.

Done.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/armv7.table:721: cccc0011001001001111iiiiiiiiiiii
On 2012/08/28 19:32:35, Karl wrote:
> This action can be repeated with " (on the next line) if you replace the
pattern
> with cccc00110010mm001111iiiiiiiiiiii. This is now allowable due to the recent
> change I added.
> 
> I assume the issue you have is with the fact that mask 00 is not allowed. We
> should consider this a "safety" issue that applies to both rows, rather than
> separate patterns. However, our tables are not there yet to allow (simple)
> specification of safety rules. Hence, if you are going to leave the patterns
the
> way you have specified, add a TODO to explain this.

Done (added TODO).

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/armv7.table:744: # MSR (Banked register)
On 2012/08/28 19:32:35, Karl wrote:
> Warning: This isn't probably parsing the way you think. The current parser
will
> only recognize a pattern if a rule name is specified (or None is used as a
> placeholder to communicate that there isn't a rule name).

Done.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/armv7.table:747: # MSR (Banked register)
On 2012/08/28 19:32:35, Karl wrote:
> Same here.

Done.

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
File src/trusted/validator_arm/baseline_classes.h (right):

https://chromiumcodereview.appspot.com/10879090/diff/3001/src/trusted/validat...
src/trusted/validator_arm/baseline_classes.h:695:
NACL_DISALLOW_DEFAULT_COPY_AND_ASSIGN(LoadStore2RegisterImm8DoubleOp);
On 2012/08/28 19:32:35, Karl wrote:
> Again, the Google C++ style guide suggests using DISALLOW_COPY_AND_ASSIGN, and
> this implicitly also forces the omission of the default. Don't use the default
> version.

Should I leave it like that to be explicit?

[Please use jfb - chromium.org, 2012-08-29 01:25:12]

Please review again.

[Karl, 2012-08-29 16:11:02]

lgtm

[commit-bot: I haz the power, 2012-08-29 16:11:20]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/jfb@chromium.org/10879090/8002

[commit-bot: I haz the power, 2012-08-29 16:51:20]

Try job failure for 10879090-8002 (retry) on nacl-lucid32_newlib_opt for steps
"annotate, small_tests, validator_ragel_tests, summary".
It's a second try, previously, steps "annotate, small_tests,
validator_ragel_tests, summary" failed.
http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-lucid32_n...

[Please use jfb - chromium.org, 2012-08-29 19:56:15]

Please check again, I had to change test_sp_updates, it was relying on undefined
behavior and failed checkin tests.

[Karl, 2012-08-29 21:03:12]

lgtm

[commit-bot: I haz the power, 2012-08-29 21:04:52]

CQ is trying da patch. Follow status at
https://nativeclient-status.appspot.com/cq/jfb@chromium.org/10879090/13005

[commit-bot: I haz the power, 2012-08-29 21:31:47]

Try job failure for 10879090-13005 (retry) on nacl-lucid32_newlib_dbg for steps
"annotate, small_tests, validator_ragel_tests, summary".
It's a second try, previously, steps "annotate, small_tests,
validator_ragel_tests, summary" failed.
http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-lucid32_n...

[Please use jfb - chromium.org, 2012-08-29 21:46:44]

Failed because the trybots can't patch binary files, so the old test still runs.
Derek suggested `git cl dcommit` since all the other tests pass.