Allow conditional lit tests in Subzero, based on build flags.

Allow conditional lit tests in Subzero, based on build flags.

Adds conditionality to lit tests in two ways:

1) Allows the use of "; REQUIRES: XXX" lines in lit tests. In this
case, the tests defined by the file are only run if all REQUIRES are
met.

2) Allows the conditional running of RUN commands, based on build
flags. This comes in two subforms. There are predefined %ifX commands
that run the command defined by remaining arguments, if the
corresponding %X2i command is applicable. Alternatively, one can use
%if with explicit '--att' arguments to define what conditions should
be checked.

In any case, unlike REQUIRES, the %if commands RUN all the time, but
simply generate empty output, rather then output defined by the
following command, if the condition is not met. These latter tests are
useful when the same input is to be tested under different conditions,
since the REQUIRES form does not allow this.

Note that m2i, p2i, l2i, and lc2i are also conditionally controlled,
so that they do nothing if the build did not construct the appropriate
Subzero translator.

This CL replaces https://codereview.chromium.org/644143002


BUG=None
R=jvoung@chromium.org, stichnot@chromium.org

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

[Karl, 2014-10-14 22:54:31]

kschimpf@google.com changed reviewers:
+ jvoung@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2014-10-15 03:16:37]

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone#newcode44
Makefile.standalone:44: OBJDIR := build/min
It looks like this creates subdirs build/Release and build/Release+Asserts, but
then build/min/Release and build/min/Release+Asserts etc.

Can we keep it flat, like build/Release+Min, build/Release+Asserts+Min, etc., or
something like that?

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone#newcode45
Makefile.standalone:45: BUILD_ATTS := $(BUILD_ATTS) minimal p2i
I think the logic of "minimal" is backwards.

None of the tests strictly require the minimal build.  However, some of them
will require the non-minimal build.

Should we instead be adding a nonminimal attribute to the non-minimal builds.

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone#newcode91
Makefile.standalone:91: CXXFLAGS :=$(CXXFLAGS) -DSZ_MINIMAL
I started thinking that maybe instead of a single define like SZ_MINIMAL, we
should have separate flags, e.g.
  -DNDEBUG
  -DNO_TEXT_ASM
  -DNO_DUMP
  -DNO_LLVM_CL
etc.

But maybe -DSZ_MINIMAL -DNDEBUG is a reasonable placeholder for now.

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone#newcode172
Makefile.standalone:172: rm -rf llvm2ice llvm2ice.build_atts *.o $(OBJDIR)
Shouldn't "make clean" remove the entire build/ directory, instead of
potentially having to run 8 different "make clean" variants?

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py
File pydir/buildatts.py (right):

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode9
pydir/buildatts.py:9: def getBuildAttributes(Filename):
Hmm, I don't know what our name capitalization style is supposed to be, but:
  git grep -w def -- '*.py'
shows a number of ThisCapsStyle but no examples of thisCapsStyle.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode13
pydir/buildatts.py:13: f = open(Filename, 'r')
with open(Filename, 'r') as f:
  return f.read().split()

then f.close() happens automatically.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode18
pydir/buildatts.py:18: def hasBuildAttributes(Filename, Attributes):
These functions ought to have docstrings.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode20
pydir/buildatts.py:20: for Att in Attributes:
return set(Attributes) <= set(BuildAtts)

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode26
pydir/buildatts.py:26: """Run check on corresponding ice executable .kind file
to see if
What is a .kind file?

llvm2ice executable?

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode41
pydir/buildatts.py:41: help='Trace command that run in separate shell')
commands

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode48
pydir/buildatts.py:48: if args.cmd is None:
if not args.cmd:

(would this work?)

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode49
pydir/buildatts.py:49: return
Should an explicit zero or nonzero value be returned to the shell?

[Karl, 2014-10-15 20:12:32]

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone#newcode44
Makefile.standalone:44: OBJDIR := build/min
On 2014/10/15 03:16:36, stichnot wrote:
> It looks like this creates subdirs build/Release and build/Release+Asserts,
but
> then build/min/Release and build/min/Release+Asserts etc.
> 
> Can we keep it flat, like build/Release+Min, build/Release+Asserts+Min, etc.,
or
> something like that?

Done.

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone#newcode45
Makefile.standalone:45: BUILD_ATTS := $(BUILD_ATTS) minimal p2i
On 2014/10/15 03:16:36, stichnot wrote:
> I think the logic of "minimal" is backwards.
> 
> None of the tests strictly require the minimal build.  However, some of them
> will require the non-minimal build.
> 
> Should we instead be adding a nonminimal attribute to the non-minimal builds.

A better choice would be to put "minimal" when minimal, and "nonminimal"
otherwise. That way, there is always a build attribute to check. Changing to
meet this choice.

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone#newcode91
Makefile.standalone:91: CXXFLAGS :=$(CXXFLAGS) -DSZ_MINIMAL
On 2014/10/15 03:16:36, stichnot wrote:
> I started thinking that maybe instead of a single define like SZ_MINIMAL, we
> should have separate flags, e.g.
>   -DNDEBUG
>   -DNO_TEXT_ASM
>   -DNO_DUMP
>   -DNO_LLVM_CL
> etc.
> 
> But maybe -DSZ_MINIMAL -DNDEBUG is a reasonable placeholder for now.

Went ahead and added multiple flags for corresponding concepts.

https://codereview.chromium.org/659513005/diff/1/Makefile.standalone#newcode172
Makefile.standalone:172: rm -rf llvm2ice llvm2ice.build_atts *.o $(OBJDIR)
On 2014/10/15 03:16:36, stichnot wrote:
> Shouldn't "make clean" remove the entire build/ directory, instead of
> potentially having to run 8 different "make clean" variants?

Added "clean-all" to clean all.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py
File pydir/buildatts.py (right):

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode9
pydir/buildatts.py:9: def getBuildAttributes(Filename):
On 2014/10/15 03:16:37, stichnot wrote:
> Hmm, I don't know what our name capitalization style is supposed to be, but:
>   git grep -w def -- '*.py'
> shows a number of ThisCapsStyle but no examples of thisCapsStyle.

Done.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode13
pydir/buildatts.py:13: f = open(Filename, 'r')
On 2014/10/15 03:16:37, stichnot wrote:
> with open(Filename, 'r') as f:
>   return f.read().split()
> 
> then f.close() happens automatically.

Done.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode18
pydir/buildatts.py:18: def hasBuildAttributes(Filename, Attributes):
On 2014/10/15 03:16:37, stichnot wrote:
> These functions ought to have docstrings.

Done.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode20
pydir/buildatts.py:20: for Att in Attributes:
On 2014/10/15 03:16:36, stichnot wrote:
> return set(Attributes) <= set(BuildAtts)

Done.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode26
pydir/buildatts.py:26: """Run check on corresponding ice executable .kind file
to see if
On 2014/10/15 03:16:37, stichnot wrote:
> What is a .kind file?
> 
> llvm2ice executable?

