Allow stubbing of called constant addresses using command line argument.

Allow stubbing of called constant addresses using command line argument.

When specified (via command line) replaces all called constant addresses
with a stubbed call to the first defined function in the bitcode file.
This allows testing of subzero without having to fix that downstream
code (after parsing) may not handle such addresses.

BUG=None
R=jvoung@chromium.org

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

[Karl, 2015-02-04 21:24:57]

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

[Jim Stichnoth, 2015-02-04 22:24:13]

This is good for timings, but I wonder if there's anything we can do to make
these weird pexes actually run correctly out of the box?

Can the target lowering recognize these bogus calls and replace them with
something valid?

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

https://codereview.chromium.org/902713002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2464: Callee->getKind() ==
Ice::Operand::kConstInteger32) {
Can you use
  llvm::isa<Ice::ConstantInteger32>(Callee)
instead?

[Mark Seaborn, 2015-02-04 22:28:00]

On 4 February 2015 at 14:24, <stichnot@chromium.org> wrote:

> This is good for timings, but I wonder if there's anything we can do to
> make
> these weird pexes actually run correctly out of the box?


I'm curious, what's the context of this?  What are these weird pexes you're
referring to?

Are they using numeric-constant function addresses like this? --
  void (*func)(void) = ((*)(void)) 0x1234;
  func();

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Karl, 2015-02-04 22:46:54]

I do believe that we can add a "translation" table from addresses to
corresponding stubbed names (which could then be added during the link for
testing). However, I leave that to a separate CL.

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

https://codereview.chromium.org/902713002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2464: Callee->getKind() ==
Ice::Operand::kConstInteger32) {
On 2015/02/04 22:24:13, stichnot wrote:
> Can you use
>   llvm::isa<Ice::ConstantInteger32>(Callee)
> instead?

Done.

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

https://codereview.chromium.org/902713002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:326: Ice::Operand *getStubbedConstCallValue(Ice::Operand
*CallValue) {
Added parameter so that we can default if no stubbed function is found. This
allows error recovery to find other issues (while turning off translation).

https://codereview.chromium.org/902713002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2466: Callee =
Context->getStubbedConstCallValue(Callee);
Added parameter to call.

[native-client-reviews_googlegroups.com, 2015-02-04 22:50:52]

It turns out that some builds translate things like NACL_sys_pid to a
(thunk) address. Some emitterIs do not know how to handle this yet. Hence,
we are looking for a (at least temporary) workaround that allows us to
translate pexes with these values in.

An example of a pexe that has such thunk addresses is pnacl-llc.pexe.


Karl Schimpf

On Wed, Feb 4, 2015 at 2:27 PM, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 4 February 2015 at 14:24, <stichnot@chromium.org> wrote:
>
>> This is good for timings, but I wonder if there's anything we can do to
>> make
>> these weird pexes actually run correctly out of the box?
>
>
> I'm curious, what's the context of this?  What are these weird pexes
> you're referring to?
>
> Are they using numeric-constant function addresses like this? --
>   void (*func)(void) = ((*)(void)) 0x1234;
>   func();
>
> Cheers,
> Mark
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2015-02-04 22:59:54]

Do you mean that those pexes fail to translate (i.e. Subzero fails during
translation), or that those pexes don't run successfully after being
translated, when run outside the NaCl sandbox?

Cheers,
Mark

On 4 February 2015 at 14:50, Karl Schimpf <kschimpf@google.com> wrote:

> It turns out that some builds translate things like NACL_sys_pid to a
> (thunk) address. Some emitterIs do not know how to handle this yet. Hence,
> we are looking for a (at least temporary) workaround that allows us to
> translate pexes with these values in.
>
> An example of a pexe that has such thunk addresses is pnacl-llc.pexe.
>
>
> Karl Schimpf
>
> On Wed, Feb 4, 2015 at 2:27 PM, Mark Seaborn <mseaborn@chromium.org>
> wrote:
>
>> On 4 February 2015 at 14:24, <stichnot@chromium.org> wrote:
>>
>>> This is good for timings, but I wonder if there's anything we can do to
>>> make
>>> these weird pexes actually run correctly out of the box?
>>
>>
>> I'm curious, what's the context of this?  What are these weird pexes
>> you're referring to?
>>
>> Are they using numeric-constant function addresses like this? --
>>   void (*func)(void) = ((*)(void)) 0x1234;
>>   func();
>>
>> Cheers,
>> Mark
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[jvoung (off chromium), 2015-02-04 23:11:26]

They fail to translate: "Some emitterIs do not know how to handle this yet."

Though this example will also require sandboxing + sel_ldr + sel_universal to
actually run the result.

More specifically, Karl is trying to build pnacl-llc.pexe for benchmarking
translation speed. Even removing the "--noirt" flag from pnacl-llc Makefile to
avoid libnacl_sys_private doesn't help. pnacl-llc also has nacl syscalls from
imc_syscalls (for srpc).





