[lldb-dev] [Reproducers] SBReproducer RFC

[Pavel Labath via lldb-dev]

On 07/01/2019 19:26, Jonas Devlieghere wrote:
> 
> 
> On Mon, Jan 7, 2019 at 1:40 AM Pavel Labath <pavel at labath.sk 
> <mailto:pavel at labath.sk>> wrote:
>     I've been thinking about how could this be done better, and the best
>     (though not ideal) way I came up with is using the functions address as
>     the key. That's guaranteed to be unique everywhere. Of course, you
>     cannot serialize that to a file, but since you already have a central
>     place where you list all intercepted functions (to register their
>     replayers), that place can be also used to assign unique integer IDs to
>     these functions. So then the idea would be that the SB_RECORD macro
>     takes the address of the current function, that gets converted to an ID
>     in the lookup table, and the ID gets serialized.
> 
> 
> It sound like you would generate the indices at run-time. How would that 
> work with regards to the the reverse mapping?
In the current implementation, SBReplayer::Init contains a list of all 
intercepted methods, right? Each of the SB_REGISTER calls takes two 
arguments: The method name, and the replay implementation.

I would change that so that this macro takes three arguments:
- the function address (the "runtime" ID)
- an integer (the "serialized" ID)
- the replay implementation

This creates a link between the function address and the serialized ID. 
So when, during capture, a method calls SB_RECORD_ENTRY and passes in 
the function address, that address can be looked up and translated to an 
ID for serialization.

The only thing that would need to be changed is to have SBReplayer::Init 
execute during record too (which probably means it shouldn't be called 
SBReplayer, but whatever..), so that the ID mapping is also available 
when capturing.

Does that make sense?

> 
>     The part that bugs me about this is that taking the address of an
>     overloaded function is extremely tedious (you have to write something
>     like static_cast<function_prototype>(&SBFoo::Bar)). That would mean all
>     of these things would have to be passed to the RECORD macro. OTOH, the
>     upshot of this would be that the macro would now have sufficient
>     information to perform pretty decent error checking on its invocation.
>     Another nice about this could be that once you already have a prototype
>     and an address of the function, it should be possible (with sufficient
>     template-fu) to synthesize replay code for the function automatically,
>     at least in the simple cases, which would avoid the repetitiveness of
>     the current replay code. Together, this might obviate the need for any
>     clang plugins or other funny build steps.
> 
> 
> See my previous question. I see how the signature would help with 
> decoding but still missing how you'd get the mapping.
> 
>     The second thing I noticed is the usage of pointers for identifying
>     object. A pointer is good for that but only while the object it points
>     to is alive. Once the object is gone, the pointer can (and most likely
>     will) be reused. So, it sounds to me like you also need to track the
>     lifetime of these objects. That may be as simple as intercepting
>     constructor/destructor calls, but I haven't seen anything like that yet
>     (though I haven't looked at all details of the patch).
> 
> 
> This shouldn't be a problem. When a new object is created it will be 
> recorded in the table with a new identifier.
Ok, sounds good.

> 
>     Tying into that is the recording of return values. It looks like the
>     current RECORD_RETURN macro will record the address of the temporary
>     object in the frame of the current function. However, that address will
>     become invalid as soon as the function returns as the result object
>     will
>     be copied into a location specified by the caller as a part of the
>     return processing. Are you handling this in any way?
> 
> 
> We capture the temporary and the call to the copy-assignment 
> constructor. This is not super efficient but it's the best we can do.

Ok, cool. I must have missed that part in the code.

> 
>     The final thing, which I noticed is the lack of any sign of threading
>     support. I'm not too worried about that, as that sounds like something
>     that could be fitted into the existing framework incrementally, but it
>     is something worth keeping in mind, as you're going to run into that
>     pretty soon.
> 
> 
> Yup, I've intentially ignored this for now.

Awasome.