Simplified python script to just be checking if a set of attributes (i.e. names)
appears in a file.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode41
pydir/buildatts.py:41: help='Trace command that run in separate shell')
On 2014/10/15 03:16:37, stichnot wrote:
> commands

Done.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode48
pydir/buildatts.py:48: if args.cmd is None:
On 2014/10/15 03:16:37, stichnot wrote:
> if not args.cmd:
> 
> (would this work?)

Done.

https://codereview.chromium.org/659513005/diff/1/pydir/buildatts.py#newcode49
pydir/buildatts.py:49: return
On 2014/10/15 03:16:37, stichnot wrote:
> Should an explicit zero or nonzero value be returned to the shell?

Done.

[jvoung (off chromium), 2014-10-15 21:57:06]

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone#newc...
Makefile.standalone:56: BUILD_ATTS := $(BUILD_ATTS) minimal p2i
It seems like the builds should always have "p2i" available, since that is to
become the default, so I'm not sure that's worth listing (or prefixing FileCheck
with ifp)?

The difference is more "minimal" vs "nonminimal", and the availability of l2i
and lc2i.

Should a test actually require "nonminimal" vs "minimal" instead of requiring
"l2i" or "lc2i"? Maybe if it's checking for some other misc feature like "dump",
but that could be listed too?

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode13
pydir/ifatts.py:13: return []
Should this ever happen, or should it be considered a user error?

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode17
pydir/ifatts.py:17: def hasFileAttributes(Filename, Attributes):
HasFileAttributes

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode20
pydir/ifatts.py:20: """
I think the usual style is to line up the comments like this:

"""One liner.