On 2015/02/04 22:59:54, Mark Seaborn wrote:
> Do you mean that those pexes fail to translate (i.e. Subzero fails during
> translation), or that those pexes don't run successfully after being
> translated, when run outside the NaCl sandbox?
> 
> Cheers,
> Mark
> 
> On 4 February 2015 at 14:50, Karl Schimpf <mailto:kschimpf@google.com> wrote:
> 
> > It turns out that some builds translate things like NACL_sys_pid to a
> > (thunk) address. Some emitterIs do not know how to handle this yet. Hence,
> > we are looking for a (at least temporary) workaround that allows us to
> > translate pexes with these values in.
> >
> > An example of a pexe that has such thunk addresses is pnacl-llc.pexe.
> >
> >
> > Karl Schimpf
> >
> > On Wed, Feb 4, 2015 at 2:27 PM, Mark Seaborn <mailto:mseaborn@chromium.org>
> > wrote:
> >
> >> On 4 February 2015 at 14:24, <mailto:stichnot@chromium.org> wrote:
> >>
> >>> This is good for timings, but I wonder if there's anything we can do to
> >>> make
> >>> these weird pexes actually run correctly out of the box?
> >>
> >>
> >> I'm curious, what's the context of this?  What are these weird pexes
> >> you're referring to?
> >>
> >> Are they using numeric-constant function addresses like this? --
> >>   void (*func)(void) = ((*)(void)) 0x1234;
> >>   func();
> >>
> >> Cheers,
> >> Mark
> >>
> >>
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.

[native-client-reviews_googlegroups.com, 2015-02-04 23:11:34]

Both, sort of.  Subzero fails to translate them because we didn't bother
with the encoding of that kind of instruction (though I assume that should
be fixed if we are going to legitimately see that pattern on real pexes in
sandboxed mode).  And even if Subzero translated them correctly, they
surely wouldn't run correctly in native mode.

I'd like to be able to verify Subzero's correctness and test its
performance on pnacl-llc.pexe and other pexes, as well as time the
translation of such pexes.

On Wed, Feb 4, 2015 at 2:59 PM, Mark Seaborn <mseaborn@chromium.org> wrote:

> Do you mean that those pexes fail to translate (i.e. Subzero fails during
> translation), or that those pexes don't run successfully after being
> translated, when run outside the NaCl sandbox?
>
> Cheers,
> Mark
>
>
> On 4 February 2015 at 14:50, Karl Schimpf <kschimpf@google.com> wrote:
>
>> It turns out that some builds translate things like NACL_sys_pid to a
>> (thunk) address. Some emitterIs do not know how to handle this yet. Hence,
>> we are looking for a (at least temporary) workaround that allows us to
>> translate pexes with these values in.
>>
>> An example of a pexe that has such thunk addresses is pnacl-llc.pexe.
>>
>>
>> Karl Schimpf
>>
>> On Wed, Feb 4, 2015 at 2:27 PM, Mark Seaborn <mseaborn@chromium.org>
>> wrote:
>>
>>> On 4 February 2015 at 14:24, <stichnot@chromium.org> wrote:
>>>
>>>> This is good for timings, but I wonder if there's anything we can do to
>>>> make
>>>> these weird pexes actually run correctly out of the box?
>>>
>>>
>>> I'm curious, what's the context of this?  What are these weird pexes
>>> you're referring to?
>>>
>>> Are they using numeric-constant function addresses like this? --
>>>   void (*func)(void) = ((*)(void)) 0x1234;
>>>   func();
>>>
>>> Cheers,
>>> Mark
>>>
>>>
>>
>  --
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
> native-client-reviews@googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[jvoung (off chromium), 2015-02-05 17:43:36]

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

https://codereview.chromium.org/902713002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2465: Callee->getKind() ==
llvm::isa<Ice::ConstantInteger32>(Callee)) {
You mean just "llvm::isa..." not "getKind() == llvm::isa..." ?

[Karl, 2015-02-05 18:26:51]

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

https://codereview.chromium.org/902713002/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2465: Callee->getKind() ==
llvm::isa<Ice::ConstantInteger32>(Callee)) {
On 2015/02/05 17:43:36, jvoung wrote:
> You mean just "llvm::isa..." not "getKind() == llvm::isa..." ?

I can't believe I did that. Fixing...

[jvoung (off chromium), 2015-02-05 18:50:06]

lgtm

https://codereview.chromium.org/902713002/diff/80001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/902713002/diff/80001/src/llvm2ice.cpp#newcode175
src/llvm2ice.cpp:175: static cl::opt<bool>
Could add a TODO to remove once the emitter handles these cases.

[Karl, 2015-02-05 18:55:43]

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

[Karl, 2015-02-05 18:56:58]

New patchsets have been uploaded after l-g-t-m from jvoung@chromium.org

[Karl, 2015-02-05 18:58:12]

https://codereview.chromium.org/902713002/diff/80001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/902713002/diff/80001/src/llvm2ice.cpp#newcode175
src/llvm2ice.cpp:175: static cl::opt<bool>
On 2015/02/05 18:50:06, jvoung wrote:
> Could add a TODO to remove once the emitter handles these cases.

Done.