Check that address is i32 for indirect calls.

Check that address is i32 for indirect calls.

Fixes bug where code did not check that the address of an indirect call must be i32.

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

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

[Karl, 2015-09-23 15:53:11]

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

[Jim Stichnoth, 2015-09-23 20:40:07]

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

https://codereview.chromium.org/1363983002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2665: Ice::Operand *Callee = getOperand(CalleeIndex);
Would it make sense to check Callee for valid type here, i.e. do the check for
both direct and indirect calls?

https://codereview.chromium.org/1363983002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2694: appendErrorInstruction(ReturnType);
Should you return after this error?  Or fall through and possibly add another
error instruction?

[Karl, 2015-09-24 21:28:22]

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

https://codereview.chromium.org/1363983002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2665: Ice::Operand *Callee = getOperand(CalleeIndex);
On 2015/09/23 20:40:06, stichnot wrote:
> Would it make sense to check Callee for valid type here, i.e. do the check for
> both direct and indirect calls?

Type signatures are explicitly defined for direct calls. They aren't defined for
indirect call. Rather, the parts are implied by the types of the parameters, and
a record field defining the return type. The question is whether we should
consume cycles (and memory) generating the type signature for the indirect call.

The code is also complex due to having to handle no IR generation, as well.

However, I agree this code is complex. So I've tried to simplify it down into
more logical steps.

https://codereview.chromium.org/1363983002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2694: appendErrorInstruction(ReturnType);
On 2015/09/23 20:40:06, stichnot wrote:
> Should you return after this error?  Or fall through and possibly add another
> error instruction?

The code in general, should error recover if possible. It should only return if
there is no recovery, or it is too difficult to do.

[Jim Stichnoth, 2015-09-25 22:13:20]

otherwise lgtm

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

https://codereview.chromium.org/1363983002/diff/60001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2067: const Ice::IceString UnknownName("function");
Can you just do:
  return "function";
?

https://codereview.chromium.org/1363983002/diff/60001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2818: for (auto Param : Params)
auto *Param

But actually, I'd prefer "Ice::Operand *Param : Params" because it's still not
that verbose and the definition of Params is non-obvious and relatively far
away.

[Karl, 2015-09-28 18:23:01]

Committed patchset #5 (id:80001) manually as
fc0a52df6a03ee82208c65ae5d33745baf7caa64 (presubmit successful).

[Karl, 2015-09-28 18:23:12]

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

https://codereview.chromium.org/1363983002/diff/60001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2067: const Ice::IceString UnknownName("function");
On 2015/09/25 22:13:20, stichnot wrote:
> Can you just do:
>   return "function";
> ?

Done.

https://codereview.chromium.org/1363983002/diff/60001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2818: for (auto Param : Params)
On 2015/09/25 22:13:20, stichnot wrote:
> auto *Param
> 
> But actually, I'd prefer "Ice::Operand *Param : Params" because it's still not
> that verbose and the definition of Params is non-obvious and relatively far
> away.

Done.