View Issue Details

IDProjectCategoryView StatusLast Update
0035988FPCRTLpublic2019-08-31 13:42
ReporterBrunoKAssigned ToSven Barth 
PrioritynormalSeverityminorReproducibilityhave not tried
Status closedResolutionreopened 
Product Version3.3.1Product Build 
Target VersionFixed in Version3.3.1 
Summary0035988: 0031462: heap.inc: SysReAllocMem should call Sys* counterparts instead of MemoryManager fields
DescriptionThe change in revision 36769 is not complete.
I didn't test on 3.3.x but I have changed these things in my 3.0.6 version.
Not using the internal memory manager in reallocmem, like in the patch I submit, will call the patched heaptrc TraceGetMem when heaptrc is on.
TagsNo tags attached.
Fixed in Revision42774,42797
FPCOldBugId
FPCTarget-
Attached Files
  • heap.inc.patch (396 bytes)
    Index: rtl/inc/heap.inc
    ===================================================================
    --- rtl/inc/heap.inc	(revision 42743)
    +++ rtl/inc/heap.inc	(working copy)
    @@ -1549,7 +1549,7 @@
           p2 := SysGetMem(newsize);
           if p2<>nil then
             Move(p^,p2^,minsize);
    -      MemoryManager.FreeMem(p);
    +      SysFreeMem(p);
           p := p2;
     {$ifdef DUMP_MEM_USAGE}
         end else begin
    
    heap.inc.patch (396 bytes)
  • heap.inc_1.patch (835 bytes)
    Index: rtl/inc/heap.inc
    ===================================================================
    --- rtl/inc/heap.inc	(revision 42794)
    +++ rtl/inc/heap.inc	(working copy)
    @@ -273,10 +273,10 @@
     {$else HAS_MEMORYMANAGER}
     {$ifdef FPC_NO_DEFAULT_MEMORYMANAGER}
       Result:=false;
    -{$else not FPC_NO_DEFAULT_MEMORYMANAGER}
    +{$else}
       IsMemoryManagerSet := (MemoryManager.GetMem<>@SysGetMem)
         or (MemoryManager.FreeMem<>@SysFreeMem);
    -{$endif notFPC_NO_DEFAULT_MEMORYMANAGER}
    +{$endif FPC_NO_DEFAULT_MEMORYMANAGER}
     {$endif HAS_MEMORYMANAGER}
     end;
     
    @@ -1328,9 +1328,9 @@
     
     function SysAllocMem(size: ptruint): pointer;
     begin
    -  result := MemoryManager.GetMem(size);
    +  result := SysGetMem(size);
       if result<>nil then
    -    FillChar(result^,MemoryManager.MemSize(result),0);
    +    FillChar(result^,SysMemSize(result),0);
     end;
     
     
    
    heap.inc_1.patch (835 bytes)

Activities

BrunoK

2019-08-20 10:51

reporter  

heap.inc.patch (396 bytes)
Index: rtl/inc/heap.inc
===================================================================
--- rtl/inc/heap.inc	(revision 42743)
+++ rtl/inc/heap.inc	(working copy)
@@ -1549,7 +1549,7 @@
       p2 := SysGetMem(newsize);
       if p2<>nil then
         Move(p^,p2^,minsize);
-      MemoryManager.FreeMem(p);
+      SysFreeMem(p);
       p := p2;
 {$ifdef DUMP_MEM_USAGE}
     end else begin
heap.inc.patch (396 bytes)

BrunoK

2019-08-20 10:54

reporter   ~0117743

Let's add that since heaptrc does its own cooking of reallocmem, it doesn't surface but is still inconsistent.

Sven Barth

2019-08-23 17:01

manager   ~0117802

There was also another location nearby that used the MemoryManager that I've changed as well.

Please test and close if okay.

BrunoK

2019-08-24 12:25

reporter   ~0117822

While on the subject, and following the logic of the last patch, I suspect we still fail in SysAllocMem.
Not tested patch on your last patch : heap.inc_1.patch
1° Part because I find it clearer.
2° What I think SysAllocMem should be (Will try later in my 3.0.6).

