Check for metadata in PNaCl ABI checker.

Check for metadata in PNaCl ABI checker.

Disallow all metadata by default.  There is a flag "-allow-debug-metadata",
which will be used in pnacl-ld driver, to not change the debugging workflow.
That flag will not be present in the pnacl-abicheck driver though.

We'll also run -strip-metadata within pnacl-ld, after optimizations are run,
so that at least that part is checked inside pnacl-ld.
CL for driver changes:
https://codereview.chromium.org/14358048/

BUG= http://code.google.com/p/nativeclient/issues/detail?id=3348
R=dschuff@chromium.org

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

[jvoung (off chromium), 2013-04-25 17:30:25]

https://codereview.chromium.org/14329025/diff/1/tools/opt/opt.cpp
File tools/opt/opt.cpp (left):

https://codereview.chromium.org/14329025/diff/1/tools/opt/opt.cpp#oldcode589
tools/opt/opt.cpp:589: initializePNaClABIVerifyFunctionsPass(Registry);
I'm not sure how to pass a flag to these generic initialize*Pass() functions, so
I think we would need to do "Passes.add(createPNaCl...(ErrReporter,
PNaClABIAllowDebug));" below.  Would you prefer extra mods like that, or just
modify pnacl-ld to run "pnacl-abicheck" separately after the expand passes? (see
current driver change).  That does make the linking a little slower, which could
affect autoconf-type things...

[Mark Seaborn, 2013-04-25 18:04:32]

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

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:46:
AllowDebugMetadata(AllowDebugMetadata_) {
I think using a trailing underscore is not LLVM style.  You could write
"AllowDebugMetadata(AllowDebugMetadata)" instead -- it works.

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:56: bool
IsWhiteListedMetadata(unsigned MDKind);
Nit: "IsWhitelisted" ("whitelisting" is one word)

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:209: Reporter->addError()
Fix indentation: remove 2 spaces

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

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:43:
AllowDebugMetadata(AllowDebugMetadata_) {
Same style comment as other file

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:114: return
MD->getName().startswith("llvm.dbg") && AllowDebugMetadata;
Maybe "llvm.dbg." (trailing dot) to be stricter?

https://codereview.chromium.org/14329025/diff/2001/test/NaCl/PNaClABI/types-f...
File test/NaCl/PNaClABI/types-function.ll (right):

https://codereview.chromium.org/14329025/diff/2001/test/NaCl/PNaClABI/types-f...
test/NaCl/PNaClABI/types-function.ll:27: ; CHECK: Function types has disallowed
instruction metadata
Change to "...metadata: !foo" to check reporting of metadata type?

https://codereview.chromium.org/14329025/diff/2001/tools/llc/llc.cpp
File tools/llc/llc.cpp (right):

https://codereview.chromium.org/14329025/diff/2001/tools/llc/llc.cpp#newcode133
tools/llc/llc.cpp:133: //// @LOCALMOD-END
"////" -> "//"

https://codereview.chromium.org/14329025/diff/2001/tools/opt/opt.cpp
File tools/opt/opt.cpp (left):

https://codereview.chromium.org/14329025/diff/2001/tools/opt/opt.cpp#oldcode589
tools/opt/opt.cpp:589: initializePNaClABIVerifyFunctionsPass(Registry);
Why removing from here?  This would break when rolled into pnacl/DEPS since we
are currently using this via 'opt'.

[jvoung (off chromium), 2013-04-25 18:09:33]

https://codereview.chromium.org/14329025/diff/2001/tools/opt/opt.cpp
File tools/opt/opt.cpp (left):

https://codereview.chromium.org/14329025/diff/2001/tools/opt/opt.cpp#oldcode589
tools/opt/opt.cpp:589: initializePNaClABIVerifyFunctionsPass(Registry);
On 2013/04/25 18:04:32, Mark Seaborn wrote:
> Why removing from here?  This would break when rolled into pnacl/DEPS since we
> are currently using this via 'opt'.

See the comment in Patch Set #1.
If I *do* do this I would make the driver change at the same time as the DEPs
roll.

[Mark Seaborn, 2013-04-25 18:11:19]

https://codereview.chromium.org/14329025/diff/1/tools/opt/opt.cpp
File tools/opt/opt.cpp (left):

https://codereview.chromium.org/14329025/diff/1/tools/opt/opt.cpp#oldcode589
tools/opt/opt.cpp:589: initializePNaClABIVerifyFunctionsPass(Registry);
On 2013/04/25 17:30:25, jvoung (cr) wrote:
> I'm not sure how to pass a flag to these generic initialize*Pass() functions,
so
> I think we would need to do "Passes.add(createPNaCl...(ErrReporter,
> PNaClABIAllowDebug));" below.  Would you prefer extra mods like that, or just
> modify pnacl-ld to run "pnacl-abicheck" separately after the expand passes?
(see
> current driver change).  That does make the linking a little slower, which
could
> affect autoconf-type things...