More stuff.
Additional stuff.
"""

So, like you had with GetFileAttributes, the ending """ is lined up with the
beginning """.

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode48
pydir/ifatts.py:48: return 0
Should this ever happen, or should it be considered a user error?

https://codereview.chromium.org/659513005/diff/40001/tests_lit/lit.cfg
File tests_lit/lit.cfg (right):

https://codereview.chromium.org/659513005/diff/40001/tests_lit/lit.cfg#newcode7
tests_lit/lit.cfg:7: #   m : Run when llvm2ice corresponds to minimal Subzero
Clarify "minimal Subzero... building ICE from PNaCl bitcode directly." That is,
it can't be mixed with "lc" or "l".

https://codereview.chromium.org/659513005/diff/40001/tests_lit/lit.cfg#newcode80
tests_lit/lit.cfg:80: config.substitutions.append(('%m2i', ' '.join(ifm_atts_cmd
+ llvm2ice_cmd)))
a bit of bikeshedding here:

m2i seems to indicate that there is input of the type "m", but the input is
still like "p2i". Maybe we don't need an ifm though? When will something only
work with the minimal build, but not a fuller build?

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/fp.pnacl.ll (right):

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/fp.pnacl.ll:10: ; REQUIRES: x86-32
Any reason this is considered REQUIRES: x86-32 vs say, ebp_args.ll ?

This one seems like it could be extended to have more arches, while the ebp
sounds more specific to x86-32 (though could apply to anything with a frame
pointer, with some changes).

Which part of the CL enables the REQUIRES: syntax?

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/globalrelocs.ll (right):

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/globalrelocs.ll:7: ; RUN: %p2i -i %s --args -verbose
inst | %ifp FileCheck %s
See earlier question -- should p2i always work (vs conditionalizing with ifp)?

[Jim Stichnoth, 2014-10-15 22:23:55]

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone#newc...
Makefile.standalone:54: ASSERTIONS := $(ASSERTIONS) -DNO_TEXT_ASM -DNO_DUMP
-DNO_LLVM_CL
Maybe name this DEFINES instead of ASSERTIONS, or some other more standard
makefile name?

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode48
pydir/ifatts.py:48: return 0
All but the last return should probably be "return 1".

Also, document in the docstring that it returns (to the shell) 0 on success and
1 on failure.

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode61
pydir/ifatts.py:61: main()
indent 2 spaces

[Karl, 2014-10-16 16:51:49]

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone#newc...
Makefile.standalone:54: ASSERTIONS := $(ASSERTIONS) -DNO_TEXT_ASM -DNO_DUMP
-DNO_LLVM_CL
On 2014/10/15 22:23:54, stichnot wrote:
> Maybe name this DEFINES instead of ASSERTIONS, or some other more standard
> makefile name?

Refactored to use CXX_DEFINES.

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone#newc...
Makefile.standalone:56: BUILD_ATTS := $(BUILD_ATTS) minimal p2i
On 2014/10/15 21:57:05, jvoung wrote:
> It seems like the builds should always have "p2i" available, since that is to
> become the default, so I'm not sure that's worth listing (or prefixing
FileCheck
> with ifp)?
> 
> The difference is more "minimal" vs "nonminimal", and the availability of l2i
> and lc2i.
> 
> Should a test actually require "nonminimal" vs "minimal" instead of requiring
> "l2i" or "lc2i"? Maybe if it's checking for some other misc feature like
"dump",
> but that could be listed too?

I tried to make the decision logic needed for tests to be simple. However, I
guess we only need to put that in test_lits/lit.cfg.
Dropping p2i, l2i, and lc2i.

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode13
pydir/ifatts.py:13: return []
On 2014/10/15 21:57:05, jvoung wrote:
> Should this ever happen, or should it be considered a user error?

Rethinking this, I agree this should be an error.

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode17
pydir/ifatts.py:17: def hasFileAttributes(Filename, Attributes):
On 2014/10/15 21:57:05, jvoung wrote:
> HasFileAttributes

Done.

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode20
pydir/ifatts.py:20: """
On 2014/10/15 21:57:05, jvoung wrote:
> I think the usual style is to line up the comments like this:
> 
> """One liner.
> 
> More stuff.
> Additional stuff.
> """
> 
> So, like you had with GetFileAttributes, the ending """ is lined up with the
> beginning """.

Done.

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode48
pydir/ifatts.py:48: return 0
On 2014/10/15 21:57:05, jvoung wrote:
> Should this ever happen, or should it be considered a user error?

Now that I have simplified this python file, and --command is more explicit, I
agree that this should be an error. Modifying.

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode48
pydir/ifatts.py:48: return 0
On 2014/10/15 22:23:54, stichnot wrote:
> All but the last return should probably be "return 1".
> 
> Also, document in the docstring that it returns (to the shell) 0 on success
and
> 1 on failure.

Actually, the last 2 should return 0! The conditional test is defined by the
second return, and should succeed with no output in this case.

The first return I have replaced with a runtime exception so that the failure is
cleanly reported.

https://codereview.chromium.org/659513005/diff/40001/pydir/ifatts.py#newcode61
pydir/ifatts.py:61: main()
On 2014/10/15 22:23:54, stichnot wrote:
> indent 2 spaces

Done.

https://codereview.chromium.org/659513005/diff/40001/tests_lit/lit.cfg
File tests_lit/lit.cfg (right):

https://codereview.chromium.org/659513005/diff/40001/tests_lit/lit.cfg#newcode7
tests_lit/lit.cfg:7: #   m : Run when llvm2ice corresponds to minimal Subzero
On 2014/10/15 21:57:06, jvoung wrote:
> Clarify "minimal Subzero... building ICE from PNaCl bitcode directly." That
is,
> it can't be mixed with "lc" or "l".

I wanted to keep the usage simple, by not forcing people to think about what
attributes mean what. If you start the line with %X2i,
then any piped results either accept empty input, or should be
preceded with a %ifX.

Also rewrote the comment to (hopefully) be clearer.

I agree that I should remove 'm' from this list. Also added more comments.

https://codereview.chromium.org/659513005/diff/40001/tests_lit/lit.cfg#newcode80
tests_lit/lit.cfg:80: config.substitutions.append(('%m2i', ' '.join(ifm_atts_cmd
+ llvm2ice_cmd)))
On 2014/10/15 21:57:06, jvoung wrote:
> a bit of bikeshedding here:
> 
> m2i seems to indicate that there is input of the type "m", but the input is
> still like "p2i". Maybe we don't need an ifm though? When will something only
> work with the minimal build, but not a fuller build?
> 

