Add remaining instructions to whitelist

Add remaining instructions to ABI verifier whitelist
(the current tests only cover allowed opcodes; no testing yet of types, attributes, etc).

R=jvoung@chromium.org,eliben@chromium.org,mseaborn@chromium.org
BUG= https://code.google.com/p/nativeclient/issues/detail?id=2196
TEST= LLVM regression

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=8ccd568

[Mark Seaborn, 2013-01-22 21:44:18]

https://codereview.chromium.org/11896044/diff/2001/lib/Analysis/NaCl/PNaClABI...
File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):

https://codereview.chromium.org/11896044/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:106: default:
If you remove the "default:", we should get werrors when new instruction types
are added.

It would be good to explicitly blacklist instructions we don't allow, such as
Indirectbr, rather than only mentioning them in comments.

[Derek Schuff, 2013-01-22 22:13:09]

https://codereview.chromium.org/11896044/diff/2001/lib/Analysis/NaCl/PNaClABI...
File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):

https://codereview.chromium.org/11896044/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:106: default:
Done. It's still a good idea, but
1) LLVM doesn't build with -Werror anyway, and
2) adding -Wswitch-enum to the LLVM build causes a tsunami-freight
train-steamroller of warnings ಠ_ಠ

[eliben, 2013-01-22 22:52:56]

otherwise lgtm

https://codereview.chromium.org/11896044/diff/8001/lib/Analysis/NaCl/PNaClABI...
File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):

https://codereview.chromium.org/11896044/diff/8001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:111: default:
I think the LLVM convention is to have "default" as the first clause in a
switch.

[eliben, 2013-01-22 22:54:00]

On 2013/01/22 22:13:09, Derek Schuff wrote:
>
https://codereview.chromium.org/11896044/diff/2001/lib/Analysis/NaCl/PNaClABI...
> File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):
> 
>
https://codereview.chromium.org/11896044/diff/2001/lib/Analysis/NaCl/PNaClABI...
> lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:106: default:
> Done. It's still a good idea, but
> 1) LLVM doesn't build with -Werror anyway, and
> 2) adding -Wswitch-enum to the LLVM build causes a tsunami-freight
> train-steamroller of warnings ಠ_ಠ

Possibly relevant: 

http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-c...

[Derek Schuff, 2013-01-22 23:52:24]

After some poking around I found that these instructions are not all part of one
enum but several different ones, and that getOpcode returns just unsigned, so no
switch warnings will help us here. So the default stays, but moved it up to the
top, along with explicit cases for the disallowed instructions, just as a
stronger form of documentation.

[jvoung (off chromium), 2013-01-23 00:42:07]

https://codereview.chromium.org/11896044/diff/4004/test/NaCl/PNaClABI/instruc...
File test/NaCl/PNaClABI/instructions.ll (right):

https://codereview.chromium.org/11896044/diff/4004/test/NaCl/PNaClABI/instruc...
test/NaCl/PNaClABI/instructions.ll:54: ret void
CHECK-NOT: disallowed after this too?

https://codereview.chromium.org/11896044/diff/4004/test/NaCl/PNaClABI/instruc...
test/NaCl/PNaClABI/instructions.ll:72: %a5 = getelementptr { i32, i32}* undef
will we support modifiers like "inbounds" -- seems related to supporting "nsw"
or not in the arithmetic instructions?

[Derek Schuff, 2013-01-23 01:00:42]

https://codereview.chromium.org/11896044/diff/4004/test/NaCl/PNaClABI/instruc...
File test/NaCl/PNaClABI/instructions.ll (right):

https://codereview.chromium.org/11896044/diff/4004/test/NaCl/PNaClABI/instruc...
test/NaCl/PNaClABI/instructions.ll:54: ret void
On 2013/01/23 00:42:07, jvoung (cr) wrote:
> CHECK-NOT: disallowed after this too?

Done.

https://codereview.chromium.org/11896044/diff/4004/test/NaCl/PNaClABI/instruc...
test/NaCl/PNaClABI/instructions.ll:72: %a5 = getelementptr { i32, i32}* undef
Yeah, I haven't added the modifiers yet.
On 2013/01/23 00:42:07, jvoung (cr) wrote:
> will we support modifiers like "inbounds" -- seems related to supporting "nsw"
> or not in the arithmetic instructions?

[jvoung (off chromium), 2013-01-23 01:05:52]

lgtm