How to modify the code for $R+

[dummzeuch 1392]

I have inherited this code from a program originally written in Borland Pascal for DOS, which I have adapted to Delphi 32 Bit:

function BuildDigits(_Digits: UInt16; _Invalids: UInt8): UInt16;
var
  TempInt: Int16;
begin
  TempInt := _Digits;
  Dec(TempInt, 2048);
  TempInt := TempInt * 11;
  TempInt := (TempInt and $FFFC) or (_Invalids and $03);  // <-- crashes here
  TempInt := Swap16(TempInt);
  Move(TempInt, Result, SizeOf(Result));
end;

The adaptations were to change variable declarations: Word -> UInt16, Byte -> UInt8, Integer -> Int16 (Integer was 16 bits in Borland Pascal).

Only Integer -> Int16 was really necessary, the others were just to confuse^d^d^d^d^d^d^d^d^d^dclarify the code.

 

(The function Swap16 swaps the bytes of a 16 bit value, so $1234 becomes $3412.)

 

The code works fine if range checking is turned off {$R-}. But with range checking turned on {$R+}, it fails for e.g. _Digits = 0, because a range check error occurs in the line marked above.

Edit: The line the crash occurred was not the one multiplying with 11 but the one below.

 

I would like to be able to keep range checking enabled in debug mode, so how can I modify the code to return the same results as if range checking was disabled?

 

My idea was to change TempInt to an Int32 but I am unsure how to proceed.

 

(I'm getting old. I'm sure this wouldn't have posed a problem for me 10 years ago. Or maybe it's the heat and not enough sleep.)

Edited June 21, 2023 by dummzeuch

[Stefan Glienke 1905]

  {$IFOPT R+}{$DEFINE RANGECHECKS_ON}{$R-}{$ENDIF}
  TempInt := TempInt * 11;
  {$IFDEF RANGECHECKS_ON}{$UNDEF RANGECHECKS_ON}{$R+}{$ENDIF}

 

[Vandrovnik 202]

For input = 0, I would expect the problem already here:

Dec(TempInt, 2048);

[Stefan Glienke 1905]

And why exactly would it be a problem to subtract 2048 from 0 in a signed 16-bit integer?

Edited June 21, 2023 by Stefan Glienke

[Uwe Raabe 1890]

I also cannot see a problem with multiplying -2048*11 in a signed 16-bit integer either.

[dummzeuch 1392]

3 hours ago, Stefan Glienke said:

  {$IFOPT R+}{$DEFINE RANGECHECKS_ON}{$R-}{$ENDIF}
  TempInt := TempInt * 11;
  {$IFDEF RANGECHECKS_ON}{$UNDEF RANGECHECKS_ON}{$R+}{$ENDIF}

 

Yes, that's how I "solved" it for now, but it feels like a hack.

Edit: No, I put the whole function body into these, still feels like a hack.

Edited June 21, 2023 by dummzeuch

[dummzeuch 1392]

20 minutes ago, Uwe Raabe said:

I also cannot see a problem with multiplying -2048*11 in a signed 16-bit integer either.

Hm, yes, that's odd: -32678 <= -22528 <= 32767

Why didn't I see that?

 

That was the line the debugger stopped on and I didn't question it. I'll need to have a closer look.

 

Edit: The line the crash occurred was not the one multiplying with 11 but the one below:

TempInt := (TempInt and $FFFC) or (_Invalids and $03);

And that's probably because $FFFC is not an Int16.

(I just changed the original post to reflect that.)

Edited June 21, 2023 by dummzeuch

[dummzeuch 1392]

1 hour ago, Vandrovnik said:

For input = 0, I would expect the problem already here:

Dec(TempInt, 2048);

Why? -2048 is a perfectly fine value for an Int16.

[Vandrovnik 202]

24 minutes ago, Stefan Glienke said:

And why exactly would it be a problem to subtract 2048 from 0 in a signed 16-bit integer?

Eh, sorry, I have seen UInt16 in parameter definition and did not notice that temporary variable is Int16, without the "U"...

[Uwe Raabe 1890]

15 minutes ago, dummzeuch said:

And that's probably because $FFFC is not an Int16.

You might get away with some hard casting then:

TempInt := Int16((UInt16(TempInt) and $FFFC) or (_Invalids and $03));

I wonder if simply disabling range checks for the whole routine would be the best solution here.

[dummzeuch 1392]

4 minutes ago, Uwe Raabe said:

You might get away with some hard casting then:

TempInt := Int16((UInt16(TempInt) and $FFFC) or (_Invalids and $03));

I wonder if simply disabling range checks for the whole routine would be the best solution here.

I hoped for a better way.

 

This does the trick:

function BuildDigits(_Digits: UInt16; _Invalids: UInt8): UInt16;
var
  TempInt: Smallint; // 16 bit signed integer
begin
  TempInt := _Digits;
  Dec(TempInt, 2048);
  TempInt := TempInt * 11;
  Move(TempInt, Result, SizeOf(Result));
  Result := (Result and $FFFC) or (_Invalids and $03);
  Result := Swap16(Result);

It returns the same values for _Digits = 0 .. 5000 and _Invalids = 0 .. $FF.

Does anybody see anything wrong with this changed code?

 

Now I need to check what the actual range of _Digits is. I hope it is documented somewhere. I guess it's 0 .. 4095 because the value is read from an analogue digital converter. But I'm not sure whether any preprocessing has already happened.

 

(As said above: My excuse is that it's hot and that I haven't slept long enough.)

[Stefan Glienke 1905]

10 minutes ago, Uwe Raabe said:

I wonder if simply disabling range checks for the whole routine would be the best solution here.

Pretty much because the very first line already is subject to a range check error.

 