Correct. p2i runs in any case. Clearing this up by dropping %m2i and %ifm.

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/fp.pnacl.ll (right):

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/fp.pnacl.ll:10: ; REQUIRES: x86-32
On 2014/10/15 21:57:06, jvoung wrote:
> Any reason this is considered REQUIRES: x86-32 vs say, ebp_args.ll ?
> 
> This one seems like it could be extended to have more arches, while the ebp
> sounds more specific to x86-32 (though could apply to anything with a frame
> pointer, with some changes).
> 
> Which part of the CL enables the REQUIRES: syntax?

The point of this change was to show that requires works. Since it appears
confusing, I will remove it. However, the point is that in
Subzero/tests_lit/lit.cfg (line 74) updates config.available_features to include
all (target) build attributes. The syntax for REQUIRES syntax is always enabled,
and uses the available_features attribute to define what symbols can be used in
REQUIRES.

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/globalrelocs.ll (right):

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/globalrelocs.ll:7: ; RUN: %p2i -i %s --args -verbose
inst | %ifp FileCheck %s
On 2014/10/15 21:57:06, jvoung wrote:
> See earlier question -- should p2i always work (vs conditionalizing with ifp)?

See comments at the beginning of lit.cfg. I clarified the use of %p2i and %ifp,
including that it is not necessary currently. I also mention that it is
currently implemented as a nop.

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg
File tests_lit/lit.cfg (right):

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg#newcode74
tests_lit/lit.cfg:74:
config.available_features.update(GetFileAttributes(llvm2icetoolatts))
Here is the code that defines what symbols will be allowed for a REQUIRES
clause.

[Jim Stichnoth, 2014-10-20 16:44:29]

https://codereview.chromium.org/659513005/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/60001/Makefile.standalone#newc...
Makefile.standalone:49: BUILD_ATTS := $(BUILD_ATTS) debug
I don't know if this is possible, but I wonder if += is workable for building up
BUILD_ATTS.

https://codereview.chromium.org/659513005/diff/60001/Makefile.standalone#newc...
Makefile.standalone:65: ASSERTIONS = -DNDEBUG
Add noasserts to BUILD_ATTS, for flexibility and consistency?

https://codereview.chromium.org/659513005/diff/60001/Makefile.standalone#newc...
Makefile.standalone:125: rm -f llvm2ice llvm2ice.build_atts
Can you change to rm -rf in case a directory of this name somehow gets in there?

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode9
pydir/ifatts.py:9: def GetFileAttributes(Filename):
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html

