ABI verifier: Add standalone tool pnacl-abicheck

ABI verifier: Add standalone tool pnacl-abicheck 
This CL adds a standalone tool pnacl-abicheck for checking the ABI validity of a bitcode file.
It also adds a flag pnaclabi-verify to llc to enable the same checking in llc.
It also improves the mechanism for handling and reporting validation errors and uses it in both tools.

R=jvoung@chromium.org,mseaborn@chromium.org
BUG= https://code.google.com/p/nativeclient/issues/detail?id=2309

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

[Derek Schuff, 2013-03-12 21:54:58]

Next CL will be a simple standalone checker.

[jvoung (off chromium), 2013-03-12 22:16:21]

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right):

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:28: class PNaClABIVerifyModule :
public ModulePass, public PNaClABIVerifierPass {
Only the module Pass seems to inherit from PNaClABIVerifierPass.  Should the
function pass also inherit?  How will PNaClABIVerifierPass interface be used?

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:89: ErrorCount++;
Perhaps just combine in one method the adding of an error to the Errors, with
counting up ErrorCount?

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp
File tools/llc/llc.cpp (right):

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp#newcode112
tools/llc/llc.cpp:112: ABIVerify("pnaclabi-verify",
Maybe have the PNaCl prefix in the variable name to match the commandline flag.

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp#newcode356
tools/llc/llc.cpp:356: ModulePass* VerifyPass =
createPNaClABIVerifyModulePass(true);
The Function pass is only added when __native_client__ isn't defined.  Perhaps
make the two consistent?

[Mark Seaborn, 2013-03-12 22:25:56]

https://codereview.chromium.org/12449014/diff/1/include/llvm/Analysis/NaCl.h
File include/llvm/Analysis/NaCl.h (right):

https://codereview.chromium.org/12449014/diff/1/include/llvm/Analysis/NaCl.h#...
include/llvm/Analysis/NaCl.h:18: // Separated from the logic of the pass itself
to
Incomplete sentence?

https://codereview.chromium.org/12449014/diff/1/include/llvm/Analysis/NaCl.h#...
include/llvm/Analysis/NaCl.h:24: // Return the number of errors from the
ditto

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right):

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:170: PNaClABIVerifyModule * P = new
PNaClABIVerifyModule();
Spacing should be "*P"

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp
File tools/llc/llc.cpp (right):

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp#newcode356
tools/llc/llc.cpp:356: ModulePass* VerifyPass =
createPNaClABIVerifyModulePass(true);
Spacing should be " *"

[Derek Schuff, 2013-03-13 21:51:01]

(previously: ABI verifier: Add option for fatal error on violation, llc flag to
use it) now renamed.

I went ahead and implemented the standalone checker, which made it easier to
decide what the pass and error-reporting interface should be. PTAL.

https://codereview.chromium.org/12449014/diff/1/include/llvm/Analysis/NaCl.h
File include/llvm/Analysis/NaCl.h (right):

https://codereview.chromium.org/12449014/diff/1/include/llvm/Analysis/NaCl.h#...
include/llvm/Analysis/NaCl.h:18: // Separated from the logic of the pass itself
to
On 2013/03/12 22:25:56, Mark Seaborn wrote:
> Incomplete sentence?

Done.

https://codereview.chromium.org/12449014/diff/1/include/llvm/Analysis/NaCl.h#...
include/llvm/Analysis/NaCl.h:24: // Return the number of errors from the
On 2013/03/12 22:25:56, Mark Seaborn wrote:
> ditto

Done.

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right):

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:28: class PNaClABIVerifyModule :
public ModulePass, public PNaClABIVerifierPass {
Both should inherit, and use is demonstrated in llc.cpp now. The standalone
verifier will do something similar.
On 2013/03/12 22:16:21, jvoung (cr) wrote:
> Only the module Pass seems to inherit from PNaClABIVerifierPass.  Should the
> function pass also inherit?  How will PNaClABIVerifierPass interface be used?

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:89: ErrorCount++;
Yeah i thought about that but the problem is that it breaks the nice ability to
chain the << operators to the Errors stream (which is ok if all the tokens are
strings as they are here, but below some of them are ints). I'm open to ideas
here.
On 2013/03/12 22:16:21, jvoung (cr) wrote:
> Perhaps just combine in one method the adding of an error to the Errors, with
> counting up ErrorCount?

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:170: PNaClABIVerifyModule * P = new
PNaClABIVerifyModule();
On 2013/03/12 22:25:56, Mark Seaborn wrote:
> Spacing should be "*P"

Done.

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp
File tools/llc/llc.cpp (right):

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp#newcode112
tools/llc/llc.cpp:112: ABIVerify("pnaclabi-verify",
On 2013/03/12 22:16:21, jvoung (cr) wrote:
> Maybe have the PNaCl prefix in the variable name to match the commandline
flag.

Done.

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp#newcode356
tools/llc/llc.cpp:356: ModulePass* VerifyPass =
createPNaClABIVerifyModulePass(true);
On 2013/03/12 22:16:21, jvoung (cr) wrote:
> The Function pass is only added when __native_client__ isn't defined.  Perhaps
> make the two consistent?

Done.

https://codereview.chromium.org/12449014/diff/1/tools/llc/llc.cpp#newcode356
tools/llc/llc.cpp:356: ModulePass* VerifyPass =
createPNaClABIVerifyModulePass(true);
On 2013/03/12 22:25:56, Mark Seaborn wrote:
> Spacing should be " *"

Done.

[jvoung (off chromium), 2013-03-13 22:40:09]

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right):

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:89: ErrorCount++;
The new version seems good.  If you are going to use <<, then perhaps that could
be used more uniformly over the +.


On 2013/03/13 21:51:01, Derek Schuff wrote:
> Yeah i thought about that but the problem is that it breaks the nice ability
to
> chain the << operators to the Errors stream (which is ok if all the tokens are
> strings as they are here, but below some of them are ints). I'm open to ideas
> here.
> On 2013/03/12 22:16:21, jvoung (cr) wrote:
> > Perhaps just combine in one method the adding of an error to the Errors,
with
> > counting up ErrorCount?
>

https://codereview.chromium.org/12449014/diff/7001/lib/Analysis/NaCl/PNaClABI...
File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right):

