[lldb-dev] [Reproducers] SBReproducer RFC

[Pavel Labath via lldb-dev]

On 07/01/2019 22:13, Jonas Devlieghere wrote:
> 
> 
> On Mon, Jan 7, 2019 at 3:52 AM Tamas Berghammer <tberghammer at google.com 
> <mailto:tberghammer at google.com>> wrote:
> 
>     One problem is when the behavior of LLDB is not deterministic for
>     whatever reason (e.g. multi threading, unordered maps, etc...). Lets
>     take SBModule::FindSymbols() what returns an SBSymbolContextList
>     without any specific order (haven't checked the implementation but I
>     would consider a random order to be valid). If a user calls this
>     function, then iterates through the elements to find an index `I`,
>     calls `GetContextAtIndex(I)` and pass the result into a subsequent
>     function then what will we do. Will we capture what did
>     `GetContextAtIndex(I)` returned in the trace and use that value or
>     will we capture the value of `I`, call `GetContextAtIndex(I)` during
>     reproduction and use that value. Doing the first would be correct in
>     this case but would mean we don't call `GetContextAtIndex(I)` while
>     doing the second case would mean we call `GetContextAtIndex(I)` with
>     a wrong index if the order in SBSymbolContextList is non
>     deterministic. In this case as we know that GetContextAtIndex is
>     just an accessor into a vector the first option is the correct one
>     but I can imagine cases where this is not the case (e.g. if
>     GetContextAtIndex would have some useful side effect).
> 
> 
> Indeed, in this scenario we would replay the call with the same `I` 
> resulting in an incorrect value. I think the only solution is fixing the 
> non-derterminism. This should be straightforward for lists (some kind of 
> sensible ordering), but maybe there are other issues I'm not aware of.

For this, I think we should adopt the same rules that llvm has for 
nondeterminism: returning entries in an unspecified order is fine as 
long as that order is always the same for the given set of inputs. So, 
using unordered maps is fine as long as it doesn't use any 
runtime-dependent values (pointers), or this nondeterminism is removed 
(explicit sort) before letting the values out.

This way, when you're debugging something, and your replay doesn't work 
because of nondeterminism, you fix the nodeterministic bug. It may not 
have been the bug you set out to fix, but you still reduce the overall 
number of bugs.

> 
>     Other interesting question is what to do with functions taking raw
>     binary data in the form of a pointer + size (e.g. SBData::SetData).
>     I think we will have to annotate these APIs to make the reproducer
>     system aware of the amount of data they have to capture and then
>     allocate these buffers with the correct lifetime during replay. I am
>     not sure what would be the best way to attach these annotations but
>     I think we might need a fairly generic framework because I won't be
>     surprised if there are more situation when we have to add
>     annotations to the API. I slightly related question is if a function
>     returns a pointer to a raw buffer (e.g. const char* or void*) then
>     do we have to capture the content of it or the pointer for it and in
>     either case what is the lifetime of the buffer returned (e.g.
>     SBError::GetCString() returns a buffer what goes out of scope when
>     the SBError goes out of scope).
> 
> 
> This a good concern and not something I had a good solution for at this 
> point. For const char* string we work around this by serializing the 
> actual string. Obviously that won't always work. Also we have the void* 
> batons for callsback, which is another tricky thing that wouldn't be 
> supported. I'm wondering if we can get away with ignoring these at first 
> (maybe printing something in the replay logic that warns the user that 
> the reproducer contains an unsupported function?).

Overall, I think we should leave some space to enable hand-written 
record/replay logic for the tricky cases. Batons will be one of those 
cases, but I don't think they're unsolvable. The only thing we can do 
with a user-specified void* baton is pass it back to the user callback. 
But we're not going to that since we don't have the code for the 
callback anyway. Nor do we need to do that since we're not interested in 
what happens outside of SB boundary.

For this, I think the right solution is to replay the *effects* of the 
call to the user callback by re-executing the SB calls it made, which 
can be recorded as usual, when they cross the SB boundary.

> 
>     Additionally I am pretty sure we have at least some functions
>     returning various indices what require remapping other then the
>     pointers either because they are just indexing into a data structure
>     with undefined internal order or they referencing some other
>     resource. Just by randomly browsing some of the SB APIs I found for
>     example SBHostOS::ThreadCreate what returns the pid/tid for the
>     newly created thread what will have to be remapped (it also takes a
>     function as an argument what is a problem as well). Because of this
>     I am not sure if we can get away with an automatically generated set
>     of API descriptions instead of wring one with explicit annotations
>     for the various remapping rules.
> 
> 
> Fixing the non-determinism should also address this, right?

I think threads should be handled the same way as callbacks, by 
replaying the contained SB calls. Since this will certainly require some 
custom replay logic, as a part of that logic we can remap the thread IDs.