Kernel.Time

[Robert]

I have just looked at Kernel.Time.

It has some logic for detecting when the 32 bit counter overflows. This logic looks suspicious (wrong !) to me, I think it may be confusing signed and unsigned arithmetic.
Since this problem would only occur every 25 days or 50 days it may never have been noticed.

Can someone give me a second opinion.

[cfbsoftware]

I agree - I think. What it is trying to do is extend the counter from a 32-bit value to a 64-bit value. Every time bit 31 of GetTickCount transitions from 0 to 1 since the last time it was retrieved (i.e. t goes negative), the value of the most significant 32 bits of the result returned by Time is incremented by one. Instead it should be doing this when bit 31 transitions from 1 to 0 shouldn't it?

If and when Windows XP is no longer supported in BlackBox the WinAPI function GetTickCount64 could be used instead and there would be less need to look for wraparound

[Ivan Denisov]

The similar problem was detected here: http://forum.blackboxframework.org/view ... =152#p1602
I am not sure, does this bug was detected experimentally or theoretically.
However some control systems and servers can run much more when 50 days, so that problem with time is serious.

[Ivan Denisov]

Code: Select all

	PROCEDURE Time* (): LONGINT;
		VAR t: INTEGER;
	BEGIN
		t := WinApi.GetTickCount();
		IF t < told THEN INC(shift) END;
		told := t;
		RETURN shift * 100000000L + t
	END Time;

There is the assumption, that Time will be called at list once in 50 days. For framework we can be sure, that it will happen.

[cfbsoftware]

Code: Select all

	PROCEDURE Time* (): LONGINT;
		VAR t: INTEGER;
	BEGIN
		t := WinApi.GetTickCount();
		IF t < told THEN INC(shift) END;
		told := t;
		RETURN shift * 100000000L + t
	END Time;

There is the assumption, that Time will be called at list once in 50 days. For framework we can be sure, that it will happen.

That is the code that I (and Robert?) think is wrong - even it is called within 50 days since the last time. See my previous message. I haven't tested it but I think the correction would be to replace the statement:

Code: Select all

 IF t < told THEN INC(shift) END;

with something like:

Code: Select all

 IF (told < 0) & (t >= 0) THEN INC(shift) END;

It would still be a problem if it wasn't called in a 50-day period. To fix both problems you should use WinApi.GetTickCount64 instead.

[Robert]

The similar problem was detected here: http://forum.blackboxframework.org/view ... =152#p1602.

A very quick reading of this thread gives the impression that Chris was suggesting replacing one duff 32-bit error with another!
What was the final decision here - was it to use Kernel.Time?

I have run a test with two proposed solutions.

The existing Oms code:

Code: Select all

omsFlg  :=  t < tOld

Proposed solution 1:

Code: Select all

rdcFlg  :=  (t >= 0)  &  (tOld  < 0)

Efficient (?) assembler solution

Code: Select all

TYPE
  Cardinal*  =  INTEGER;

PROCEDURE [c.o.d.e] Cmp* (a, b : Cardinal) : SET
  05AH,	(*  pop	b          ->  edx    *)
  039H, 0D0H,	(*  cmp	eax - edx  ->  flags  *)
  09FH;	(*  lahf	flags      ->  ah     *)


PROCEDURE  GtC* (a, b : Cardinal) : BOOLEAN;
  CONST
    cOutFlag  =   8;
    zeroFlag  =  14;
  BEGIN
    RETURN  {cOutFlag, zeroFlag} * Cmp (a, b) = {}
  END  GtC;

        asmFlg  :=  GtC (tOld, t)

You will probably not want to use the assembler solution, but I could not resist the opportunity to test a fragment of my "Cardinals" library!

The test:

Code: Select all

PROCEDURE  DoTime*;	(*  Kernel.Time  *)
  VAR
    t, tOld, p, k           :  INTEGER;
    omsFlg, rdcFlg, asmFlg  :  BOOLEAN;
  BEGIN
    FOR  p  :=  1  TO  0  BY  -1  DO
      t  :=  p * MAX (INTEGER) - 5;
      FOR  k  :=  0  TO  8  DO
        tOld    :=  t; INC (t);
        omsFlg  :=  t < tOld;
        rdcFlg  :=  (t >= 0)  &  (tOld  < 0);
        asmFlg  :=  GtC (tOld, t);
        f.Int (t, 12);
        f.StrBool   ('   ', omsFlg);
        f.StrBool   ('   ', rdcFlg);
        f.StrBoolLn ('   ', asmFlg)
      END;
      f.Ln
    END
  END  DoTime;

The results

Code: Select all

  2147483643    FALSE    FALSE    FALSE
  2147483644    FALSE    FALSE    FALSE
  2147483645    FALSE    FALSE    FALSE
  2147483646    FALSE    FALSE    FALSE
  2147483647    FALSE    FALSE    FALSE
 −2147483648    TRUE     FALSE    FALSE
 −2147483647    FALSE    FALSE    FALSE
 −2147483646    FALSE    FALSE    FALSE
 −2147483645    FALSE    FALSE    FALSE

          −4    FALSE    FALSE    FALSE
          −3    FALSE    FALSE    FALSE
          −2    FALSE    FALSE    FALSE
          −1    FALSE    FALSE    FALSE
           0    FALSE    TRUE     TRUE 
           1    FALSE    FALSE    FALSE
           2    FALSE    FALSE    FALSE
           3    FALSE    FALSE    FALSE
           4    FALSE    FALSE    FALSE

[cfbsoftware]

The similar problem was detected here: http://forum.blackboxframework.org/view ... =152#p1602.

A very quick reading of this thread gives the impression that Chris was suggesting replacing one duff 32-bit error with another!

Sorry - I don't follow you. Are you talking about the GetTickCount64 proposal?

[Robert]

The similar problem was detected here: http://forum.blackboxframework.org/view ... =152#p1602.

A very quick reading of this thread gives the impression that Chris was suggesting replacing one duff 32-bit error with another!

Sorry - I don't follow you. Are you talking about the GetTickCount64 proposal?

Sorry - I read the thread too quickly and was talking nonsense.

I thought you were suggesting using Kernel.Time on the assumption that it was a correct implementation.
In fact you were proposing changing Kernel.Time to use a more recent Microsoft function.

Is GetTickCount64 a good solution here? I can't find it in WinApi.

[cfbsoftware]

Is GetTickCount64 a good solution here? I can't find it in WinApi.

Yes - but the Microsoft documentation leads me to believe GetTickCount64 wasn't introduced until Windows Vista. XP is no longer supported by Microsoft so I'm not worried if Windows XP is not supported in the latest versions of BlackBox but I suspect others might protest.

[Josef Templ]

I agree - I think. What it is trying to do is extend the counter from a 32-bit value to a 64-bit value. Every time bit 31 of GetTickCount transitions from 0 to 1 since the last time it was retrieved (i.e. t goes negative), the value of the most significant 32 bits of the result returned by Time is incremented by one. Instead it should be doing this when bit 31 transitions from 1 to 0 shouldn't it?

If and when Windows XP is no longer supported in BlackBox the WinAPI function GetTickCount64 could be used instead and there would be less need to look for wraparound

I cannot find a bug in Kernel.Time.
Can you give a test case where the bug shows up?

No speculations, please. I am asking for a real test case.

- Josef