You could have a "static cl::opt<bool>" in PNaClABIVerify*.cpp. 
lib/Transforms/Utils/LowerInvoke.cpp has one for "-enable-correct-eh-support". 
It's icky having a global variable for this, but LLVM seems to like it that way.

Probably you can't have this cl::opt as static if you need this option for two
passes.  I'm not sure this really needs to be two separate passes, though?

[jvoung (off chromium), 2013-04-25 23:22:20]

https://codereview.chromium.org/14329025/diff/1/tools/opt/opt.cpp
File tools/opt/opt.cpp (left):

https://codereview.chromium.org/14329025/diff/1/tools/opt/opt.cpp#oldcode589
tools/opt/opt.cpp:589: initializePNaClABIVerifyFunctionsPass(Registry);
On 2013/04/25 18:11:19, Mark Seaborn wrote:
> On 2013/04/25 17:30:25, jvoung (cr) wrote:
> > I'm not sure how to pass a flag to these generic initialize*Pass()
functions,
> so
> > I think we would need to do "Passes.add(createPNaCl...(ErrReporter,
> > PNaClABIAllowDebug));" below.  Would you prefer extra mods like that, or
just
> > modify pnacl-ld to run "pnacl-abicheck" separately after the expand passes?
> (see
> > current driver change).  That does make the linking a little slower, which
> could
> > affect autoconf-type things...
> 
> You could have a "static cl::opt<bool>" in PNaClABIVerify*.cpp. 
> lib/Transforms/Utils/LowerInvoke.cpp has one for "-enable-correct-eh-support".

> It's icky having a global variable for this, but LLVM seems to like it that
way.
> 
> Probably you can't have this cl::opt as static if you need this option for two
> passes.  I'm not sure this really needs to be two separate passes, though?


I think it's separate for streaming -- one is a module-level pass that never
touches functions, and the other is a function-level pass.  Perhaps they can be
defined in the same file though...

