[Lazarus] Playing with debuggers

[Joost van der Sluis]

Op 14-09-2021 om 14:27 schreef Martin Frb via lazarus:
> I started about "sub watches" before reading your "every watch has a 
> reference" ....
> 
> 
> On 14/09/2021 11:00, Joost van der Sluis via lazarus wrote:
>> Op 14-09-2021 om 02:18 schreef Martin Frb via lazarus:

> 1)
> All watches should be part of the "watches" list. (or alternative means 
> must be found). This is so the debugger can update them, if the user 
> changes memory (set a new value to a variable).
> This is also helpful for the history window, and for "idle eval" (if the 
> watches window is closed but the debugger is idle, then eval starts 
> anyway).

Yes, as you look carefully at the screencast, you'll see that the 
history-option is removed. ;)

And idle... I didn't really thought about that. My idea was that the gui 
is the most 'responsible' place to determine when to request data. And 
in this case this is the dialog itself. The dialogs in the screencast 
fetch their data only when they have to be showed, just to make it fast.

Catching data in advance when the debugger is idle slightly contradict 
with that approach. Although, the 'gui'/dialogs can always decide to do 
so. But in that case a 'global cache' of retrieved variables might be 
useful, yes.

> 2)
> Currently formatting  (hex vs dec) is done in the debugger. That is also 
> nonsense. As much as can be should be moved to the frontend.

Yes, I agree. (But for the fpdserver-backend I use the DAB-protocol 
which has all kind of issues with this)

> 3)
> Btw, if arrays can be expanded, precautions are needed. I don't think we 
> want to expand 100000 child values.

The fpdserver has a solution for that, I only did not implement it on 
the Lazarus-side. But it basically adds paging, together with the 
load-on-demand features of the treeview, this should be no problem.

> ----
> If we go for the flag and sub-watches, then "debug history" needs to be 
> aware of it.

Yup, that one needs work.

>>> Ideally the entire debugger frontend could move into a package of its 
>>> own too (all the current windows).
>>
>> Exactly, that is why I created a new debug-intf package instead of 
>> adapting the existing one.
> Well we may need to create an interface for the debugger frontend, and 
> then add the frontend package. Though DebugManager already has parts of 
> that.
> 
> On the other hand its not strictly mandatory yet. As I see it your new 
> watches can simply replace the old one. (as soon as watches provide that 
> flag, so old watches can be displayed too)

I'll see if I can implement that flag.

>>> Currently the debugger intf allows to fetch watches as plain text, or 
>>> as structure. But this has to be passed in as flag at the request time.
>>>
>>> Any watch object should instead have methods to fetch "sub watches" 
>>> so structures can unfold.
>>
>> What I have now is that each object has a 'reference', and there is a 
>> separate call to get the sub-watches that belong to the reference. 
>> That way the data (in the object) and the logic (function to retrieve 
>> sub-watches) are separated.
> Ok, I have to look at that.
> 
> Another important factor on the design of every debugger-object (eg 
> watches) is "notify on free" or TRefCountedObject.
> 
> Any debugger backend holds on to the "watch object" while it evaluates 
> the data needed. However the frontend may remove (and free/dereference) 
> that watch during that time. The backend must know that. (and 
> potentially even call the callback, if the object no longer exists.... / 
> the editor hint code requires this )

That's easy. The gui requests a variable using a reference. Once it 
retrieves the callback with the data that response contains the 
reference. If the gui does not need that reference anymore, just throw 
away the data.

The request to the backend only contains the reference. The response is 
a newly created object, that the receiving party gets ownership to. And, 
as long as there is no global-cache, a variable has only one 'owner'.

All the gui-related stuff goes into the main thread.