Local variables, including parameters, get the lower_with_under naming style.

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode18
pydir/ifatts.py:18: """Returns true if the set of names in Attributes also
appear
I thought Jan already mentioned this, but multi-line docstrings should have a
single summary line, followed by a blank line, followed by the extended
description.

http://legacy.python.org/dev/peps/pep-0257/#multi-line-docstrings

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg
File tests_lit/lit.cfg (right):

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg#newcode16
tests_lit/lit.cfg:16: # These commands can be used in RUN lines by FileCheck. If
the Subzero build,
How about something like this:

If the Subzero build being tested lacks any required attributes (e.g., the
ability to parse .ll files), the command ...

[jvoung (off chromium), 2014-10-20 17:36:00]

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone#newc...
Makefile.standalone:56: BUILD_ATTS := $(BUILD_ATTS) minimal p2i
On 2014/10/16 16:51:48, Karl wrote:
> On 2014/10/15 21:57:05, jvoung wrote:
> > It seems like the builds should always have "p2i" available, since that is
to
> > become the default, so I'm not sure that's worth listing (or prefixing
> FileCheck
> > with ifp)?
> > 
> > The difference is more "minimal" vs "nonminimal", and the availability of
l2i
> > and lc2i.
> > 
> > Should a test actually require "nonminimal" vs "minimal" instead of
requiring
> > "l2i" or "lc2i"? Maybe if it's checking for some other misc feature like
> "dump",
> > but that could be listed too?
> 
> I tried to make the decision logic needed for tests to be simple. However, I
> guess we only need to put that in test_lits/lit.cfg.
> Dropping p2i, l2i, and lc2i.

Ah, what I meant was actually that a list of features might make more sense than
"minimal vs nonminimal", but there is a baseline set of features that always
exist which you shouldn't need to list in tests (e.g., p2i). So, kind of like
what you had before =( Sorry for the confusion.

Maybe something baseline like p2i could be listed here in the BUILD_ATTS, but
I'm not sure you want to make every test check for it via %ifp. It could be
broadly checked for a whole set of tests in a lit.cfg file without as many files
to change I guess.

My thought on why features instead of "nonminimal": it makes it more clear what
features are part of "nonminimal", and makes it clear which feature of
nonminimal the test is requiring. You've done that in the lit.cfg layer by
translating between this and the various %if forms.

On the other hand, maybe there is an argument for "minimal": I can imagine there
might be a test that tried to run "llvm2ice --verbose foo", and was expecting
that this *fail* because it is actually pruned out. There might be other ways to
check minimalness though. E.g., using features, but inverting the condition on
the %if?

Thoughts?

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/fp.pnacl.ll (right):

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/fp.pnacl.ll:10: ; REQUIRES: x86-32
On 2014/10/16 16:51:48, Karl wrote:
> On 2014/10/15 21:57:06, jvoung wrote:
> > Any reason this is considered REQUIRES: x86-32 vs say, ebp_args.ll ?
> > 
> > This one seems like it could be extended to have more arches, while the ebp
> > sounds more specific to x86-32 (though could apply to anything with a frame
> > pointer, with some changes).
> > 
> > Which part of the CL enables the REQUIRES: syntax?
> 
> The point of this change was to show that requires works. Since it appears
> confusing, I will remove it. However, the point is that in
> Subzero/tests_lit/lit.cfg (line 74) updates config.available_features to
include
> all (target) build attributes. The syntax for REQUIRES syntax is always
enabled,
> and uses the available_features attribute to define what symbols can be used
in
> REQUIRES.


A good example might be tests_lit/llvm2ice_tests/addr-opt-multi-def-var.ll. That
requires some logging from "--verbose addropt" to work. I guess that could also
be done with the %if.

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode29
pydir/ifatts.py:29: """
could line this """ up too

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode58
pydir/ifatts.py:58: main()
This should be "sys.exit(main())", to use main's return value?

I guess right now it always returns 0, so main() could just not return anything.
Either propagate or not?

[Jim Stichnoth, 2014-10-20 18:06:03]

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg
File tests_lit/lit.cfg (right):

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg#newcode79
tests_lit/lit.cfg:79: ifnm_atts_cmd = if_atts_cmd + ['--att=nonminal',
'--command']
nonminimal

[Karl, 2014-10-20 21:00:43]

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/40001/Makefile.standalone#newc...
Makefile.standalone:56: BUILD_ATTS := $(BUILD_ATTS) minimal p2i
On 2014/10/20 17:36:00, jvoung wrote:
> On 2014/10/16 16:51:48, Karl wrote:
> > On 2014/10/15 21:57:05, jvoung wrote:
> > > It seems like the builds should always have "p2i" available, since that is
> to
> > > become the default, so I'm not sure that's worth listing (or prefixing
> > FileCheck
> > > with ifp)?
> > > 
> > > The difference is more "minimal" vs "nonminimal", and the availability of
> l2i
> > > and lc2i.
> > > 
> > > Should a test actually require "nonminimal" vs "minimal" instead of
> requiring
> > > "l2i" or "lc2i"? Maybe if it's checking for some other misc feature like
> > "dump",
> > > but that could be listed too?
> > 
> > I tried to make the decision logic needed for tests to be simple. However, I
> > guess we only need to put that in test_lits/lit.cfg.
> > Dropping p2i, l2i, and lc2i.
> 
> Ah, what I meant was actually that a list of features might make more sense
than
> "minimal vs nonminimal", but there is a baseline set of features that always
> exist which you shouldn't need to list in tests (e.g., p2i). So, kind of like
> what you had before =( Sorry for the confusion.
> 
> Maybe something baseline like p2i could be listed here in the BUILD_ATTS, but
> I'm not sure you want to make every test check for it via %ifp. It could be
> broadly checked for a whole set of tests in a lit.cfg file without as many
files
> to change I guess.
> 
> My thought on why features instead of "nonminimal": it makes it more clear
what
> features are part of "nonminimal", and makes it clear which feature of
> nonminimal the test is requiring. You've done that in the lit.cfg layer by
> translating between this and the various %if forms.
> 
> On the other hand, maybe there is an argument for "minimal": I can imagine
there
> might be a test that tried to run "llvm2ice --verbose foo", and was expecting
> that this *fail* because it is actually pruned out. There might be other ways
to
> check minimalness though. E.g., using features, but inverting the condition on
> the %if?
> 
> Thoughts?

Talked with Jim, and decided that:

1) We assume that p2i is always built, and hence not an attribute.

2) Removed minimal/nonminimal attributes, since they can too easily change with
time.

3) Added l2i and lc2i to BUILD_ATTS if not MINIMAL.

4) Added -Dno_llvm_ir to CXX_DEFINES.

5) Added to BUILD_ATTS, flags of form: no_XXX and allow_XXX for each ifdef (i.e.
XXX in {text_asm, dump, llvm_cl, and llvm_ir}).

We also discussed modifying llvm2ice.cpp to check ifdefs and produce the
corresponding set of build attributes, and complain with error if for some XX,
neither allow_XXX or no_XXX is defined. Then modify iffatts to use this. This
will get rid of the corresponding llvm2ice.build_atts file. However, it was much
harder than I initially thought to fix ifatts.py. Hence, I have only implemented
the llvm2ice.cpp changes. Fixing ifatts.py to use the executable for build
attributes will be done in a later CL.

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
File tests_lit/llvm2ice_tests/fp.pnacl.ll (right):

https://codereview.chromium.org/659513005/diff/40001/tests_lit/llvm2ice_tests...
tests_lit/llvm2ice_tests/fp.pnacl.ll:10: ; REQUIRES: x86-32
On 2014/10/20 17:36:00, jvoung wrote:
> On 2014/10/16 16:51:48, Karl wrote:
> > On 2014/10/15 21:57:06, jvoung wrote:
> > > Any reason this is considered REQUIRES: x86-32 vs say, ebp_args.ll ?
> > > 
> > > This one seems like it could be extended to have more arches, while the
ebp
> > > sounds more specific to x86-32 (though could apply to anything with a
frame
> > > pointer, with some changes).
> > > 
> > > Which part of the CL enables the REQUIRES: syntax?
> > 
> > The point of this change was to show that requires works. Since it appears
> > confusing, I will remove it. However, the point is that in
> > Subzero/tests_lit/lit.cfg (line 74) updates config.available_features to
> include
> > all (target) build attributes. The syntax for REQUIRES syntax is always
> enabled,
> > and uses the available_features attribute to define what symbols can be used
> in
> > REQUIRES.
> 
> 
> A good example might be tests_lit/llvm2ice_tests/addr-opt-multi-def-var.ll.
That
> requires some logging from "--verbose addropt" to work. I guess that could
also
> be done with the %if.

Added "REQUIRES: x86-32" to test file, since it is part of the x8632 lowering
code.

https://codereview.chromium.org/659513005/diff/60001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/60001/Makefile.standalone#newc...
Makefile.standalone:49: BUILD_ATTS := $(BUILD_ATTS) debug
On 2014/10/20 16:44:28, stichnot wrote:
> I don't know if this is possible, but I wonder if += is workable for building
up
> BUILD_ATTS.

Done.

https://codereview.chromium.org/659513005/diff/60001/Makefile.standalone#newc...
Makefile.standalone:65: ASSERTIONS = -DNDEBUG
On 2014/10/20 16:44:28, stichnot wrote:
> Add noasserts to BUILD_ATTS, for flexibility and consistency?

Done.

https://codereview.chromium.org/659513005/diff/60001/Makefile.standalone#newc...
Makefile.standalone:125: rm -f llvm2ice llvm2ice.build_atts
On 2014/10/20 16:44:28, stichnot wrote:
> Can you change to rm -rf in case a directory of this name somehow gets in
there?

Done.

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode9
pydir/ifatts.py:9: def GetFileAttributes(Filename):
On 2014/10/20 16:44:28, stichnot wrote:
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html
> 
> Local variables, including parameters, get the lower_with_under naming style.

Done.

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode18
pydir/ifatts.py:18: """Returns true if the set of names in Attributes also
appear
On 2014/10/20 16:44:28, stichnot wrote:
> I thought Jan already mentioned this, but multi-line docstrings should have a
> single summary line, followed by a blank line, followed by the extended
> description.
> 
> http://legacy.python.org/dev/peps/pep-0257/#multi-line-docstrings

