All Projects FPC fpcprojects fpprofilerfpGUI Lazarus Packages PatchesLazarus CCR Mantis pas2js

View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0038534LazarusDebuggerpublic2021-02-23 07:372021-02-27 11:12
ReporterChristo Crause Assigned ToMartin Friebe  
PriorityhighSeverityminorReproducibilityhave not tried
Status closedResolutionfixed 
Product Version2.1 (SVN) 
Target Version2.2Fixed in Version2.2 
Summary0038534: Register view does not update when using gdbmidebugger
DescriptionA suspected side effect of r62591 is that gdbmidebugger doesn't update register information after the first evaluation. Although changed register entries are marked as modified, this property is not checked in UpdateFormat. To fix this I changed setting the Modified property for changed entries to DataValidity := ddsInvalid.

Patch attached.
Tagsregression
Fixed in Revision60267
LazTarget2.2
Widgetset
Attached Files

Activities

Christo Crause

2021-02-23 07:37

reporter  

registerupdatefix.patch (936 bytes)    
diff --git a/components/lazdebuggergdbmi/gdbmidebugger.pp b/components/lazdebuggergdbmi/gdbmidebugger.pp
index 97d5810bb6..e2b13d503e 100644
--- a/components/lazdebuggergdbmi/gdbmidebugger.pp
+++ b/components/lazdebuggergdbmi/gdbmidebugger.pp
@@ -2107,11 +2107,11 @@ begin
 
       if ChangedRegList <> nil then begin
         for i := 0 to FRegisters.Count - 1 do
-          FRegisters[i].Modified := False;
+        FRegisters[i].DataValidity := ddsValid;
         for i := 0 to ChangedRegList.Count - 1 do begin
           idx := StrToIntDef(Unquote(ChangedRegList.GetString(i)), -1);
           if (idx < 0) or (idx > High(FGDBMIRegSupplier.FRegNamesCache)) then Continue;
-          FRegisters.EntriesByName[FGDBMIRegSupplier.FRegNamesCache[idx]].Modified := True;
+          FRegisters.EntriesByName[FGDBMIRegSupplier.FRegNamesCache[idx]].DataValidity := ddsInvalid;
         end;
         FreeAndNil(ChangedRegList);
       end;
registerupdatefix.patch (936 bytes)   

Christo Crause

2021-02-23 18:11

reporter   ~0129125

Apologies, the first patch killed the modified register status indication. The second patch fixes the register updates while retaining the modified register indication.
registerupdatefix2.patch (1,049 bytes)    
diff --git a/components/lazdebuggergdbmi/gdbmidebugger.pp b/components/lazdebuggergdbmi/gdbmidebugger.pp
index 97d5810bb6..6a66624714 100644
--- a/components/lazdebuggergdbmi/gdbmidebugger.pp
+++ b/components/lazdebuggergdbmi/gdbmidebugger.pp
@@ -2106,12 +2106,15 @@ begin
       FRegisters.DataValidity := ddsValid;
 
       if ChangedRegList <> nil then begin
-        for i := 0 to FRegisters.Count - 1 do
+        for i := 0 to FRegisters.Count - 1 do begin
           FRegisters[i].Modified := False;
+          FRegisters[i].DataValidity := ddsValid;
+        end;
         for i := 0 to ChangedRegList.Count - 1 do begin
           idx := StrToIntDef(Unquote(ChangedRegList.GetString(i)), -1);
           if (idx < 0) or (idx > High(FGDBMIRegSupplier.FRegNamesCache)) then Continue;
           FRegisters.EntriesByName[FGDBMIRegSupplier.FRegNamesCache[idx]].Modified := True;
+          FRegisters.EntriesByName[FGDBMIRegSupplier.FRegNamesCache[idx]].DataValidity := ddsInvalid;
         end;
         FreeAndNil(ChangedRegList);
       end;
registerupdatefix2.patch (1,049 bytes)   

Dimitrios Chr. Ioannidis

2021-02-25 12:39

reporter   ~0129158

I confirm that it works for me.

Martin Friebe

2021-02-26 23:40

manager   ~0129187

Investigating.

I don't currently get why
           FRegisters.EntriesByName[FGDBMIRegSupplier.FRegNamesCache[idx]].Modified := True;
+ FRegisters.EntriesByName[FGDBMIRegSupplier.FRegNamesCache[idx]].DataValidity := ddsInvalid;

If we set ddsInvalid, then the value of Modified becomes ddsInvalid, so why set it first?

--------------
I do see, that registers are not correctly invalidated.
And as they stay valid forever, they never are set again.

Normally, when Leaving dsPause => all registers should become invalid....
But I have to go back and find out what was changed and why.

Martin Friebe

2021-02-27 02:05

manager   ~0129189

Please test and close if ok.
The current way the registers work probably needs a big redesign.

Each Stackframe (and per each thread) has its own storage.
Note here that stackframe 0 is the top, when you step out the former frame 1 does NOT becomes frame 0 => instead the existing frame 0 is applied to the former frame 1.

That means that if you change RAX to hex, you need to do so for each frame/thread. And when you step out, the loose alignment.
That also means internally a lot of duplicated data that must be kept.

See also the TODO notes I inserted.

Christo Crause

2021-02-27 11:12

reporter   ~0129196

Thanks Martin! Yes, the current management of register information is a bit awkward.

Issue History

Date Modified Username Field Change
2021-02-23 07:37 Christo Crause New Issue
2021-02-23 07:37 Christo Crause Status new => assigned
2021-02-23 07:37 Christo Crause Assigned To => Martin Friebe
2021-02-23 07:37 Christo Crause File Added: registerupdatefix.patch
2021-02-23 18:11 Christo Crause Note Added: 0129125
2021-02-23 18:11 Christo Crause File Added: registerupdatefix2.patch
2021-02-25 12:39 Dimitrios Chr. Ioannidis Note Added: 0129158
2021-02-26 15:57 Martin Friebe Priority normal => high
2021-02-26 15:57 Martin Friebe Target Version => 2.2
2021-02-26 15:57 Martin Friebe LazTarget => 2.2
2021-02-26 15:57 Martin Friebe Tag Attached: regression
2021-02-26 23:40 Martin Friebe Note Added: 0129187
2021-02-27 02:05 Martin Friebe Status assigned => resolved
2021-02-27 02:05 Martin Friebe Resolution open => fixed
2021-02-27 02:05 Martin Friebe Fixed in Version => 2.2
2021-02-27 02:05 Martin Friebe Fixed in Revision => 60267
2021-02-27 02:05 Martin Friebe Note Added: 0129189
2021-02-27 11:12 Christo Crause Status resolved => closed
2021-02-27 11:12 Christo Crause Note Added: 0129196