[Lazarus] Playing with debuggers

[Martin Frb]

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:
>> But ideally this should just replace/extend the existing watches. Of 
>> course that also would need the debugger intf to be updated.  (That 
>> does need an overhaul, and yes it could mean all existing debuggers 
>> will need a bit of work to follow / ideas welcome)
>
> What I did now, is add new dialogs, and created an debug-interface 
> separate from the existing one. But with the idea that it could 
> replace/be merged with the existing one.
>
> What we could also do is make the debug-windows (watches, locals, 
> evaluation pluggable, so that a debugger can choose which one to use.)

Actually all it would need is for the watch to have a new method, and to 
have a flag if it can be expanded. (a debugger that can not do that, 
will not set the flag).

However, it is a bit more....

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).

The problem of not being in the "watches" exists for "debug inspector". 
It does not always update correctly. Adding to the "watches" would 
however mean that each watch needs some sort of owner, so the 
watch-window does not display watches added by others. So that ends up 
more rework.... (and maybe there is a better way)

Of course instead of adding new watch to the "watches", those watches 
can be hold as a sublist by each watch.

Note that watches for structures already have a list, with a description 
of each member field. (if the watch request had the correct flag set). 
But those list should probably be reworked...

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.

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

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


>
>> 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)


>> 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 )


>
> 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 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.


>
>> 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.

>
>> 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

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.


>
> 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.


>
> 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).