I started about "sub watches" before reading your "every watch has a reference" ....
Joost van der Sluis wrote 12 lines
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.
Show quoted text 6 lines
> 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)
Show quoted text 10 lines
> 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 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.
Show quoted text 8 lines
> 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.
Show quoted text 25 lines
> 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.
Show quoted text 8 lines
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).
