> On Sep 27, 2016, at 2:55 PM, Daniel Austin Noland <[email protected]>
> wrote:
>
>> Folks on the outside of the SB API’s need to be able to pass callbacks in.
>> We don’t currently have any templates you need to instantiate or classes you
>> need to override to go from outside the SB API to inside it. So whatever
>> happens on the lldb_private side, for the SB API’s we’ll probably have to
>> stick with void * & function pointers.
> That is fair. I may be able to avoid the entire problem with
> llvm::function_ref<>. Will look into it.
I do not think we want the SB API’s to require llvm header files.
Jim
>>> 5. Confusing or simply wrong documentation (e.g.
>>> Breakpoint::ResolveBreakpointInModules has arguments marked as @param[in]
>>> when the description of those arguments clearly indicates that they will be
>>> the output of the function).
>>> 6. We simply don't have a Finishpoint class (triggers callback at the
>>> conclusion of the function or scope)
>>>
>>> My plan:
>>>
>>> I would like to refactor this in a few phases.
>>>
>>> Phase 1: Low hanging fruit
>>> --------------------------
>>>
>>> * Kick as much functionality as is reasonable from Breakpoint up to
>>> Stoppoint. At a minimum I would expand the Stoppoint API to include
>>> features which should logically be available to Breakpoint, Watchpoint, a
>>> hypothetical Finishpoint, and any future *point class. I would also try to
>>> refactor the Breakpoint support classes (e.g. BreakpointID) to derive from
>>> a new class (i.e. StoppointID) and kick functionality up the chain there as
>>> well. This would include methods like GetDescription, SetOptions,
>>> GetOptions, AddName, and so on.
>> Again, most of the overlap isn’t actually in the breakpoint and watchpoint
>> classes, but in the Options classes. After all, the setting of watchpoints
>> and breakpoints are very different, but that they have conditions and
>> callbacks are very similar. Keeping the setting part separate and sharing
>> the reaction to hitting the stop points seems to more closely model the
>> objects better.
>>
>>> * Review and clean the source docs
>>>
>>> * Expand test coverage where that seems easy (perhaps there are tests
>>> hiding somewhere I didn't see?)
>> test/testcases/functionalities/breakpoint
>>
>> There are lots of tests there, but feel free to add tests as you go.
>>
>>> I would try to break private APIs minimally if at all. SB api will not be
>>> broken.
>>>
>>> This should go a long ways toward resolving problems 1, 2, 3, and 5.
>>>
>>> Phase 2: Restructure / Modernize the Private API / Implementation
>>> -----------------------------------------------------------------
>>>
>>> * Change Error& out parameters to Expected<ReturnType>.
>> Again, this is a larger policy change. We shouldn’t make different sections
>> of lldb handle errors differently.
>>
>>> * Get rid of all the const char* vars / args in favor of a better string
>>> type (which?)
>> StringRef’s seem to be the vogue these days.
>>
>>> * Prefer explicitly deleted copy ctor / assignments over multiline macro
>>> DISALLOW_COPY_AND_ASSIGN
>>> * Try to reduce the (perhaps excessive) use of friendship between the
>>> support classes. For instance, is it necessary to give BreakpointLocation
>>> access to private data members from BreakpointLocationList, Process,
>>> BreakpointSite, and StopInfoBreakpoint? Wouldn't it be better to expand
>>> the functionality of those other classes?
>>>
>>> A more significant change could be a rewrite of the callback functionality.
>>>
>>> There are some problems with the way callbacks are implemented in terms of
>>> maintainability and extensibility. I think that using templates and simple
>>> inheritance we could shift to using callback objects instead of function
>>> pointers. I have a crude code sketch of this plan in the works now, and I
>>> will submit that if there is interest in this idea. Essentially, the idea
>>> is to let the user define their own Breakpoint or Watchpoint (or whatever)
>>> child class which overrides a pure virtual run method from a parent
>>> StoppointCallback templated class. The template parameter of
>>> StoppointCallback would allow us to define a std::unique_ptr<UserData>
>>> member where the user can hang any data they want for the callback. That
>>> saves us from void pointers (which I find very awkward).
>>>
>>> template <class UserData>
>>> class StoppointCallback {
>>> private:
>>> std::unique_ptr<UserData> m_data;
>>> // ...
>>> };
>>>
>>> I also think that such a decision requires significant thought before we
>>> pull that trigger. It does constitute a significant addition to the SB
>>> api, tho I don't think it would break the SB api per se since we can always
>>> use overloading and template specialization to keep the old functionality.
>>> On the other hand, ABI compatibility may be a problem. I don't know that
>>> much about SWIG or the needs of LLDB from a ABI stability perspective, so
>>> please speak up if I'm suggesting something crazy.
>> We’ve been pretty strict about not requiring subclassing or specialization
>> of SB API’s classes to use them. We’re serious about maintaining binary
>> compatibility across lldb versions.
>>
>>> That said, it is somewhat difficult to keep the API consistent between
>>> Breakpoint and Watchpoint using function pointers; the signature associated
>>> with each will differ (e.g. Watchpoint should provide old and new values of
>>> the subject variable, while that does not apply to Breakpoint at all). I
>>> welcome any suggestions which allow us to avoid logic duplication. My goto
>>> just happens to be templates here.
>> I think you would call back into the watchpoint to get the old and new
>> values. I don’t think you would need more than the frame passed into the
>> watchpoint callback, and the location.
>
_______________________________________________
lldb-dev mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
