Support for return-address rewriting profilers.

Adds a new API where the host can supply a callback function. The callback function can resolve the location of a return address on stack to the location where a return-address rewriting profiler stashed the original return address.

Committed: https://code.google.com/p/v8/source/detail?r=10845

[Roger McFarlane (Chromium), 2012-02-15 21:19:02]

some musings

https://chromiumcodereview.appspot.com/9401019/diff/1004/src/platform-win32.cc
File src/platform-win32.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/1004/src/platform-win32.c...
src/platform-win32.cc:131: // TODO(siggi): Check for the presence of the Syzygy
instrumentation by
Another way to do this would be to introduce and reference a known symbol that
is set to NULL by default in the source code, but rewritten to point to the
imported resolution function during instrumentation.

That way less knowledge of the profiler is required here.

i.e.,
  // Declared non-const, at global scope, and marked extern.
  extern "C" ReturnAddressResolutionFunction address_resolution_func = NULL;

Usage below is then simplified:

  Address* OS::ResolveReturnAddressLocation(Address* pc_address) {
    if (address_resolution_func == NULL)
      return pc_address;
    return address_resolution_func(pc_address);
  }

It's the responsiblity of the instrumenter to recognize the symbol and rewrite
all references/relocs to it.

[Sigurður Ásgeirsson, 2012-02-15 21:45:55]

https://chromiumcodereview.appspot.com/9401019/diff/1004/src/platform-win32.cc
File src/platform-win32.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/1004/src/platform-win32.c...
src/platform-win32.cc:131: // TODO(siggi): Check for the presence of the Syzygy
instrumentation by
On 2012/02/15 21:19:03, Roger McFarlane (Chromium) wrote:
> Another way to do this would be to introduce and reference a known symbol that
> is set to NULL by default in the source code, but rewritten to point to the
> imported resolution function during instrumentation.
> 
> That way less knowledge of the profiler is required here.
> 
> i.e.,
>   // Declared non-const, at global scope, and marked extern.
>   extern "C" ReturnAddressResolutionFunction address_resolution_func = NULL;
> 
> Usage below is then simplified:
> 
>   Address* OS::ResolveReturnAddressLocation(Address* pc_address) {
>     if (address_resolution_func == NULL)
>       return pc_address;
>     return address_resolution_func(pc_address);
>   }
> 
> It's the responsiblity of the instrumenter to recognize the symbol and rewrite
> all references/relocs to it.
> 
To expand on my email reply, the issue is that binary rewriting is so far
outside the ABI that results are totally unpredictable.
With WPO, the linker has sufficient information to know that nothing in the
binary refers to the pointer above, so the whole thing can and will be folded to
a noop.
As V8 is compiled by many users to in many different ways, and both to
executables and DLLs (as in the Chrome case), I hesitate to add mandatory
exports, especially a data export. There are all sorts of considerations that
come into play at that point. Even for a function export, it's not commonplace
to see those on EXEs, and the compiler/linker has freedom to duplicate
functions, so you could e.g. get a trivial inline at usage, in addition to an
export.
I feel binary rewriting by the instrumenter is not the way to go here.

[Sigurður Ásgeirsson, 2012-02-23 19:22:47]

Vyacheslav, this exposes a new API whereby the host can set a callback function
to fix the effects of return-address rewriting profilers. I believe the change
is ready for primetime, though I still have some work to do to test this in
combination with Chrome and Syzygy.

[Vyacheslav Egorov (Chromium), 2012-02-24 10:34:02]

lgtm once you address comments.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/api.cc#newcode4027
src/api.cc:4027: 
add empty line

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/api.cc#newcode4032
src/api.cc:4032: 
add empty line

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc
File src/frames.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcode45
src/frames.cc:45: ReturnAddressLocationResolver return_address_location_resolver
= NULL;
declare it static

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcode49
src/frames.cc:49: Address* ResolveReturnAddressLocation(Address* pc_address) {
declare it static

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcod...
src/frames.cc:431: 
add empty line

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcod...
src/frames.cc:434: return_address_location_resolver = resolver;
assert that we don't have any other resolver installed.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/v8.cc
File src/v8.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/v8.cc#newcode147
src/v8.cc:147: 
+\n

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/v8.cc#newcode152
src/v8.cc:152: 
+\n

[Vyacheslav Egorov (Chromium), 2012-02-24 10:35:59]

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.h
File src/frames.h (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.h#newcode245
src/frames.h:245: // to resolve the location of a return address to the location
of the
trailing white space.

[Sigurður Ásgeirsson, 2012-02-24 14:46:04]

Thanks. How do I go about submitting this CL?

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/api.cc
File src/api.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/api.cc#newcode4027
src/api.cc:4027: 
On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> add empty line

Done.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/api.cc#newcode4032
src/api.cc:4032: 
On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> add empty line

Done.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc
File src/frames.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcode45
src/frames.cc:45: ReturnAddressLocationResolver return_address_location_resolver
= NULL;
On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> declare it static

Done.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcode49
src/frames.cc:49: Address* ResolveReturnAddressLocation(Address* pc_address) {
On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> declare it static

Done.
Does this warrant "static inline" or are the compilers we use generally smart
enough to work that out for themselves?

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcod...
src/frames.cc:431: 
On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> add empty line

Done.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcod...
src/frames.cc:434: return_address_location_resolver = resolver;
On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> assert that we don't have any other resolver installed.

Done.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.h
File src/frames.h (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.h#newcode245
src/frames.h:245: // to resolve the location of a return address to the location
of the
On 2012/02/24 10:35:59, Vyacheslav Egorov wrote:
> trailing white space.

Ooops, done.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/v8.cc
File src/v8.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/v8.cc#newcode147
src/v8.cc:147: 
On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> +\n

Oops, force of habit. Done.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/v8.cc#newcode152
src/v8.cc:152: 
On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> +\n

Done.

[Vyacheslav Egorov (Chromium), 2012-02-24 14:48:21]

I will commit it for you.

CQ does not work for V8.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc
File src/frames.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcode49
src/frames.cc:49: Address* ResolveReturnAddressLocation(Address* pc_address) {
On 2012/02/24 14:46:04, Ruðrugis wrote:
> On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> > declare it static
> 
> Done.
> Does this warrant "static inline" or are the compilers we use generally smart
> enough to work that out for themselves?

Yes, I think there is no harm declaring it inline (though these days it serves
more as a way to communicate our "hope of inlining" to other code readers than
to compiler).

[Sigurður Ásgeirsson, 2012-02-24 14:59:40]

Thanks.

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc
File src/frames.cc (right):

https://chromiumcodereview.appspot.com/9401019/diff/5001/src/frames.cc#newcode49
src/frames.cc:49: Address* ResolveReturnAddressLocation(Address* pc_address) {
On 2012/02/24 14:48:21, Vyacheslav Egorov wrote:
> On 2012/02/24 14:46:04, Ruðrugis wrote:
> > On 2012/02/24 10:34:02, Vyacheslav Egorov wrote:
> > > declare it static
> > 
> > Done.
> > Does this warrant "static inline" or are the compilers we use generally
smart
> > enough to work that out for themselves?
> 
> Yes, I think there is no harm declaring it inline (though these days it serves
> more as a way to communicate our "hope of inlining" to other code readers than
> to compiler).

Done.