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.
--
_______________________________________________
lazarus mailing list
lazarus@lists.lazarus-ide.org
https://lists.lazarus-ide.org/listinfo/lazarus