Ended up extern'ing the commandline flag.  There's one case in
include/llvm/CodeGen/MachineScheduler.h "extern cl::opt<bool> ForceBottomUp;"
and a couple cases in lib/Target, but it is rare =/

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

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:46:
AllowDebugMetadata(AllowDebugMetadata_) {
On 2013/04/25 18:04:32, Mark Seaborn wrote:
> I think using a trailing underscore is not LLVM style.  You could write
> "AllowDebugMetadata(AllowDebugMetadata)" instead -- it works.

Hmm it's not that consistent =)  MCAssembler uses underscores, but removing this
argument anyway, and using the commandline flag.

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:56: bool
IsWhiteListedMetadata(unsigned MDKind);
On 2013/04/25 18:04:32, Mark Seaborn wrote:
> Nit: "IsWhitelisted" ("whitelisting" is one word)

Done.

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:209: Reporter->addError()
On 2013/04/25 18:04:32, Mark Seaborn wrote:
> Fix indentation: remove 2 spaces

Done.

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

https://codereview.chromium.org/14329025/diff/2001/lib/Analysis/NaCl/PNaClABI...
lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:114: return
MD->getName().startswith("llvm.dbg") && AllowDebugMetadata;
On 2013/04/25 18:04:32, Mark Seaborn wrote:
> Maybe "llvm.dbg." (trailing dot) to be stricter?

Done.

https://codereview.chromium.org/14329025/diff/2001/test/NaCl/PNaClABI/types-f...
File test/NaCl/PNaClABI/types-function.ll (right):

https://codereview.chromium.org/14329025/diff/2001/test/NaCl/PNaClABI/types-f...
test/NaCl/PNaClABI/types-function.ll:27: ; CHECK: Function types has disallowed
instruction metadata
On 2013/04/25 18:04:32, Mark Seaborn wrote:
> Change to "...metadata: !foo" to check reporting of metadata type?

Not sure what you mean?  What is "...metadata:" ?

[Mark Seaborn, 2013-04-25 23:25:50]

https://codereview.chromium.org/14329025/diff/2001/test/NaCl/PNaClABI/types-f...
File test/NaCl/PNaClABI/types-function.ll (right):

https://codereview.chromium.org/14329025/diff/2001/test/NaCl/PNaClABI/types-f...
test/NaCl/PNaClABI/types-function.ll:27: ; CHECK: Function types has disallowed
instruction metadata
On 2013/04/25 23:22:20, jvoung (cr) wrote:
> On 2013/04/25 18:04:32, Mark Seaborn wrote:
> > Change to "...metadata: !foo" to check reporting of metadata type?
> 
> Not sure what you mean?  What is "...metadata:" ?

I meant you can change it to:
"; CHECK: Function types has disallowed instruction metadata: foo"

[Derek Schuff, 2013-04-26 00:34:34]

https://codereview.chromium.org/14329025/diff/1/tools/opt/opt.cpp
File tools/opt/opt.cpp (left):

https://codereview.chromium.org/14329025/diff/1/tools/opt/opt.cpp#oldcode589
tools/opt/opt.cpp:589: initializePNaClABIVerifyFunctionsPass(Registry);
On 2013/04/25 23:22:20, jvoung (cr) wrote:
The separation is for streaming. There are a few extern flag decls lying around.
Also in our MIPS port, Petar re-used the sfi-foo flags we had for ARM, which is
even worse because they are in different libraries. It's only by accident that
our CMake shared library build successfully links (it doesn't on mac).

https://codereview.chromium.org/14329025/diff/15001/lib/Analysis/NaCl/PNaClAB...
File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):

https://codereview.chromium.org/14329025/diff/15001/lib/Analysis/NaCl/PNaClAB...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:71: return MDKind ==
LLVMContext::MD_dbg && llvm::PNaClABIAllowDebugMetadata;
llvm:: not necessary if using namespace llvm?

[jvoung (off chromium), 2013-04-26 16:51:29]

https://codereview.chromium.org/14329025/diff/2001/test/NaCl/PNaClABI/types-f...
File test/NaCl/PNaClABI/types-function.ll (right):

https://codereview.chromium.org/14329025/diff/2001/test/NaCl/PNaClABI/types-f...
test/NaCl/PNaClABI/types-function.ll:27: ; CHECK: Function types has disallowed
instruction metadata
On 2013/04/25 23:25:50, Mark Seaborn wrote:
> On 2013/04/25 23:22:20, jvoung (cr) wrote:
> > On 2013/04/25 18:04:32, Mark Seaborn wrote:
> > > Change to "...metadata: !foo" to check reporting of metadata type?
> > 
> > Not sure what you mean?  What is "...metadata:" ?
> 
> I meant you can change it to:
> "; CHECK: Function types has disallowed instruction metadata: foo"

Done.

https://codereview.chromium.org/14329025/diff/15001/lib/Analysis/NaCl/PNaClAB...
File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):

https://codereview.chromium.org/14329025/diff/15001/lib/Analysis/NaCl/PNaClAB...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:71: return MDKind ==
LLVMContext::MD_dbg && llvm::PNaClABIAllowDebugMetadata;
On 2013/04/26 00:34:34, Derek Schuff wrote:
> llvm:: not necessary if using namespace llvm?

Done.

[Derek Schuff, 2013-04-26 19:46:26]

lgtm

[jvoung (off chromium), 2013-04-26 21:13:00]

Committed patchset #6 manually as rf8b761e (presubmit successful).