Done.

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode29
pydir/ifatts.py:29: """
On 2014/10/20 17:36:00, jvoung wrote:
> could line this """ up too

Done.

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode58
pydir/ifatts.py:58: main()
On 2014/10/20 17:36:00, jvoung wrote:
> This should be "sys.exit(main())", to use main's return value?
> 
> I guess right now it always returns 0, so main() could just not return
anything.
> Either propagate or not?

Ok. Chose the latter.

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg
File tests_lit/lit.cfg (right):

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg#newcode16
tests_lit/lit.cfg:16: # These commands can be used in RUN lines by FileCheck. If
the Subzero build,
On 2014/10/20 16:44:29, stichnot wrote:
> How about something like this:
> 
> If the Subzero build being tested lacks any required attributes (e.g., the
> ability to parse .ll files), the command ...

Done.

https://codereview.chromium.org/659513005/diff/60001/tests_lit/lit.cfg#newcode79
tests_lit/lit.cfg:79: ifnm_atts_cmd = if_atts_cmd + ['--att=nonminal',
'--command']
On 2014/10/20 18:06:03, stichnot wrote:
> nonminimal

Removed, since we no longer treat minimal/nonminimal as an attribute.

[jvoung (off chromium), 2014-10-21 19:12:01]

Otherwise looks pretty good

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode58
pydir/ifatts.py:58: main()
On 2014/10/20 21:00:42, Karl wrote:
> On 2014/10/20 17:36:00, jvoung wrote:
> > This should be "sys.exit(main())", to use main's return value?
> > 
> > I guess right now it always returns 0, so main() could just not return
> anything.
> > Either propagate or not?
> 
> Ok. Chose the latter.

Can you upload the new copy?

https://codereview.chromium.org/659513005/diff/100001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/659513005/diff/100001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:184: #elif !defined NO_DUMP && defined ALLOW_TEXT_ASM
NO_TEXT_ASM instead of NO_DUMP?

Maybe there can be a macro that systematically emits these checks and prints to
the stream?

https://codereview.chromium.org/659513005/diff/100001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:196: #elif defined DO_DUMP
DO_DUMP -> NO_DUMP?

https://codereview.chromium.org/659513005/diff/100001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:252: 
could squash it to one newline instead of two

https://codereview.chromium.org/659513005/diff/100001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-atomic-fence-all.ll (right):