If it has no merit, will close this issue on your next fixed/resolved.

In trunk heaptrc.pp there is a potential trouble with function TraceAllocMem(size:ptruint):Pointer;
It looks that it is pure chance that it works, at least the block obtained from SysAllocMem will never be traced in the current code.
I had the problem with my agnostic heaptrc.pp version. (Agnostic -> being insensitive to the memory manager, heap.inc or cmem.pp and hopefully within a week with a new windows winheap.pp TMemoryManager).

BrunoK

2019-08-24 12:25

reporter  

heap.inc_1.patch (835 bytes)
Index: rtl/inc/heap.inc
===================================================================
--- rtl/inc/heap.inc	(revision 42794)
+++ rtl/inc/heap.inc	(working copy)
@@ -273,10 +273,10 @@
 {$else HAS_MEMORYMANAGER}
 {$ifdef FPC_NO_DEFAULT_MEMORYMANAGER}
   Result:=false;
-{$else not FPC_NO_DEFAULT_MEMORYMANAGER}
+{$else}
   IsMemoryManagerSet := (MemoryManager.GetMem<>@SysGetMem)
     or (MemoryManager.FreeMem<>@SysFreeMem);
-{$endif notFPC_NO_DEFAULT_MEMORYMANAGER}
+{$endif FPC_NO_DEFAULT_MEMORYMANAGER}
 {$endif HAS_MEMORYMANAGER}
 end;
 
@@ -1328,9 +1328,9 @@
 
 function SysAllocMem(size: ptruint): pointer;
 begin
-  result := MemoryManager.GetMem(size);
+  result := SysGetMem(size);
   if result<>nil then
-    FillChar(result^,MemoryManager.MemSize(result),0);
+    FillChar(result^,SysMemSize(result),0);
 end;
 
 
heap.inc_1.patch (835 bytes)

Sven Barth

2019-08-30 14:51

manager   ~0117876

I've done the change to SysAllocMem already independently of your reopening in r42797 after fixing TraceAllocMem in heaptrc in r42796 cause the change to SysAllocMem did indeed lead to failures when running the testsuite (some tests use heaptrc, thus the problem was spotted there).

I'm ignoring the first part however.

BrunoK

2019-08-31 13:42

reporter   ~0117894

Lets close that issue, I'm already much further with agnostic HeapTrc and winheap TMemoryManager unit. And also memory manager loading first followed by heap trace before any other unit, that hasn't caused any evident trouble for the last 2 weeks.

Issue History

Date Modified Username Field Change
2019-08-20 10:51 BrunoK New Issue
2019-08-20 10:51 BrunoK File Added: heap.inc.patch
2019-08-20 10:54 BrunoK Note Added: 0117743
2019-08-23 16:35 Sven Barth Assigned To => Sven Barth
2019-08-23 16:35 Sven Barth Status new => assigned
2019-08-23 17:01 Sven Barth Status assigned => resolved
2019-08-23 17:01 Sven Barth Resolution open => fixed
2019-08-23 17:01 Sven Barth Fixed in Version => 3.3.1
2019-08-23 17:01 Sven Barth Fixed in Revision => 42774
2019-08-23 17:01 Sven Barth FPCTarget => -
2019-08-23 17:01 Sven Barth Note Added: 0117802
2019-08-24 12:25 BrunoK Status resolved => feedback
2019-08-24 12:25 BrunoK Resolution fixed => reopened
2019-08-24 12:25 BrunoK Note Added: 0117822
2019-08-24 12:25 BrunoK File Added: heap.inc_1.patch
2019-08-30 14:51 Sven Barth Status feedback => resolved
2019-08-30 14:51 Sven Barth Fixed in Revision 42774 => 42774,42797
2019-08-30 14:51 Sven Barth Note Added: 0117876
2019-08-31 13:42 BrunoK Status resolved => closed
2019-08-31 13:42 BrunoK Note Added: 0117894