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 <[email protected]> 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits Thanks, - Enrico 📩 egranata@.com ☎️ 27683
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