https://codereview.chromium.org/659513005/diff/100001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-atomic-fence-all.ll:6: ; TODO(kschimpf) Find out
why lc2i is needed.
Sorry I think I changed this one...

[Karl, 2014-10-21 21:14:18]

Note: On tests that only used %lc2i, a removed the %iflc and added a
corresponding REQUIRES clause.

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode58
pydir/ifatts.py:58: main()
On 2014/10/21 19:12:00, jvoung wrote:
> On 2014/10/20 21:00:42, Karl wrote:
> > On 2014/10/20 17:36:00, jvoung wrote:
> > > This should be "sys.exit(main())", to use main's return value?
> > > 
> > > I guess right now it always returns 0, so main() could just not return
> > anything.
> > > Either propagate or not?
> > 
> > Ok. Chose the latter.
> 
> Can you upload the new copy?

Sorry about that. Apparently, I modified it after uploading it. It should be
there now.

https://codereview.chromium.org/659513005/diff/100001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/100001/Makefile.standalone#new...
Makefile.standalone:58: -DNO_LLVM_IR_AS_INPUT
Decided to always define the macros, and set to 0/1 depending on feature. Only
allowing the ALLOW_XXX form. This means that we don't have to test the
exclusivity of NO_XXX and ALLOW_XXX. It also means
that we can simplify the generation of the build attributes using a support
function.

Also, modified to generate llvm2ice.build_atts from executable, rather than
repeating logic here.

https://codereview.chromium.org/659513005/diff/100001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/659513005/diff/100001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:184: #elif !defined NO_DUMP && defined ALLOW_TEXT_ASM
On 2014/10/21 19:12:00, jvoung wrote:
> NO_TEXT_ASM instead of NO_DUMP?
> 
> Maybe there can be a macro that systematically emits these checks and prints
to
> the stream?

After simplifying the define flags (see Makefile.standalone), I was able to more
systematically handle the generation/verification of build attributes.

https://codereview.chromium.org/659513005/diff/100001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:196: #elif defined DO_DUMP
On 2014/10/21 19:12:00, jvoung wrote:
> DO_DUMP -> NO_DUMP?

Done.

https://codereview.chromium.org/659513005/diff/100001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:252: 
On 2014/10/21 19:12:00, jvoung wrote:
> could squash it to one newline instead of two

Done.

https://codereview.chromium.org/659513005/diff/100001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-atomic-fence-all.ll (right):

https://codereview.chromium.org/659513005/diff/100001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-atomic-fence-all.ll:6: ; TODO(kschimpf) Find out
why lc2i is needed.
On 2014/10/21 19:12:01, jvoung wrote:
> Sorry I think I changed this one...

Acknowledged.

[Jim Stichnoth, 2014-10-21 21:58:42]

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode18
pydir/ifatts.py:18: """Returns true if the set of names in Attributes also
appear
On 2014/10/20 21:00:42, Karl wrote:
> On 2014/10/20 16:44:28, stichnot wrote:
> > I thought Jan already mentioned this, but multi-line docstrings should have
a
> > single summary line, followed by a blank line, followed by the extended
> > description.
> > 
> > http://legacy.python.org/dev/peps/pep-0257/#multi-line-docstrings
> 
> Done.

Not done, please do...

https://codereview.chromium.org/659513005/diff/140001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/140001/Makefile.standalone#new...
Makefile.standalone:129: $@ --build-atts > $@.build_atts
It would be good to leave a TODO about removing the .build_atts file and
directly using llvm2ice instead.

https://codereview.chromium.org/659513005/diff/140001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/140001/pydir/ifatts.py#newcode27
pydir/ifatts.py:27: arguments
end with a period.

https://codereview.chromium.org/659513005/diff/140001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/659513005/diff/140001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:191: // TODO(kschimpf): Conditionalize this once we have
multiple targets.
After the GlobalContext object is created, you could actually do something like

  *Stream << Ctx.getTarget()->getArchAttr() << "\n";

Either add this to the TODO, or implement it. :)

[Karl, 2014-10-22 17:03:36]

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/60001/pydir/ifatts.py#newcode18
pydir/ifatts.py:18: """Returns true if the set of names in Attributes also
appear
On 2014/10/21 21:58:41, stichnot wrote:
> On 2014/10/20 21:00:42, Karl wrote:
> > On 2014/10/20 16:44:28, stichnot wrote:
> > > I thought Jan already mentioned this, but multi-line docstrings should
have
> a
> > > single summary line, followed by a blank line, followed by the extended
> > > description.
> > > 
> > > http://legacy.python.org/dev/peps/pep-0257/#multi-line-docstrings
> > 
> > Done.
> 
> Not done, please do...

Done.

https://codereview.chromium.org/659513005/diff/140001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/659513005/diff/140001/Makefile.standalone#new...
Makefile.standalone:129: $@ --build-atts > $@.build_atts
On 2014/10/21 21:58:41, stichnot wrote:
> It would be good to leave a TODO about removing the .build_atts file and
> directly using llvm2ice instead.

Done.