https://codereview.chromium.org/12449014/diff/7001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:17: #include "llvm/ADT/OwningPtr.h"
where is OwningPtr used?  Was that replaced with the ReporterIsOwned bool?

[Derek Schuff, 2013-03-13 22:52:23]

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right):

https://codereview.chromium.org/12449014/diff/1/lib/Analysis/NaCl/PNaClABIVer...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:89: ErrorCount++;
On 2013/03/13 22:40:09, jvoung (cr) wrote:
> The new version seems good.  If you are going to use <<, then perhaps that
could
> be used more uniformly over the +.
> 
> 
> On 2013/03/13 21:51:01, Derek Schuff wrote:
> > Yeah i thought about that but the problem is that it breaks the nice ability
> to
> > chain the << operators to the Errors stream (which is ok if all the tokens
are
> > strings as they are here, but below some of them are ints). I'm open to
ideas
> > here.
> > On 2013/03/12 22:16:21, jvoung (cr) wrote:
> > > Perhaps just combine in one method the adding of an error to the Errors,
> with
> > > counting up ErrorCount?
> > 
> 

Done.

https://codereview.chromium.org/12449014/diff/7001/lib/Analysis/NaCl/PNaClABI...
File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right):

https://codereview.chromium.org/12449014/diff/7001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:17: #include "llvm/ADT/OwningPtr.h"
Ah yes, removed.
On 2013/03/13 22:40:09, jvoung (cr) wrote:
> where is OwningPtr used?  Was that replaced with the ReporterIsOwned bool?

[jvoung (off chromium), 2013-03-13 23:02:52]

lgtm

[Derek Schuff, 2013-03-13 23:04:10]

Committed patchset #3 manually as rcb0dcd0 (presubmit successful).