[lldb-dev] [Reproducers] SBReproducer RFC

[Pavel Labath via lldb-dev]

On 07/01/2019 22:45, Frédéric Riss wrote:
> 
> 
>> On Jan 7, 2019, at 11:31 AM, Pavel Labath via lldb-dev 
>> <lldb-dev at lists.llvm.org <mailto:lldb-dev at lists.llvm.org>> wrote:
>>
>> 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><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?
> 
> I think I understand what you’re explaining, and the mapping side of 
> things makes sense. But I’m concerned about the size and complexity of 
> the SB_RECORD macro that will need to be written. IIUC, those would need 
> to take the address of the current function and the prototype, which is 
> a lot of cumbersome text to type. It seems like having a specialized 
> tool to generate those would be nice, but once you have a tool you also 
> don’t need all this complexity, do you?
> 
> Fred
> 

Yes, if the tool generates the IDs for you and checks that the macro 
invocations are correct, then you don't need the function prototype. 
However, that tool also doesn't come for free: Somebody has to write it, 
and it adds complexity in the form of an extra step in the build process.

My point is that this extended macro could provide all the 
error-checking benefits of this tool. It's a tradeoff, of course, and 
the cost here is a more complex macro invocation. I think the choice 
here is mostly down to personal preference of whoever implements this. 
However, if I was implementing this, I'd go for an extended macro, 
because I don't find the extra macro complexity to be too much. For 
example, this should be the macro invocation for 
SBData::SetDataFromDoubleArray:

SB_RECORD(bool, SBData, SetDataFromDoubleArray, (double *, size_t),
     array, array_len);

It's a bit long, but it's not that hard to type, and all of this 
information should be present on the previous line, where 
SBData::SetDataFromDoubleArray is defined (I deliberately made the macro 
argument order match the function definition syntax).

And this approach can be further tweaked. For instance, if we're willing 
to take the hit of having "weird" function definitions, then we can 
avoid the repetition altogether, and make the macro define the function too:

SB_METHOD2(bool, SBData, SetDataFromDoubleArray, double *, array,
     size_t, array_len, {
   // Method body
})

This would also enable you to automatically capture method return value 
for the "object" results.

pl