>>
>> I don't have a solution for requesting the result as text(string) 
>> only. But that's not a bad idea.
> Currently we have (as param to eval)
>    TDBGEvaluateFlag =
>      (defNoTypeInfo,        // No Typeinfo object will be returned
>       defSimpleTypeInfo,    // Returns: Kind (skSimple, skClass, ..); 
> TypeName (but does make no attempt to avoid an alias)
>       defFullTypeInfo,      // Get all typeinfo, resolve all anchestors
>       defClassAutoCast,     // Find real class of instance, and use, 
> instead of declared class of variable
>       defAllowFunctionCall
>      );
>    TDBGEvaluateFlags = set of TDBGEvaluateFlag;
> 
> Currently
> 
>    TWatchValue = class(TFreeNotifyingObject)
>       property TypeInfo: TDBGType;
> 
> and
> TDBGType = class(TObject)
>    property Fields: TDBGFields;
> which is a list of TDBGField
> 
> So if (full)typeinfo is request, you have a list of fields, and you can 
> then get the sub-watches.
> 
> But that is not a good way....
> That means you then have lots of dup data => each subwatch, has some 
> data also in a field.
> 
> ** The other downside is, that the gdb debugger needs more time to fill 
> the fields. So not every debugger should provide them by default.....

The big advantage of the tree-view, is that you do not evaluate 
sub-watches, only when they have to be shown. So only those fields that 
need to be shown have to be evaluated by gdb. (It also solves the 
endless loop you sometimes have)

> ** The upside is, that having type info (and fields) the frontend can do 
> all the formatting (almost... / mem dump still needs to get data as byte 
> stream)
> If the backend delivers a point (x,y) as 2 fields, the frontend can 
> decide to print "(x: 1; y:2)"  or "(1,2)". The frontend can also print 
> the individual values in dec,hex,bin,oct....
> 
> So always having type info would be nice.
> 
> But for gdb only remote debug targets, in needs to be seen if it can be 
> done, without slow down.
> It should be possible with one limitations.
> 
> Currently each Field has type (tkInteger, tkClass, tkRecord, ....) and 
> ClassName (indicating which parent class introduced that field).
> Getting those fields is time consuming.
> 
> ===================================
> So as a first draft I would aim for something like this:
> 
> TDBGField needs to be a TWatch. => avoid data duplication / and provide 
> for recursive eval.
> 
> By default the (if no flags are set) the debugger should provide fields. 
> But only field name, and a value (int or string).
> All other info can be retrieved later, on request.
> 
> In fact, even the field info can be retrieved on request (editor hint 
> may not need it, but if formatting code goes to the frontend, then 
> editor hint also needs to do the formatting).
> In the same way, that a watch has a "validity" field (dsEvaluating, 
> dsValid, dsError, ...) it can have validity for typeinfo and fields.
> 
> On top of that validity for typeinfo/fields can be dsNotAvail. In which 
> case there just is a default text value for the watch.

Let's not over complicate things (immediately). But let's see which 
functionality we need, and then come up with a design for that.

>>> I see you pass the variable to the debugger, that is ok. But that may 
>>> well get abstracted into the Watch, so the watch has its own ref to 
>>> the debugger, and makes the needed calls.
>>
>> I don't follow exactly. But if there's a need for an extra abstraction 
>> layer before sending the variable to the debugger directly, this could 
>> be added.
> See above, if the subwatches are already in place of the current fields, 
> then they have a ds...validity. And if accessed they will further evaluate.

Ok, let's dive something more into this particular case. I used flags 
for variables, for example to indicate that they are 'loading'. But at 
this moment I do not have an error-flag. When the 'gui' requests a 
variable from the backend, it can receive an error. In that cast it just 
fills in the error-message as the value of the variable and it's done.

Ok, when we want to have an history, this might not be what you want, 
but in that case the 'gui' (the history being part of the gui) can 
decide to do otherwise.