Many people have not noticed many potential range check errors because range and overflow checks were disabled even in debug config and only after they finally changed that they occur in a lot of code that unknowingly implicitly converts between signed and unsigned.

[dummzeuch 1392]

16 minutes ago, Stefan Glienke said:

Pretty much because the very first line already is subject to a range check error.

 

Many people have not noticed many potential range check errors because range and overflow checks were disabled even in debug config and only after they finally changed that they occur in a lot of code that unknowingly implicitly converts between signed and unsigned.

The point is that I want to find range check errors and fix them. That's why I have enabled them in the debug build (in Delphi 2007 where it was off by default).

[Stefan Glienke 1905]

4 minutes ago, dummzeuch said:

The point is that I want to find range check errors and fix them.

And my point was that you cannot assign signed to unsigned and vice versa without potentially getting them.

[dummzeuch 1392]

19 minutes ago, dummzeuch said:

Now I need to check what the actual range of _Digits is. I hope it is documented somewhere. I guess it's 0 .. 4095 because the value is read from an analogue digital converter. But I'm not sure whether any preprocessing has already happened.

It turns out that the range for _Digits is indeed 0 .. 4095 because that value comes from the following code:

  // 3. Project the value on 12 Bit (11 = 2 * $5800 / 4096)
  // (The value of the laser laser is -$5800 to +$5800.)
  Value := Round(DigitInt / 11);
  // 4. signed -> unsigned 12 Bit
  Inc(Value, 2048);
  if Value < 0 then
    Value := 0;
  if Value > 4095 then
    Value := 4095;
  
  Digits := Value; // <== This is where the digits value come from

So the changed function from above should do the trick as I tested it for _Digits = 0 .. 4095

[dummzeuch 1392]

1 minute ago, Stefan Glienke said:

And my point was that you cannot assign signed to unsigned and vice versa without potentially getting them.

... unless you know exactly which values you are dealing with.

[Stefan Glienke 1905]

Which you obviously didn't which is the raison d'être of this thread

[dummzeuch 1392]

13 minutes ago, Stefan Glienke said:

Which you obviously didn't which is the raison d'être of this thread

I know now.

 

I'm thinking about adding Assertions but only in debug mode. But that would make the code even less readable ... 😞

I could also use a sub range type.

[mytbo 5]

Question for the experts. Is there a reason not to write this way?

function BuildDigits(_Digits: UInt16; _Invalids: UInt8): UInt16;
var
  tmp: SmallInt absolute _Digits;
begin
  Dec(tmp, 2048);
  tmp := tmp * 11;
  _Digits := (_Digits and $FFFC) or (_Invalids and $03);
  Result := Swap(_Digits);
end;

With best regards

Thomas

[Uwe Raabe 1890]

20 minutes ago, mytbo said:

Is there a reason not to write this way?

Yes, it obfuscates what it is doing.

[Stefan Glienke 1905]

13 minutes ago, mytbo said:

Question for the experts. Is there a reason not to write this way?

function BuildDigits(_Digits: UInt16; _Invalids: UInt8): UInt16;
var
  tmp: SmallInt absolute _Digits;
begin
  Dec(tmp, 2048);
  tmp := tmp * 11;
  _Digits := (_Digits and $FFFC) or (_Invalids and $03);
  Result := Swap(_Digits);
end;

With best regards

Thomas

Actually, it's way simpler:

function BuildDigits(_Digits: UInt16; _Invalids: UInt8): UInt16;
begin
  _Digits := UInt16((_Digits - 2048) * 11);
  _Digits := (_Digits and $FFFC) or (_Invalids and $03);
  Result := Swap(_Digits);
end;

IIRC a smaller than register size unsigned ordinal type will not cause an overflow when it wraps around 0 so it's not necessary to turn it into a signed to avoid that. The CPU uses 32bit register math anyway. The UInt16 of the multiplication is necessary though.

[mytbo 5]

30 minutes ago, Stefan Glienke said:

Actually, it's way simpler:

function BuildDigits(_Digits: UInt16; _Invalids: UInt8): UInt16;
begin
  _Digits := UInt16((_Digits - 2048) * 11);
  _Digits := (_Digits and $FFFC) or (_Invalids and $03);
  Result := Swap(_Digits);
end;

IIRC a smaller than register size unsigned ordinal type will not cause an overflow when it wraps around 0 so it's not necessary to turn it into a signed to avoid that. The CPU uses 32bit register math anyway. The UInt16 of the multiplication is necessary though.

You are right. Thanks for the explanation. I didn't realize how easy it is to bypass range-checking.

 

With best regards

Thomas

[Remy Lebeau 1260]

8 hours ago, Stefan Glienke said:

  {$IFOPT R+}{$DEFINE RANGECHECKS_ON}{$R-}{$ENDIF}
  TempInt := TempInt * 11;
  {$IFDEF RANGECHECKS_ON}{$UNDEF RANGECHECKS_ON}{$R+}{$ENDIF}

 

It's a shame that Delphi does not have {$PUSH}/{$POP} directives like FreePascal has, that would clean up that code a bit, eg:

{$PUSH}{$R-}
TempInt := TempInt * 11;
{$POP}

This has already been requested in Delphi for many many years, but nothing ever came of it:

 

QC #56908: Save and restore compiler directives with push/pop

RSP-14045: {$BEGINOPT R+,Q-} ... {$ENDOPT}

RSP-38847: Implement stack-like push/pop for compiler directives to preserve/restore state

Edited June 21, 2023 by Remy Lebeau

[Lars Fosdal 1732]

Voted for RSP-38847.

[dummzeuch 1392]

Isn't there something like {$SomeOption default} to reset an option to the value given in the project options? I seem to remember reading about that at some time in the what's new with Delphi Xxxx.