Create local config file for subzero reader tests.

Create local config file for subzero reader tests.

Using lit.local.cfg, don't allow reader tests unless dumping
of IR is allowed.

This was suggested by Jan in: https://codereview.chromium.org/686913005

BUG=None
R=jvoung@chromium.org

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

[Karl, 2014-11-17 20:52:09]

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

[jvoung (off chromium), 2014-11-17 22:16:10]

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/i...
File tests_lit/reader_tests/insertextract-err.ll (left):

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/i...
tests_lit/reader_tests/insertextract-err.ll:12: ; CHECK: Error: {{.*}} not
{{.*}} constant
I didn't think about this when making the suggesting, but... we'll probably want
some lightweight error message even in the minimal builds, right?

That might mean moving the -err tests to a directory that does not require
allow_dump.

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
File tests_lit/reader_tests/lit.local.cfg (right):

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
tests_lit/reader_tests/lit.local.cfg:4: # parsed
could merge line 4 and 5 ?

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/n...
File tests_lit/reader_tests/nacl-fake-intrinsic.ll (left):

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/n...
tests_lit/reader_tests/nacl-fake-intrinsic.ll:6: ; RUN: %if --need=allow_dump
--command \
Similar comment about error messages, except I guess this one did test for
no_dump.

[Jim Stichnoth, 2014-11-18 16:04:59]

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/i...
File tests_lit/reader_tests/insertextract-err.ll (left):

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/i...
tests_lit/reader_tests/insertextract-err.ll:12: ; CHECK: Error: {{.*}} not
{{.*}} constant
On 2014/11/17 22:16:10, jvoung wrote:
> I didn't think about this when making the suggesting, but... we'll probably
want
> some lightweight error message even in the minimal builds, right?
> 
> That might mean moving the -err tests to a directory that does not require
> allow_dump.

Good point.  What about retaining most or all of the error dump output even in
the minimal build, and not requiring allow_dump in the -err.ll tests?  And/or
put those tests in a separate directory for clarity as Jan suggests?

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
File tests_lit/reader_tests/lit.local.cfg (right):

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
tests_lit/reader_tests/lit.local.cfg:3: # This directory contain reader tests
that require the ability to dump
contains

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
tests_lit/reader_tests/lit.local.cfg:4: # parsed
On 2014/11/17 22:16:10, jvoung wrote:
> could merge line 4 and 5 ?

How about 3, 4, and 5? :)

[Karl, 2014-11-19 18:34:10]

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/i...
File tests_lit/reader_tests/insertextract-err.ll (left):

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/i...
tests_lit/reader_tests/insertextract-err.ll:12: ; CHECK: Error: {{.*}} not
{{.*}} constant
On 2014/11/18 16:04:58, stichnot wrote:
> On 2014/11/17 22:16:10, jvoung wrote:
> > I didn't think about this when making the suggesting, but... we'll probably
> want
> > some lightweight error message even in the minimal builds, right?
> > 
> > That might mean moving the -err tests to a directory that does not require
> > allow_dump.
> 
> Good point.  What about retaining most or all of the error dump output even in
> the minimal build, and not requiring allow_dump in the -err.ll tests?  And/or
> put those tests in a separate directory for clarity as Jan suggests?

Built a test directory "parse-errs" for parsing errors. Tests what errors are
generated when "dump" is both allowed and disallowed. Modified errors when
"dump" is not allowed to generate a simplistic  parse error message based on the
bitcode record being parsed, since many of the parse error count on the
availability of dump routines.

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
File tests_lit/reader_tests/lit.local.cfg (right):

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
tests_lit/reader_tests/lit.local.cfg:3: # This directory contain reader tests
that require the ability to dump
On 2014/11/18 16:04:59, stichnot wrote:
> contains

Done.

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
tests_lit/reader_tests/lit.local.cfg:4: # parsed
On 2014/11/17 22:16:10, jvoung wrote:
> could merge line 4 and 5 ?

Done.

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/l...
tests_lit/reader_tests/lit.local.cfg:4: # parsed
On 2014/11/18 16:04:59, stichnot wrote:
> On 2014/11/17 22:16:10, jvoung wrote:
> > could merge line 4 and 5 ?
> 
> How about 3, 4, and 5? :)

