Oleg-N-Cher / OfrontPlus

Oberon family of languages to C translator for ARM, x64 and x86 architectures
Other
57 stars 11 forks source link

GC bug: no call to Heap.GC if lockdepth is too deep #86

Closed Oleg-N-Cher closed 4 years ago

Oleg-N-Cher commented 4 years ago

This bug was found by Heiner Schwarte. I am very grateful for his search in solving this problem. I also thank Dave Brown for his assistance.

Here is an example sent by Heiner that took up too much memory when it was working.

(* Heiner Schwarte, 9 June 2020 *)
(* Memory Management illustration *) 

(* for ofront 1.4: Out -> Console *)
MODULE Examp;
   IMPORT Out;
   TYPE
      Vec = POINTER TO ARRAY OF INTEGER;
   PROCEDURE alloc(size, times: INTEGER);
      VAR X: Vec;
     n,k: INTEGER;
   BEGIN
      FOR k:= 0 TO times-1 DO
     NEW(X,size);
     FOR n:=0 TO size-1 DO
        (* use memory *)
        X[n]:=n;
     END;
      END
   END alloc;
   PROCEDURE run();
      VAR n: INTEGER;
      CONST
     SIZE=10000;
     TIMES = 1000;
   BEGIN
      Out.String("Start");
      Out.Ln;
      FOR n:= 0 TO TIMES DO
     IF n MOD 10 = 0 THEN
        (* heart beat *)
        Out.Int(n, 0);
        Out.Ln;
     END;
     (* nested loop, for 2 byte INTEGER *)
     alloc(SIZE, TIMES);
      END;
      Out.String("End");
      Out.Ln; 
   END run;
BEGIN
   run();
END Examp.
Oleg-N-Cher commented 4 years ago

I'm very impressed with the way Heiner famously handled this issue, narrowing down our search. Yes, indeed, these are two different types of blocking. And I in my correction tend to do as done in Ofront. But I would call the variable not "gclock", but "lockGC". Why? It seems to me that the name "gclock" has a great association with a clock, what can be confusing and misleading.

Heiner, you did all the work for us. Respect!

Notes on Ofront: I can't find in the code where SYSTEM_lock is initialized.

This variable is defined in SYSTEM.c0 as

LONGINT SYSTEM_lock;

As far as we know from the C standard, only a static variable is guaranteed to have a value of 0 at the beginning of the code runs.

However, the variable SYSTEM_lock does not have a 'static' modifier. What can we assume here? We're not surprised that the variable will most often be equal to 0, but this is not guaranteed. So it would be good to initialize it explicitly.

The same about the variable SYSTEM_interrupted.

By the way, Dave took this point into account and described this assignment explicitly (in procedure InitHeap):

interrupted := FALSE;

But there is no such code in Ofront.

Oleg-N-Cher commented 4 years ago

image