On 07/01/2019 19:26, Jonas Devlieghere wrote:

On Mon, Jan 7, 2019 at 1:40 AM Pavel Labath <pa...@labath.sk <mailto:pa...@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
    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.

lldb-dev mailing list

Reply via email to