1491067 - Templatize and common up redirections code

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently, we require a separate definition for each function that is being redirected.  There are a ton of macros to reduce many of these to one line, but even slightly non-trivial cases need a hand written definition with a decent amount of boilerplate involved.

The attached patch refactors the implementation for redirections to eliminate these macros and remove most of the boilerplate.  The basic idea is to take the assembly routines used for the objc_msgSend redirection and adapt them for use by all redirections.  These routines capture all the call arguments into a structure, which also stores any return value the call should produce.  Individual redirections then operate on this structure to save any outputs the call produces or to modify its behavior.  Using the call arguments structure makes this process friendly for use with templated common helper functions.  A single line can be used to specify how to redirect functions with complex behaviors, such as ones that either save a value to errno or write to an output buffer, depending on their return value.

While this is a great improvement for conciseness and readability of the redirections code, it is especially important for simplifying the new redirection logic which bug 1488808 will need.

3 files changed, 1402 insertions(+), 2630 deletions(-)

Comment 1

[Nathan Froyd [:froydnj]]

Comment on attachment 9008804 [details] [diff] [review]
patch

Review of attachment 9008804 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for taking so long to review this.  The basic idea is really simple, I just got caught up in thinking there was something more complex lurking.

::: toolkit/recordreplay/ProcessRedirect.h
@@ +178,5 @@
>    // mBaseFunction.
>    uint8_t* mOriginalFunction;
> +
> +  // Redirection hooks are used to control the behavior of the redirected
> +  // function. 

Nit: trailing space.

::: toolkit/recordreplay/ProcessRedirectDarwin.cpp
@@ +45,5 @@
> +// function's name, followed by any hooks associated with the redirection for
> +// saving its output or adding a preamble.
> +#define FOR_EACH_REDIRECTION(MACRO)                              \
> +  /* System call wrappers */                                     \
> +  MACRO(kevent, RR_SaveRvalHadErrorNegative<RR_WriteBuffer<3, 4, struct kevent>>) \

All of these are in the mozilla::recordreplay namespace, correct?  Perhaps we could lose the RR_ prefixes on everything, either here or in a followup patch.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30cc41a0c65b
Templatize and common up redirections code, r=froydnj.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Nathan Froyd [:froydnj] from comment #1)
> ::: toolkit/recordreplay/ProcessRedirectDarwin.cpp
> @@ +45,5 @@
> > +// function's name, followed by any hooks associated with the redirection for
> > +// saving its output or adding a preamble.
> > +#define FOR_EACH_REDIRECTION(MACRO)                              \
> > +  /* System call wrappers */                                     \
> > +  MACRO(kevent, RR_SaveRvalHadErrorNegative<RR_WriteBuffer<3, 4, struct kevent>>) \
> 
> All of these are in the mozilla::recordreplay namespace, correct?  Perhaps
> we could lose the RR_ prefixes on everything, either here or in a followup
> patch.

For now I've left the RR_ prefixes in place.  Bug 1488808 adds another hook in the redirections and I find the prefixes helpful for distinguishing the different kinds of hooks when specifying the redirections, but after bug 1488808 I can try removing the RR_ prefixes and seeing how that affects things.

Comment 4

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/30cc41a0c65b