https://codereview.chromium.org/659513005/diff/140001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/140001/pydir/ifatts.py#newcode27
pydir/ifatts.py:27: arguments
On 2014/10/21 21:58:41, stichnot wrote:
> end with a period.

Done.

https://codereview.chromium.org/659513005/diff/140001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/659513005/diff/140001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:191: // TODO(kschimpf): Conditionalize this once we have
multiple targets.
On 2014/10/21 21:58:41, stichnot wrote:
> After the GlobalContext object is created, you could actually do something
like
> 
>   *Stream << Ctx.getTarget()->getArchAttr() << "\n";
> 
> Either add this to the TODO, or implement it. :)

Done.

[Jim Stichnoth, 2014-10-22 18:09:21]

lgtm

https://codereview.chromium.org/659513005/diff/160001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/160001/pydir/ifatts.py#newcode24
pydir/ifatts.py:24: """Run command only if attributes are defined.
maybe "Run a particular command" or "Run the specified command"?

https://codereview.chromium.org/659513005/diff/160001/pydir/ifatts.py#newcode26
pydir/ifatts.py:26: Check if the set attributes (i.e. names), contained in FILE,
"set of attributes", right?

https://codereview.chromium.org/659513005/diff/160001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/659513005/diff/160001/src/IceTypes.def#newcode21
src/IceTypes.def:21: X(Target_X8664, "x86-b4")                                  
           \
b4 --> 64

https://codereview.chromium.org/659513005/diff/160001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/659513005/diff/160001/src/IceTypes.h#newcode38
src/IceTypes.h:38: 
I don't think either "const" in "const TargetArch" is necessary.

[jvoung (off chromium), 2014-10-22 19:09:54]

LGTM too

https://codereview.chromium.org/659513005/diff/160001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/phi.ll (right):

https://codereview.chromium.org/659513005/diff/160001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/phi.ll:11: ; RUN: %lc2i -i %s --insts | %iflc %szdiff
%s | FileCheck --check-prefix=DUMP %s
don't need this iflc?

[Karl, 2014-10-27 21:24:22]

Sent out for review because I updated Makefile to handle building subzero with
toolchain_build_pnacl.py

https://codereview.chromium.org/659513005/diff/160001/pydir/ifatts.py
File pydir/ifatts.py (right):

https://codereview.chromium.org/659513005/diff/160001/pydir/ifatts.py#newcode24
pydir/ifatts.py:24: """Run command only if attributes are defined.
On 2014/10/22 18:09:20, stichnot wrote:
> maybe "Run a particular command" or "Run the specified command"?

Done.

https://codereview.chromium.org/659513005/diff/160001/pydir/ifatts.py#newcode26
pydir/ifatts.py:26: Check if the set attributes (i.e. names), contained in FILE,
On 2014/10/22 18:09:20, stichnot wrote:
> "set of attributes", right?

Done.

https://codereview.chromium.org/659513005/diff/160001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/659513005/diff/160001/src/IceTypes.def#newcode21
src/IceTypes.def:21: X(Target_X8664, "x86-b4")                                  
           \
On 2014/10/22 18:09:20, stichnot wrote:
> b4 --> 64

Done.

https://codereview.chromium.org/659513005/diff/160001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/659513005/diff/160001/src/IceTypes.h#newcode38
src/IceTypes.h:38: 
On 2014/10/22 18:09:21, stichnot wrote:
> I don't think either "const" in "const TargetArch" is necessary.

Done.

https://codereview.chromium.org/659513005/diff/160001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/phi.ll (right):

https://codereview.chromium.org/659513005/diff/160001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/phi.ll:11: ; RUN: %lc2i -i %s --insts | %iflc %szdiff
%s | FileCheck --check-prefix=DUMP %s
On 2014/10/22 19:09:53, jvoung wrote:
> don't need this iflc?

Done.

[Jim Stichnoth, 2014-10-27 21:33:09]

still lgtm

https://codereview.chromium.org/659513005/diff/180001/Makefile
File Makefile (right):

https://codereview.chromium.org/659513005/diff/180001/Makefile#newcode15
Makefile:15: CXX.Flags += -std=c++11 -Wextra -Werror
We shouldn't need -std=c++11 after the LLVM 3.5 merge, so either remove it or
add a TODO to remove it, depending on whether you land this before or after the
merge.

And to be safe, maybe add -Wno-error=unused-parameter ?

[Karl, 2014-10-27 21:42:11]

Committed patchset #11 (id:200001) manually as
b262c5e0b110e63ec706213adf0af202274c442e (presubmit successful).

[Karl, 2014-10-27 21:42:28]

https://codereview.chromium.org/659513005/diff/180001/Makefile
File Makefile (right):

https://codereview.chromium.org/659513005/diff/180001/Makefile#newcode15
Makefile:15: CXX.Flags += -std=c++11 -Wextra -Werror
On 2014/10/27 21:33:09, stichnot wrote:
> We shouldn't need -std=c++11 after the LLVM 3.5 merge, so either remove it or
> add a TODO to remove it, depending on whether you land this before or after
the
> merge.
> 
> And to be safe, maybe add -Wno-error=unused-parameter ?

Done.