Can't merge all three lines, goes over 80 columns (82).

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/n...
File tests_lit/reader_tests/nacl-fake-intrinsic.ll (left):

https://codereview.chromium.org/735513002/diff/20001/tests_lit/reader_tests/n...
tests_lit/reader_tests/nacl-fake-intrinsic.ll:6: ; RUN: %if --need=allow_dump
--command \
On 2014/11/17 22:16:10, jvoung wrote:
> Similar comment about error messages, except I guess this one did test for
> no_dump.

Moved this to parse-errs directory, and added tests for both dump and no-dump
cases.

[Jim Stichnoth, 2014-11-19 19:21:00]

https://codereview.chromium.org/735513002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/735513002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:167: friend class BlockParserBaseClass;
Can we avoid resorting to friend declarations this early in the design?  (Same
argument I made against mutable.)  I'd rather add public getter/setter methods
as needed.

(I realize none of this matters much when none of these classes are declared in
headers, but still.)

(I also see that there are some other friend declarations in the code base that
managed to evade my notice, so they're now on my list...)

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/nac...
File tests_lit/parse_errs/nacl-fake-intrinsic.ll (right):

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/nac...
tests_lit/parse_errs/nacl-fake-intrinsic.ll:33: 
Git is unhappy with extra blank lines at the end of a file.

[jvoung (off chromium), 2014-11-19 19:57:55]

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
File tests_lit/parse_errs/insertextract-err.ll (right):

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
tests_lit/parse_errs/insertextract-err.ll:18: ; MIN: Error: {{.*}} Invalid
function record: <6 4 3>
Hmm it wasn't clear to me what the first {{.*}} was, so I tried the patch =)

I don't think people will be able to do much with:

MIN: Error: (512:0) Invalid function record: <6 4 3>

This is contrast to:

CHECK: Error: (512:0) Vector index not integer constant: extractelement <4 x i1>
%__0, i32 %__1

=============

Can we try to make the two more similar?

CHECK: Error in function foo (512:0) Vector index not integer constant in
extractelement instruction: <4 x i1> %__0, i32 %__1

MIN: Error in function foo (512:0) Vector index not integer constant in
extractelement instruction.

Then the minimal version might omit the Instruction and operands dump, but allow
the dumpVectorIndexCheckValue. Then the context is the same and has the
containing function, the type of the inst record, and the reason, so is pretty
close to the full thing.

Other ideas? Other ways of formatting it? Recommend that people run
pnacl-abicheck (the in-browser error messages are pretty short)?

[Jim Stichnoth, 2014-11-19 23:30:37]

https://codereview.chromium.org/735513002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/735513002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:653: for (const auto Val : Record.GetValues()) {
Probably better not to use auto here, since one has to do some work to track
down the the actual type.

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
File tests_lit/parse_errs/insertextract-err.ll (right):

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
tests_lit/parse_errs/insertextract-err.ll:18: ; MIN: Error: {{.*}} Invalid
function record: <6 4 3>
On 2014/11/19 19:57:55, jvoung wrote:
> Hmm it wasn't clear to me what the first {{.*}} was, so I tried the patch =)
> 
> I don't think people will be able to do much with:
> 
> MIN: Error: (512:0) Invalid function record: <6 4 3>
> 
> This is contrast to:
> 
> CHECK: Error: (512:0) Vector index not integer constant: extractelement <4 x
i1>
> %__0, i32 %__1
> 
> =============
> 
> Can we try to make the two more similar?
> 
> CHECK: Error in function foo (512:0) Vector index not integer constant in
> extractelement instruction: <4 x i1> %__0, i32 %__1
> 
> MIN: Error in function foo (512:0) Vector index not integer constant in
> extractelement instruction.
> 
> Then the minimal version might omit the Instruction and operands dump, but
allow
> the dumpVectorIndexCheckValue. Then the context is the same and has the
> containing function, the type of the inst record, and the reason, so is pretty
> close to the full thing.
> 
> Other ideas? Other ways of formatting it? Recommend that people run
> pnacl-abicheck (the in-browser error messages are pretty short)?

One thing Karl mentioned offline is that if the minimal build makes it hard to
debug, you can switch to a non-minimal build.  Does that address part of your
question?

That would be the cheap solution here.  Otherwise, some effort will be needed to
cleanly generate well-formatted messages without an over-abundance of "if
(ALLOW_DUMP) {...}" tests.

[jvoung (off chromium), 2014-11-19 23:59:51]

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
File tests_lit/parse_errs/insertextract-err.ll (right):

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
tests_lit/parse_errs/insertextract-err.ll:18: ; MIN: Error: {{.*}} Invalid
function record: <6 4 3>
On 2014/11/19 23:30:36, stichnot wrote:
> On 2014/11/19 19:57:55, jvoung wrote:
> > Hmm it wasn't clear to me what the first {{.*}} was, so I tried the patch =)
> > 
> > I don't think people will be able to do much with:
> > 
> > MIN: Error: (512:0) Invalid function record: <6 4 3>
> > 
> > This is contrast to:
> > 
> > CHECK: Error: (512:0) Vector index not integer constant: extractelement <4 x
> i1>
> > %__0, i32 %__1
> > 
> > =============
> > 
> > Can we try to make the two more similar?
> > 
> > CHECK: Error in function foo (512:0) Vector index not integer constant in
> > extractelement instruction: <4 x i1> %__0, i32 %__1
> > 
> > MIN: Error in function foo (512:0) Vector index not integer constant in
> > extractelement instruction.
> > 
> > Then the minimal version might omit the Instruction and operands dump, but
> allow
> > the dumpVectorIndexCheckValue. Then the context is the same and has the
> > containing function, the type of the inst record, and the reason, so is
pretty
> > close to the full thing.
> > 
> > Other ideas? Other ways of formatting it? Recommend that people run
> > pnacl-abicheck (the in-browser error messages are pretty short)?
> 
> One thing Karl mentioned offline is that if the minimal build makes it hard to
> debug, you can switch to a non-minimal build.  Does that address part of your
> question?
> 
> That would be the cheap solution here.  Otherwise, some effort will be needed
to
> cleanly generate well-formatted messages without an over-abundance of "if
> (ALLOW_DUMP) {...}" tests.


The cheap solution would be fine for development.

I'm trying to see what we want in the future in-browser translator, and I
imagined the minimal build is what we would use.

Right now with the in-browser pnacl-llc, we don't have very useful
*translation-time* error messages in the browser console, and an outside
developer would need to run some other tools to get more information.

E.g., see "[native-client-discuss] PNaCl ABI verification failed", or 
([native-client-discuss] (SIMD) Error reading bitcode file: Invalid argument):
https://code.google.com/p/nativeclient/issues/detail?id=3953.

Maybe this isn't a real problem, since the the linker is supposed to run ABI
verification after linking. However, some things seem to slip through sometimes
so maybe there is bug somewhere. Then we should focus on figuring out how things
slip through from build-time error to browser-translation-time error.


Either that or we can change the browser console message to say "please run
pnacl-abicheck for more details", so that it's more self-documenting?

[Karl, 2014-11-20 17:32:09]

https://codereview.chromium.org/735513002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/735513002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:167: friend class BlockParserBaseClass;
On 2014/11/19 19:21:00, stichnot wrote:
> Can we avoid resorting to friend declarations this early in the design?  (Same
> argument I made against mutable.)  I'd rather add public getter/setter methods
> as needed.
> 
> (I realize none of this matters much when none of these classes are declared
in
> headers, but still.)
> 
> (I also see that there are some other friend declarations in the code base
that
> managed to evade my notice, so they're now on my list...)

No longer friendly.

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
File tests_lit/parse_errs/insertextract-err.ll (right):

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
tests_lit/parse_errs/insertextract-err.ll:18: ; MIN: Error: {{.*}} Invalid
function record: <6 4 3>
On 2014/11/19 23:59:51, jvoung wrote:
> On 2014/11/19 23:30:36, stichnot wrote:
> > On 2014/11/19 19:57:55, jvoung wrote:
> > > Hmm it wasn't clear to me what the first {{.*}} was, so I tried the patch
=)
> > > 
> > > I don't think people will be able to do much with:
> > > 
> > > MIN: Error: (512:0) Invalid function record: <6 4 3>
> > > 
> > > This is contrast to:
> > > 
> > > CHECK: Error: (512:0) Vector index not integer constant: extractelement <4
x
> > i1>
> > > %__0, i32 %__1
> > > 
> > > =============
> > > 
> > > Can we try to make the two more similar?
> > > 
> > > CHECK: Error in function foo (512:0) Vector index not integer constant in
> > > extractelement instruction: <4 x i1> %__0, i32 %__1
> > > 
> > > MIN: Error in function foo (512:0) Vector index not integer constant in
> > > extractelement instruction.
> > > 
> > > Then the minimal version might omit the Instruction and operands dump, but
> > allow
> > > the dumpVectorIndexCheckValue. Then the context is the same and has the
> > > containing function, the type of the inst record, and the reason, so is
> pretty
> > > close to the full thing.
> > > 
> > > Other ideas? Other ways of formatting it? Recommend that people run
> > > pnacl-abicheck (the in-browser error messages are pretty short)?
> > 
> > One thing Karl mentioned offline is that if the minimal build makes it hard
to
> > debug, you can switch to a non-minimal build.  Does that address part of
your
> > question?
> > 
> > That would be the cheap solution here.  Otherwise, some effort will be
needed
> to
> > cleanly generate well-formatted messages without an over-abundance of "if
> > (ALLOW_DUMP) {...}" tests.
> 
> 
> The cheap solution would be fine for development.
> 
> I'm trying to see what we want in the future in-browser translator, and I
> imagined the minimal build is what we would use.
> 
> Right now with the in-browser pnacl-llc, we don't have very useful
> *translation-time* error messages in the browser console, and an outside
> developer would need to run some other tools to get more information.
> 
> E.g., see "[native-client-discuss] PNaCl ABI verification failed", or 
> ([native-client-discuss] (SIMD) Error reading bitcode file: Invalid argument):
> https://code.google.com/p/nativeclient/issues/detail?id=3953.
> 
> Maybe this isn't a real problem, since the the linker is supposed to run ABI
> verification after linking. However, some things seem to slip through
sometimes
> so maybe there is bug somewhere. Then we should focus on figuring out how
things
> slip through from build-time error to browser-translation-time error.
> 
> 
> Either that or we can change the browser console message to say "please run
> pnacl-abicheck for more details", so that it's more self-documenting?

I'm not sure that asking to run pnacl-abicheck is (necessarily) the  right
answer. That assumes that there wasn't a bug in the PNaCl bitcode file, so that
LLVM IR can be built.

Remember that (as in GO) there are now bitcode generators that may  have bugs in
them. These bugs may generate parsing error messages.

In any case, If I have read Jan's comments correctly, I assume that the current
solution is acceptable?

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/nac...
File tests_lit/parse_errs/nacl-fake-intrinsic.ll (right):

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/nac...
tests_lit/parse_errs/nacl-fake-intrinsic.ll:33: 
On 2014/11/19 19:21:00, stichnot wrote:
> Git is unhappy with extra blank lines at the end of a file.

Done.

[jvoung (off chromium), 2014-11-20 18:14:43]

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
File tests_lit/parse_errs/insertextract-err.ll (right):

https://codereview.chromium.org/735513002/diff/60001/tests_lit/parse_errs/ins...
tests_lit/parse_errs/insertextract-err.ll:18: ; MIN: Error: {{.*}} Invalid
function record: <6 4 3>
On 2014/11/20 17:32:09, Karl wrote:
> On 2014/11/19 23:59:51, jvoung wrote:
> > On 2014/11/19 23:30:36, stichnot wrote:
> > > On 2014/11/19 19:57:55, jvoung wrote:
> > > > Hmm it wasn't clear to me what the first {{.*}} was, so I tried the
patch
> =)
> > > > 
> > > > I don't think people will be able to do much with:
> > > > 
> > > > MIN: Error: (512:0) Invalid function record: <6 4 3>
> > > > 
> > > > This is contrast to:
> > > > 
> > > > CHECK: Error: (512:0) Vector index not integer constant: extractelement
<4
> x
> > > i1>
> > > > %__0, i32 %__1
> > > > 
> > > > =============
> > > > 
> > > > Can we try to make the two more similar?
> > > > 
> > > > CHECK: Error in function foo (512:0) Vector index not integer constant
in
> > > > extractelement instruction: <4 x i1> %__0, i32 %__1
> > > > 
> > > > MIN: Error in function foo (512:0) Vector index not integer constant in
> > > > extractelement instruction.
> > > > 
> > > > Then the minimal version might omit the Instruction and operands dump,
but
> > > allow
> > > > the dumpVectorIndexCheckValue. Then the context is the same and has the
> > > > containing function, the type of the inst record, and the reason, so is
> > pretty
> > > > close to the full thing.
> > > > 
> > > > Other ideas? Other ways of formatting it? Recommend that people run
> > > > pnacl-abicheck (the in-browser error messages are pretty short)?
> > > 
> > > One thing Karl mentioned offline is that if the minimal build makes it
hard
> to
> > > debug, you can switch to a non-minimal build.  Does that address part of
> your
> > > question?
> > > 
> > > That would be the cheap solution here.  Otherwise, some effort will be
> needed
> > to
> > > cleanly generate well-formatted messages without an over-abundance of "if
> > > (ALLOW_DUMP) {...}" tests.
> > 
> > 
> > The cheap solution would be fine for development.
> > 
> > I'm trying to see what we want in the future in-browser translator, and I
> > imagined the minimal build is what we would use.
> > 
> > Right now with the in-browser pnacl-llc, we don't have very useful
> > *translation-time* error messages in the browser console, and an outside
> > developer would need to run some other tools to get more information.
> > 
> > E.g., see "[native-client-discuss] PNaCl ABI verification failed", or 
> > ([native-client-discuss] (SIMD) Error reading bitcode file: Invalid
argument):
> > https://code.google.com/p/nativeclient/issues/detail?id=3953.
> > 
> > Maybe this isn't a real problem, since the the linker is supposed to run ABI
> > verification after linking. However, some things seem to slip through
> sometimes
> > so maybe there is bug somewhere. Then we should focus on figuring out how
> things
> > slip through from build-time error to browser-translation-time error.
> > 
> > 
> > Either that or we can change the browser console message to say "please run
> > pnacl-abicheck for more details", so that it's more self-documenting?
> 
> I'm not sure that asking to run pnacl-abicheck is (necessarily) the  right
> answer. That assumes that there wasn't a bug in the PNaCl bitcode file, so
that
> LLVM IR can be built.
> 
> Remember that (as in GO) there are now bitcode generators that may  have bugs
in
> them. These bugs may generate parsing error messages.