>>> Not sure about passing callbacks directly. Thinking of the current 
>>> notification system (data monitors).
>>> What does your new watch window do, if the user modifies a variable? 
>>> This triggers re-eval of all watches (so they need to unfold again).
>>> Currently the debugger backend can trigger the re-eval, and through 
>>> the data-monitors tell everyone about the updates.
>>>
>>> Have to look at it in more detail....
>>
>> I have something similar as the data-monitors.
>>
>> Main difference is that the events are based on what the gui needs, 
>> instead of what the debugger does.
>>
>> So: there is an event to tell the 'gui' that it is inpossible to show 
>> any data. (for example: the application has stopped or there is no 
>> debugger at all) There is an event for the case that debug-data has 
>> become available or unavailable. (Application paused or continued)
>>
>> And I should add an event for the case that the debug-info should be 
>> re-evaluated. (context switch, or variable-change). And I can think of 
>> another event that can be send when the debug-data could have been 
>> changed, due to unforeseen effect, for example when a propperty with a 
>> getter has been evaluated.
> All of those are available already

Yes, I know. Basicly everything I made is available already. But by 
changing a few paradigma's I hope that things can become less complex. 
(But time will tell)


> Most of those are DebuggerState changes. They are currently propagated 
> hardcoded to each of the existing listeners. So they may need an API to 
> subscribe to.
> But they also need to be done in a prioritized order.
> Certain data needs to be done in specific order (partly that is enforced 
> in the backend anyway)
> 
> Context changes are subscribe-able.
>    CallStackNotification.OnChange   := @CallStackChanged;
>    CallStackNotification.OnCurrent  := @CallStackCurrent;
>    BreakpointsNotification.OnAdd    := @BreakPointChanged;
>    BreakpointsNotification.OnUpdate := @BreakPointChanged;
>    BreakpointsNotification.OnRemove := @BreakPointChanged;
> 
> Note that the debugger is not the only thing that can change context. If 
> the user selects a history entry, then all watches change.

I was thinking about a user thanging the current entry in a call-stack, 
but it's basivly the same, yes.

>> The advantage of 'events' that are more geared towards the gui, is 
>> that it is easier to add catchy things. For example: to avoid 
>> flickering, when the gui receives an event that debug-data is not 
>> available anymore, it starts a timer of 200 mseconds, and only after 
>> that timer the data is removed from the screen. This way, when 
>> stepping through the code, there is no flickering in the small periods 
>> of time that the application runs.
> 
> That can be done with the above Notification too.
> 
>>
>> It also made it easy to highlight variables that changed.
> Ah, but does it?
> 
> Say I have an object with a delegate (object / class based => that is 
> pointer to heap).
> 
> TBar = class
>    i: integer;
> end;
> TFoo = class
>    bar: TBar;
> end;
> 
> foo.bar := otherBar.
> 
> If otherBar has the same value in "i", and if the address of "bar" in 
> tfoo is not displayed. => then the displayed result of the "foo" watch 
> does not change. The value however has changed.

True, but I don't think users will matter much. And, the field that 
displays the class itself also includes the address, and that one *will* 
change.

>> But atm they are only suitable for displaying variables. Stuff like 
>> the call-stack are not touched.
> 
> In generally there may be reasons to extend on the current 
> monitor/notification.
> However, I would avoid passing a callback to a function for that.
> 
> Rather adding a notification list [1] to each watch.
> So if a single watch change (updates any of its validities), we no 
> longer need to call the global watch changed.
> Downside, if all watches change (to invalid) we don't want to call 
> hundreds of callbacks.
> 
> So there are some design choices to be made.
> There needs to be a good design to split into notifications for 
> individual watches, and for groups of watches (usually the whole list)
> 
> 
> There could in future be more than one monitor.
> One idea is to allow to open more than one watches-window. So you can 
> display an history entry next to a live value. (or 2 values from 
> different stack frames).
> But then (even if not sensible) a user could  have both windows showing 
> the same data too (the list of watches is in TWatches, it is not bound 
> to the window).

Regards,

Joost.