Change tracking of basic blocks (within function) to use a vector.

Change tracking of basic blocks (within function) to use a vector.

Changing the code to "preallocate" basic blocks in a vector, rather
than dynamically creating on demand. This has the advantage of not
requiring basic blocks to be sorted after the bitcode is parsed.

This also means that the name of the basic blocks remain constant,
even during parsing, making debugging easier.

The drawback is that the DECLAREBLOCKS bitcode record of a function
block can define a very large number of basic blocks. To control this,
we look at the function block size (within the bitstream) to determine
the maximal number of basic blocks that could be defined. If the
DECLAREBLOCKS record specifies a number larger than this, we generate
an error and recover (if applicable).

We also add an cleanup test that confirms the number of declared basic
blocks correspond to the number of basic blocks defined in the
function.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4261
R=stichnot@chromium.org

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

[Karl, 2015-08-13 21:28:42]

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

[Jim Stichnoth, 2015-08-13 23:16:25]

The assumption that Cfg::getNodes returns list<> rather than a vector<> is
incorrect, and so maybe you want to revise this CL accordingly?

Apart from "list" in the name NodeList, you might have been thinking of a
previous CL where Cfg::reorderNodes() works out of a list<> before finalizing
into a NodeList.

https://codereview.chromium.org/1297433002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1297433002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1332: // faster access (constant vs linear).
This comment is wrong -- Func->getNodes() returns a NodeList which is actually a
std::vector.

https://codereview.chromium.org/1297433002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1400: // TODO(kschimpf) Remove error recovery once
implementation complete.
Remove this TODO like in the previous CL?

[Karl, 2015-08-14 19:18:21]

Sorry about adding the vector BlockNodes. This was confusion on my part between
the LLVM bitcode parsers, and the ICE implementation. Removed BlockNodes and
referred to Cfg.getNodes() instead.

https://codereview.chromium.org/1297433002/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1297433002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1332: // faster access (constant vs linear).
On 2015/08/13 23:16:25, stichnot wrote:
> This comment is wrong -- Func->getNodes() returns a NodeList which is actually
a
> std::vector.

Your right. I was confusing this with LLVM code. Removing BlockNodes.

https://codereview.chromium.org/1297433002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1400: // TODO(kschimpf) Remove error recovery once
implementation complete.
On 2015/08/13 23:16:25, stichnot wrote:
> Remove this TODO like in the previous CL?

Done.

https://codereview.chromium.org/1297433002/diff/20001/tests_lit/parse_errs/ba...
File tests_lit/parse_errs/bad-bb-size.test (right):

https://codereview.chromium.org/1297433002/diff/20001/tests_lit/parse_errs/ba...
tests_lit/parse_errs/bad-bb-size.test:5: ; RUN: pnacl-bcfuzz -bitcode-as-text
%p/Inputs/bad-bb-size.tbc -output - \
Fixing test to not use pnacl-bcfuzz, since the results are not used by the call
to pnacl-sz.

[Jim Stichnoth, 2015-08-14 19:48:43]

lgtm

https://codereview.chromium.org/1297433002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1297433002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1372: NumBytesDefiningFunction = NumWords * 4;
Maybe use some sizeof() instead of 4?

https://codereview.chromium.org/1297433002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1390: if (Index >= Nodes.size()) {
You could use Func->getNumNodes() instead of Nodes.size(), then maybe "return
Func->getNodes()[Index];" below, might be slightly easier to read (and it's the
pattern used in setBbName() below).

[Karl, 2015-08-14 20:08:47]

Committed patchset #4 (id:60001) manually as
98ed44642caf4ce694fbc3401c0ec958137b6f31 (presubmit successful).

[Karl, 2015-08-14 20:09:15]

Also removed last paragraph of description, since it no longer applies.

https://codereview.chromium.org/1297433002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1297433002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1372: NumBytesDefiningFunction = NumWords * 4;
On 2015/08/14 19:48:43, stichnot wrote:
> Maybe use some sizeof() instead of 4?

Done.

https://codereview.chromium.org/1297433002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1390: if (Index >= Nodes.size()) {
On 2015/08/14 19:48:43, stichnot wrote:
> You could use Func->getNumNodes() instead of Nodes.size(), then maybe "return
> Func->getNodes()[Index];" below, might be slightly easier to read (and it's
the
> pattern used in setBbName() below).

Done.