Fix processing of local variable indices in fuction blocks.

Fix processing of local variable indices in fuction blocks.

The previous code used a vector to hold local values associated with
indices in the bitcode file. The problem was that the vector would be
expanded to match the index of a "variable index forward reference".
If the index was very large, the program would freeze the computer
trying to allocate an array large enough to contain the index.

This patch fixes this by using a local unordered map instead of a
vector.  Hence, forward index references just add a sinle entry into
the map.

Note that this fix doesn't have a corresponding issue. However, the
problem was made apparent from the problems noted in issues 4257 and
4261.

BUG=None
R=stichnot@chromium.org

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

[Karl, 2015-08-07 22:33:14]

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

[Jim Stichnoth, 2015-08-08 16:14:48]

lgtm

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

https://codereview.chromium.org/1282523002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1291: typedef std::unordered_map<NaClBcIndexSize_t,
Ice::Operand *> OperandsMap;
For naming consistency, I would use "OperandMap" or "CfgNodesMap".  (I prefer
OperandMap/CfgNodeMap, fwiw.)

https://codereview.chromium.org/1282523002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1445: // TODO(kschimpf) Remove error recovery once
implementation complete.
Trying to remember the context of these TODOs...  Isn't implementation complete
by now?

https://codereview.chromium.org/1282523002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1957: for (const std::pair<NaClBcIndexSize_t,
Ice::Operand *> Elmt : LocalOperands)
I think this calls for auto:

  for (auto &Elmt : LocalOperands)

Or if not:

  for (const OperandsMap::value_type &Elmt : LocalOperands)

[Karl, 2015-08-10 18:13:01]

Committed patchset #3 (id:40001) manually as
c6acf08fae334eeade1577853a471e33cb05bc25 (presubmit successful).

[Karl, 2015-08-10 18:13:09]

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

https://codereview.chromium.org/1282523002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1291: typedef std::unordered_map<NaClBcIndexSize_t,
Ice::Operand *> OperandsMap;
On 2015/08/08 16:14:48, stichnot wrote:
> For naming consistency, I would use "OperandMap" or "CfgNodesMap".  (I prefer
> OperandMap/CfgNodeMap, fwiw.)

Done.

https://codereview.chromium.org/1282523002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1445: // TODO(kschimpf) Remove error recovery once
implementation complete.
On 2015/08/08 16:14:48, stichnot wrote:
> Trying to remember the context of these TODOs...  Isn't implementation
complete
> by now?

This was back when someone wasn't sure we should keep error recovery, once we
get things working (i.e. consider it only for debugging). I think that keeping
error recovery around is good, and will fix in a separate CL>

https://codereview.chromium.org/1282523002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1957: for (const std::pair<NaClBcIndexSize_t,
Ice::Operand *> Elmt : LocalOperands)
On 2015/08/08 16:14:48, stichnot wrote:
> I think this calls for auto:
> 
>   for (auto &Elmt : LocalOperands)
> 
> Or if not:
> 
>   for (const OperandsMap::value_type &Elmt : LocalOperands)

Good point. I didn't put the auto in because I didn't think the type was clear
enough. However, Using the second form is clear and simpler.