[Lldb-commits] [PATCH] [lldb] ASan history threads SB API

[Enrico Granata]

Inlined!

Sent from my iPhone

> On Sep 3, 2014, at 6:48 PM, Kuba Brecka <kuba.brecka at gmail.com> wrote:
> 
> Hi Enrico,
> 
> > However, a HistoryThreads hardly seems to fit in [ValueImpl]
> > I am assuming this is mostly because:
> > - this is a pointer-based lookup, so it makes little sense on the ValueObject proper
> > - but you still want to store it away somewhere once you computed it
> 
> Exactly.
> 
> > I admit not having followed through your previous patch in depth, so take my
> > suggestion with a grain of salt.. couldn’t your MemoryHistory plugin handle
> > the caching? i.e. all I would ever do when asked for a HistoryThreadsSP is
> > plugin->GetHistoryThreads(pointer_value); and the plugin would internally
> > handle a per-process cache of pointer_value —> thread list
> 
> I would also like to be able to "renew" that thread list, so you don't get stuck with one list that is no longer correct. The SBValue API did the caching and you could always create a new SBValue to "renew".
> 

When would the list be invalidated?
If the list can turn wrong without anyone being made aware of it, and your only hope is to fetch the data again, or risk failure, then no level of caching makes sense.
If I have a way to detect that the list is now wrong, and I am supposed to make a new SBValue, why not just detect it automatically and fetch valid data again inside the plugin?
If detection of changes is entirely impossible, I would argue against caching entirely - slow but correct wins over fast but potentially (and undetectably so) wrong IMO.

> Although I think you're right, a generic "here's a pointer, give me a thread list" sounds reasonable.

I would argue in favor of that, yes.
You can stick this API on ValueObject and hence SBValue as an helper, I ended up seeing the value in that, so no objection there. But I do believe that at the end of the day, your API is really pointer --> threads, anything else being a convenience for users.

> The issue with that is that SB doesn't know about a thread list. Mostly because even internally, ThreadList is not really just a container for Thread objects, but has a lot of process-related logic.
> 
> What would you suggest? Having a SBThreadList that would be just a container (and thus would differ from ThreadList)?
> 

That's not a bad idea. Just like SBValue has a ValueImpl, SBThreadList would have an Impl object. The Impl would contain a set of ThreadSP/SBThread objects and return those when asked.
Just don't put a vector<> or anything like that directly in the SB object if you make one, that causes problems!

> Kuba
> 
> 
> 
>> On Wed, Sep 3, 2014 at 2:54 PM, Enrico Granata <egranata at apple.com> wrote:
>> I think I mentioned that I was not sure SBValue was the best place to place this functionality (it’s address based, not ValueObject based), but it looked like others on the team felt differently, so no big deal there
>> 
>> One thing that worries me a little bit more is how the history threads are being stored on the ValueImpl - I am not sure I agree with that
>> 
>> The ValueImpl is essentially a way to reconstruct the underlying ValueObject for an SBValue given choices on dynamic/synthetic values; I could see us adding more axes to that class if we ended up with more features that could potentially generate different “views” of the same underlying root ValueObject
>> 
>> However, a HistoryThreads hardly seems to fit in there - I am assuming this is mostly because:
>> - this is a pointer-based lookup, so it makes little sense on the ValueObject proper
>> - but you still want to store it away somewhere once you computed it
>> 
>> I admit not having followed through your previous patch in depth, so take my suggestion with a grain of salt.. couldn’t your MemoryHistory plugin handle the caching? i.e. all I would ever do when asked for a HistoryThreadsSP is plugin->GetHistoryThreads(pointer_value); and the plugin would internally handle a per-process cache of pointer_value —> thread list
>> 
>> The advantage is that now the question could be asked from anywhere; I could ask it of an internal ValueObject, or of an SBValue, and the right thing would happen consistently (w.r.t. caching and whatnot) because now the plugin is handling this in a sane manner
>> 
>> This last one is minor but,
>> +typedef std::shared_ptr<HistoryThreads> HistoryThreadsSP;
>> +
>> 
>> why do you need to define this in SBValue.cpp?
>> Can’t HistoryThreadsSP be defined with all the other FooSP typedefs, so it’s visible everywhere?
>> 
>>> On Sep 3, 2014, at 11:41 AM, Kuba Brecka <kuba.brecka at gmail.com> wrote:
>>> 
>>> This patch depends on http://reviews.llvm.org/D4596.
>>> 
>>> As a continuation of the previous patch that adds a MemoryHistory plugin and implementation for ASan-provided malloc/free stack traces, this patch exposes this into the SB API. In short, these two new methods are added into SBValue:
>>> 
>>> * uint32_t SBValue::GetNumMemoryHistoryThreads ();
>>> * SBThread SBValue::GetMemoryHistoryThreadAtIndex (uint32_t idx);
>>> 
>>> This corresponds to how we provide objects for which we don't have containers (SBFrame and GetNumFrames + GetFrameAtIndex). Note that exposing ThreadList into a generic SBThreadList container would not be straightforward, because currently ThreadList is not a generic container of threads, but instead holds functionality tied to a process and can currently only be used to hold all threads in a process.
>>> 
>>> http://reviews.llvm.org/D5175
>>> 
>>> Files:
>>>  include/lldb/API/SBValue.h
>>>  scripts/Python/interface/SBValue.i
>>>  source/API/SBValue.cpp
>>>  test/functionalities/asan/TestAsan.py
>>> <D5175.13219.patch>_______________________________________________
>>> lldb-commits mailing list
>>> lldb-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>> 
>> Thanks,
>> - Enrico
>> 📩 egranata@.com ☎️ 27683
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140903/070b55d0/attachment.html>