synopse / mORMot2

OpenSource RESTful ORM/SOA/MVC Framework for Delphi and FreePascal
https://synopse.info
Other
531 stars 132 forks source link

Can cause exception when setting the counter #280

Closed dkounal closed 3 months ago

dkounal commented 3 months ago

Random32 returns cardinal and this can cause overflow to a 32bit integer _TmpCounter.

synopse commented 3 months ago

Does %x raise an exception in format()? I am very doubtful about it. On which compiler? It does not on FPC nor Delphi 7, 2007, XE4 or XE7. For -1, it returns FFFFFFFF on all platforms, as it should. Also InterlockedIncrement() won't raise any exception. And runtime overflow checks are disabled in this unit.

We may just use Random31 do be sure, but I don't see where the exception could be.

dkounal commented 3 months ago

when _tmpcounter is set for the first time, if the random32 is a positive bigger than the max allowed positive integer, it gives an exception. It happened to me. Win32 program with delphi 12 compiler. random31 could be an acceptable solution also. I am always learning new functions .....

synopse commented 3 months ago

An integer variable can NOT contain anything bigger than a 32-bit value. It just rounds towards zero, as negative number, and it makes no difference for %x in format(). I just tested with Delphi 11.2 with no problem:

function rnd: cardinal;
begin
  result := cardinal(MaxInt) + 10;
end;

var i: integer;
begin
  i := rnd;
  interlockedincrement(i);
  writeln(format('%d=%x', [i, i])); 

Which exception did you have? I you changed mormot.defines.inc to enable runtime overflow, it is an error: you should NOT do that.

dkounal commented 3 months ago

The return of random32 is a positive 32bit cardinal. Please run the following: var n,_TmpCounter:integer; begin for n:=0 to high(integer) do _TmpCounter:=random32;

synopse commented 3 months ago

If runtime overflow are disabled, which IS THE CASE with this line of mormot.define.inc there is no problem with this code: {$Q-} // disable overflow checking in our code

I have NO exception on my side running your loop.

synopse commented 3 months ago

We need to find out what happens, and why. Current code sounds just correct to me.

Did you change mormot.define.inc content to remove {$Q-}?

dkounal commented 3 months ago

I did not remove {$Q-} but I am running it inside IDE. I change the commit with random31. It works ok with that

synopse commented 3 months ago

Yes, but I can't reproduce your problem here. This is a real concern. Something is wrong somewhere.

synopse commented 3 months ago

If 32-bit overflow fails in this place, there are plenty of other places in our code where we RELY on this standard and well-defined overflow to take place.

dkounal commented 3 months ago

I can not find something else wrong to the following code: unit Unit2;

interface

uses Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics, Vcl.Controls, Vcl.Forms, Vcl.Dialogs, Vcl.StdCtrls;

type TForm2 = class(TForm) Button1: TButton; procedure Button1Click(Sender: TObject); private { Private declarations } public { Public declarations } end;

var Form2: TForm2;

implementation uses mormot.core.base; {$R *.dfm}

procedure TForm2.Button1Click(Sender: TObject); var n,m:integer; begin for n:=0 to high(integer) do m:=random32;

end;

end.

dkounal commented 3 months ago

εικόνα

dkounal commented 3 months ago

The following works OK, but m can have also negative values for n:=0 to high(integer) do m:=integer(random32);

dkounal commented 3 months ago

Interesting it does not happen with the same code in FPC 3.2.3 x86_64-win64-win32/win64 it is just like running the m:=integer(random32); in FPC. To be honest, my way of thinking, expected this error: random32 returns a cardinal number that the compiler has to fit it in an integer. To convert it to negative number to fit in memory, it is a different number...

dkounal commented 3 months ago

when enabling the range check in FPC, I have the same exception and the same results. In Delphi I did not change anything in mormot.define.inc but the project options in debug mode have range checking and probably {$Q-} is ignored. Anyway, in our case, do we need a negative value in tmpcounter?

synopse commented 3 months ago

Perhaps they did change something in Delphi 12 - but I don't have access to this compiler. I can't reproduce the problem with Delphi 11.2 Alexandria or before.

In all ABIs I know (Intel/AMD or ARM) the 32-bit result is passed within a register or a memory location, without any notification about its sign. It is only when an operation is done that the sign could make a difference, but never overflow anyway at CPU level. Overflow may be checked by the compiler at runtime, but it is slower (it involves additional code) and not necessarily safer, because sometimes we don't care, or don't want to care and rely on the overflow to happen, or the sign to be changed.

If you find something weird elsewhere with Delphi 12, please report in our forum - it is easier to discuss.

synopse commented 3 months ago

Confirmation: the ERangeCheck did happen in your code in debug mode, as expected. The compiler create some additional code to check the range at runtime.

If {$R-} is defined, as in mormot.defines.inc, then you don't have this exception. So your exception should never be raised in mormot.core.os.pas because it has {$R-} defined.

I have made it explicit in our code: https://github.com/synopse/mORMot2/commit/81e13277