That's a good point. The errors can be for any number of reasons (ABI issues,
bitcode parsing issues, or maybe hitting something unexpected somewhere in a
later phase)... So is the solution then to ship a non-minimal commandline
version in the SDK, but a minimal browser version, and direct people to the
commandline version in the browser console? That could also be true for
pnacl-llc, so people may need to decide between running llvm2ice or pnacl-llc.
The in-browser-tool would know which it is, and could tell people which
commandline tool to run perhaps. Currently the message that shows up in the
browser is the message sent to report_fatal_error() in pnacl-llc, I believe.


> In any case, If I have read Jan's comments correctly, I assume that the
current
> solution is acceptable?

Sure, this is okay for now. I just wanted to be sure
https://code.google.com/p/nativeclient/issues/detail?id=3953 is on the radar and
we consider that.

[Karl, 2014-11-24 17:31:33]

Ping? (Still looking for LGTM or comments on what needs to change).

[jvoung (off chromium), 2014-11-24 18:45:54]

Otherwise LGTM

https://codereview.chromium.org/735513002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/735513002/diff/80001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2530: if (!isValidRecordSize(1, "constants block set
type"))
Just "set type" now

[Karl, 2014-12-01 21:39:08]

Committed patchset #7 (id:120001) manually as
2f6f8605b944b17a307425a81013b51b5a00a1d3 (presubmit successful).

[Karl, 2014-12-01 21:39:33]

https://codereview.chromium.org/735513002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/735513002/diff/80001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2530: if (!isValidRecordSize(1, "constants block set
type"))
On 2014/11/24 18:45:53, jvoung wrote:
> Just "set type" now

Done.