6

I use simple circular buffer like this

var
  Values: array [byte] of single;
  ptr: byte;

In this test example

for ptr:=0 to 10 do Values[Byte(ptr-5)]:=1;

I expect to have set to 1 first 5 values and last 5 values, but XE4 compiller produce incorrect code, its using 32bit pointer math to calculate array index:

for ptr:=0 to 10 do Values[Byte(ptr-5)]:=1;
005B94BB C645FB00         mov byte ptr [ebp-$05],$00
005B94BF 33C0             xor eax,eax
005B94C1 8A45FB           mov al,[ebp-$05]
005B94C4 C78485E0FBFFFF0000803F mov [ebp+eax*4-$0420],$3f800000
005B94CF FE45FB           inc byte ptr [ebp-$05]
005B94D2 807DFB0B         cmp byte ptr [ebp-$05],$0b
005B94D6 75E7             jnz $005b94bf

Is it my wrong code and whats proper way to operate byte indexes?

kludg
  • 27,213
  • 5
  • 67
  • 118
Michael Gendelev
  • 471
  • 4
  • 16
  • That is a bug in the code generator. It "optimizes" the wrong way. See my answer. – Rudy Velthuis Jul 22 '16 at 23:57
  • Yes it is a compiler bug. Good catch. – kludg Jul 23 '16 at 01:58
  • Yes, good catch indeed – Rudy Velthuis Jul 23 '16 at 03:24
  • 2
    I don't agree. I think your code is wrong. Why do you expect wrap around? Code your own wrap around explicitly. – David Heffernan Jul 23 '16 at 05:36
  • @DavidHeffernan the code is correct. It is really strange to see this bug in an old mature compiler. – kludg Jul 23 '16 at 05:43
  • @serg I disagree. Show me the documentation that backs up your claim. – David Heffernan Jul 23 '16 at 05:46
  • @DavidHeffernan `Byte(ptr-5)` is a value in range `[0..255]`, and a valid array index. FPC is correct here. – kludg Jul 23 '16 at 05:50
  • @serg documentation? – David Heffernan Jul 23 '16 at 06:11
  • @DavidHeffernan, from documentation, [Platform-Independent Integer Types](http://docwiki.embarcadero.com/RADStudio/Seattle/en/Simple_Types#Platform-Independent_Integer_Types): `When you increment the last value or decrement the first value of an integer type, the result wraps around the beginning or end of the range.` snip `If compiler range-checking is enabled, however, this code generates a runtime error.` So, you are correct that it would generate a run-time error if range check is on, but a wrap is to be expected for arithmetic operations. – LU RD Jul 23 '16 at 06:15
  • @LURD - the code does not (and should not) generate runtime error with range check on. Tested in Delphi XE. – kludg Jul 23 '16 at 06:21
  • @serg that's the other side of the bug, that the runtime error is not produced. Did you read the documentation. – David Heffernan Jul 23 '16 at 06:35
  • @DavidHeffernan `Values[Byte(ptr-5)]` is perfectly valid code, with or without range check because of hard typecast. Range check error is (and should be) generated for `Values[ptr-5]`. – kludg Jul 23 '16 at 06:39
  • @serg did you read the documentation which seems to disagree with you – David Heffernan Jul 23 '16 at 06:52
  • @DavidHeffernan range check error is generated when you assign (to a variable of type `Byte`) a value which is out of its range. Documentation does not disagree with me, it is incomplete. – kludg Jul 23 '16 at 07:02
  • @serg, You are correct that an assignment causes the range error. See also [Cause of Range Check Error (Delphi)](http://stackoverflow.com/a/11658849/576719). – LU RD Jul 23 '16 at 07:15
  • @serg in that case I've still not seen any documentation for what the code in the question should do – David Heffernan Jul 23 '16 at 07:17
  • From `{$R+}` : `all array and string-indexing expressions are verified as being within the defined bounds, and all assignments to scalar and subrange variables are checked to be within range.` To me it says that both a wrong array index or a wrong assignment would cause a range check error. If a wrap was intended within the byte cast, an array range error would not have been produced. Since there is no assignment involved here, also no range error should have been produced. Conclusion is that a wrap is expected and the compiler generates wrong code. – LU RD Jul 23 '16 at 07:31
  • @LURD it is whether or not the wrap is expected that isn't 100% clear to me – David Heffernan Jul 23 '16 at 07:33
  • @DavidHeffernan well it is quite clear what the compiler should do calculating `Byte(ptr-5) (ptr=0)`: first the compiler is free to promote (for optimization) `ptr` variable of type `Byte` to 32-bit integer, then it evaluates `ptr-5` as 32-bit value '-5' in two-complement format (0xfffffffb), then it takes the last 8 bits (0xfb = 251). Wrap around thing works with or without type promotion. – kludg Jul 23 '16 at 07:38
  • @DavidHeffernan, This is creapy, `{$Q+}` makes the wrap work. – LU RD Jul 23 '16 at 07:40
  • @serg not clear to me anyway – David Heffernan Jul 23 '16 at 07:47
  • @serg also are you going to update or retract this: https://sergworks.wordpress.com/2012/09/26/why-stackoverflow-sucks/ – David Heffernan Jul 23 '16 at 07:48
  • 1
    @DavidHeffernan Should I? So deep and meaningful discussion going on where :) – kludg Jul 23 '16 at 07:52

3 Answers3

4

The question is:

Is a wrap expected within the Byte() cast?

Lets compare the disassembly with overflow checking on/off.

{$Q+}
Project71.dpr.21: for ptr:= 0 to 10 do Values[Byte(ptr-5)]:= 1;
0041D568 33DB             xor ebx,ebx
0041D56A 0FB6C3           movzx eax,bl
0041D56D 83E805           sub eax,$05
0041D570 7105             jno $0041d577
0041D572 E82D8DFEFF       call @IntOver
0041D577 0FB6C0           movzx eax,al
0041D57A C704870000803F   mov [edi+eax*4],$3f800000
0041D581 43               inc ebx
0041D582 80FB0B           cmp bl,$0b
0041D585 75E3             jnz $0041d56a

{$Q-}
Project71.dpr.21: for ptr:= 0 to 10 do Values[Byte(ptr-5)]:= 1;
0041D566 B30B             mov bl,$0b
0041D568 B808584200       mov eax,$00425808
0041D56D C7000000803F     mov [eax],$3f800000
0041D573 83C004           add eax,$04
0041D576 FECB             dec bl
0041D578 75F3             jnz $0041d56d

With {$Q+} the wraps works, while with {$Q-} the wrap does not work and the compiler does not generate a range error for the wrong array indexing when {$R+} is set.

So, to me the conclusion is: Since the range check on does not generate a run-time error for an array index out of bounds, a wrap is expected.

This is further proved by the fact that a wrap is done when overflow checking is on.


This should be reported as a bug in the compiler.

Done: https://quality.embarcadero.com/browse/RSP-15527 "Type cast fail within array indexing"


Note: a workaround is given by @Rudy in his answer.


Addendum:

Following code:

for ptr:= 0 to 10 do WriteLn(Byte(ptr-5));

generates:

251
252
253
254
255
0
1
2
3
4
5

for all combinations of range/overflow checking.

Likewise Values[Byte(-1)] := 1; assigns 1 to Values[255] for all compiler options.


The documentation for Value Typecasts says:

The resulting value is obtained by converting the expression in parentheses. This may involve truncation or extension if the size of the specified type differs from that of the expression. The expression's sign is always preserved.

LU RD
  • 34,438
  • 5
  • 88
  • 296
  • I had tested the cast too. If it is done separately, e.g. "I := Byte(ptr - 5); Values[I] := 1.0;", then all works well. Only the cast in the index goes wrong. – Rudy Velthuis Jul 23 '16 at 10:04
3

My code is written in Delphi 10.1 Berlin, but the result seems to be the same.

Let's extend your little code piece a little:

procedure Test;
var
  Values: array[Byte] of Single;
  Ptr: byte;
begin
  Values[0] := 1.0;
  for Ptr := 0 to 10 do
    Values[Byte(Ptr - 5)] := 1.0;
end;

This gives the following code in the CPU view:

Project80.dpr.15: Values[0] := 1.0;
0041A1DD C785FCFBFFFF0000803F mov [ebp-$00000404],$3f800000
Project80.dpr.16: for Ptr := 0 to 10 do
0041A1E7 C645FF00         mov byte ptr [ebp-$01],$00
Project80.dpr.17: Values[Byte(Ptr-5)] := 1.0;
0041A1EB 33C0             xor eax,eax
0041A1ED 8A45FF           mov al,[ebp-$01]
0041A1F0 C78485E8FBFFFF0000803F mov [ebp+eax*4-$0418],$3f800000
0041A1FB FE45FF           inc byte ptr [ebp-$01]
Project80.dpr.16: for Ptr := 0 to 10 do
0041A1FE 807DFF0B         cmp byte ptr [ebp-$01],$0b
0041A202 75E7             jnz $0041a1eb

As we can see, the first element of the array is at [ebp-$00000404], so [ebp+eax*4-$0418] is indeed below the array (for values 0..4).

That looks like a bug to me, because for Ptr = 0, Byte(Ptr - 5) should wrap around to $FB. The generated code should be something like:

    mov byte ptr [ebp-$01],$00
    xor eax,eax
@loop:
    mov al,[ebp-$01]
    sub al,5                        // Byte(Ptr - 5)
    mov [ebp+4*eax-$0404],$3f800000 // al = $FB, $FC, $FD, $FE, $FF, 00, etc..
    inc byte ptr [ebp-$01]
    cmp byte ptr [ebp-$01],$0b
    jnz @loop

Good find!

There is a workaround, though:

    Values[Byte(Ptr - 5) + 0] := 1.0;

This produces:

Project80.dpr.19: Values[Byte(Ptr - 5) + 0] := 1.0;
0040F16B 8A45FF           mov al,[ebp-$01]
0040F16E 2C05             sub al,$05
0040F170 25FF000000       and eax,$000000ff
0040F175 C78485FCFBFFFF0000803F mov [ebp+eax*4-$0404],$3f800000

And that works nicely, although the and eax,$000000ff seems unnecessary to me.

FWIW, I also looked at the code generated with optimization on. Both in XE and Berlin, the error exists as well, and the workaround works too.

Rudy Velthuis
  • 28,387
  • 5
  • 46
  • 94
  • The " and eax,$000000ff" is mandatory, otherwise you would have a buffer overflow, writing for sure in `Values[-5] Values[-4] Values[-3] Values[-2]Values[-1]`, which is incorrect. – Arnaud Bouchez Jul 23 '16 at 11:47
  • @Arnaud: No, it is not mandatory. The upper 24 bits of `EAX` are `0`, and `AL` automatically wraps around. So `SUB AL,5` will wrap, say `$02` to `$FD`. That means that the whole of `EAX` has the value `$000000FD`. That does not overflow/underflow. Changing `AL` does not affect the rest of `EAX`. – Rudy Velthuis Jul 23 '16 at 13:04
  • @Arnaud: Only in 64 bit, changing a register like `EAX` affects the upper 32 bits of `RAX`as well. **Otherwise**, addressing a sub-register does not affect the rest of the full register, not in 32 bit, not in 64 bit Intel/AMD, at least if it is a general purpose register. – Rudy Velthuis Jul 23 '16 at 13:12
  • You are right, I overlooked the asm and assumed the asm generated was "sub eax,5", whereas it was in fact "sub al,5". – Arnaud Bouchez Jul 25 '16 at 13:42
1

Sounds like an unexpected behavior of the compiler. But I never assume that transtyping integers using byte() would always make a rounding around $ff. It does, most of the time, e.g. if you assign values between variables, but there are cases where it doesn't - as you discovered. So I would have never used this byte() expression within an array index computation.

I always observed that using byte variables is not worth it, and you should rather use plain integer (or NativeInt), so that it matches the CPU registers, and then don't assume any complex rounding.

In all cases, I would rather make the 255 rounding explicit, as such:

procedure test;
var
  Values: array [byte] of single;
  ptr: integer;
begin
  for ptr:=0 to 10 do Values[(ptr-5) and high(Values)]:=1;
end;

As you can see, I've made some modifications:

  • Define the for loop index as an integer, to use a CPU register;
  • Use the and operation for fast binary rounding (writing (ptr-5) mod 256 would be much slower);
  • Use high(Values) instead of fixed $ff constant, which indicates where this rounding comes from.

Then the generated code is quick and optimized:

TestAll.dpr.114: begin
0064810C 81C400FCFFFF     add esp,$fffffc00
TestAll.dpr.115: for ptr:=0 to 10 do Values[(ptr-5) and high(Values)]:=1;
00648112 33C0             xor eax,eax
00648114 8BD0             mov edx,eax
00648116 83EA05           sub edx,$05
00648119 81E2FF000000     and edx,$000000ff
0064811F C704940000803F   mov [esp+edx*4],$3f800000
00648126 40               inc eax
00648127 83F80B           cmp eax,$0b
0064812A 75E8             jnz -$18
TestAll.dpr.116: end;
0064812C 81C400040000     add esp,$00000400
00648132 C3               ret 
Arnaud Bouchez
  • 42,305
  • 3
  • 71
  • 159
  • It is a bug, not an unexpected behavior. – kludg Jul 23 '16 at 12:07
  • Until anybody produces documentation to say otherwise I think it is unclear whether or not it is a bug. – David Heffernan Jul 23 '16 at 16:14
  • @DavidHeffernan, you mean that the result of for example `Byte(1000)` could be anything else than the lower byte of 1000? – LU RD Jul 23 '16 at 20:10
  • @LURD I don't see a clear documentation link. The language documentation is incomplete though. – David Heffernan Jul 23 '16 at 20:29
  • 1
    @DavidHeffernan, from [Value Typecasts](http://docwiki.embarcadero.com/RADStudio/en/Expressions_(Delphi)#Value_Typecasts): `The resulting value is obtained by converting the expression in parentheses. This may involve truncation or extension if the size of the specified type differs from that of the expression. The expression's sign is always preserved.` You mean that truncation in my example above could mean something else than the lower byte? – LU RD Jul 23 '16 at 21:01
  • 1
    @LURD I think that is the relevant part of the docs. It is imprecise though. And that's a generous verdict. *The expression's sign is always preserved.* What can that mean when a negative value is converted to unsigned. It probably is a bug but the docs are not massively helpful. For sure it is odd that the behaviour depends on context. Different when used as an index. It is a terrible design flaw of Delphi that context can change the meaning of an expression. – David Heffernan Jul 24 '16 at 07:41
  • I, for myself, am following @DavidHeffernan concern about the confusing docs. The Delphi docs were in fact never the specs... And integer conversion is always a very unsafe part of most languages, not only pascal but also C/C++. Even the definition of "integer" or "int", and the transtyping abilities may vary depending on the CPU it runs on... This was the point of my answer: don't take integer/byte conversion for granted, and make the value cast as explicit as possible. – Arnaud Bouchez Jul 25 '16 at 13:46
  • @ArnaudBouchez - I disagree. C/C++ specs define integer conversions as precise as possible, and Delphi follows C/C++ here. Delphi docs were never good, but that is not the reason not to see the obvious compiler bug. – kludg Jul 25 '16 at 17:16
  • @kludg So how do you expect something like Values[byte(ptr-5)] to be translated and compiled by all C compilers? Are you sure all compiler are consistent? AFAIR Visual C emits a warning and expect an explicit &0xff expression when you assign an int into uint8_t variable... – Arnaud Bouchez Aug 03 '16 at 08:12