Hello,
While working on packed array support for FPC and verifying against GPC, I discovered a bug in GPC's packed array support. The following program prints 0 as second line instead of 1540222835
*** type ta = packed 0..$7fffffff; tb = packed array[0..40] of ta;
function test(i: longint): integer; var b: tb; begin b[1] := $5BCDEF73; test := b[i]; end;
begin writeln(sizeof(tb)); writeln(test(1)); end. ***
Jonas
Jonas Maebe wrote:
Hello,
While working on packed array support for FPC and verifying against GPC, I discovered a bug in GPC's packed array support. The following program prints 0 as second line instead of 1540222835
type ta = packed 0..$7fffffff; tb = packed array[0..40] of ta;
function test(i: longint): integer; var b: tb; begin b[1] := $5BCDEF73; test := b[i]; end;
begin writeln(sizeof(tb)); writeln(test(1)); end.
Thanks for the testprogram. On Mac OS X, I get
[P17:~/gpc/testgpc/adriaan] adriaan% gp packing.p PC=gpc-ppc [P17:~/gpc/testgpc/adriaan] adriaan% ./packing 160 0
[P17:~/gpc/testgpc/adriaan] adriaan% gp packing.p PC=gpc-i386 [P17:~/gpc/testgpc/adriaan] adriaan% ./packing 160 126835
[P17:~/gpc/testgpc/adriaan] adriaan% gpc -v Reading specs from /Developer/Pascal/gpc345u2/lib/gcc/i386-apple-darwin8/3.4.5/specs Configured with: ../gcc-3.4.5/configure --enable-languages=pascal,c --enable-threads=posix --target=i386-apple-darwin8 --host=i386-apple-darwin8 --build=i386-apple-darwin8 --prefix=/Developer/Pascal/gpc345u2 --with-arch=pentium-m --with-tune=prescott Thread model: posix gpc version 20051116, based on gcc-3.4.5
Regards,
Adriaan van Os
On Sat, 5 Aug 2006, Jonas Maebe wrote:
Hello,
While working on packed array support for FPC and verifying against GPC, I discovered a bug in GPC's packed array support. The following program prints 0 as second line instead of 1540222835
type ta = packed 0..$7fffffff; tb = packed array[0..40] of ta;
function test(i: longint): integer; var b: tb; begin b[1] := $5BCDEF73; test := b[i]; end;
begin writeln(sizeof(tb)); writeln(test(1)); end.
I got: 160 126835
using: Reading specs from /usr/local/lib/gcc/i686-pc-linux-gnu/3.4.6/specs Configured with: ../gcc-3.4.6/configure --enable-languages=c,c++,pascal --enable-shared --enable-threads-posix --enable-__cxa_atexit Thread model: posix gpc version 20060325, based on gcc-3.4.6
russ
On 05 Aug 2006, at 04:17, Russell Whitaker wrote:
I got: 160 126835
using: Reading specs from /usr/local/lib/gcc/i686-pc-linux-gnu/3.4.6/specs Configured with: ../gcc-3.4.6/configure --enable-languages=c,c+ +,pascal --enable-shared --enable-threads-posix --enable-__cxa_atexit Thread model: posix gpc version 20060325, based on gcc-3.4.6
Sorry I forgot to mention my compiler version and platform:
Reading specs from /Developer/Pascal/gpc345u2/lib/gcc/powerpc-apple- darwin8/3.4.5/specs Configured with: ../gcc-3.4.5/configure --enable-languages=pascal,c -- enable-threads=posix --target=powerpc-apple-darwin8 --host=powerpc- apple-darwin8 --build=powerpc-apple-darwin8 --prefix=/Developer/ Pascal/gpc345u2 Thread model: posix gpc version 20051116, based on gcc-3.4.5
Although the output you get is also wrong (the 160 is correct, but the 126835 isn't).
Another one I found: the program below produces an internal compiler error (at least on 32 bit systems, but I guess you can create a similar one on 64 bit systems, possibly by increasing the size of the array to high(longint)+1)
*** type ta = packed 0..63; tb = packed array[0..3000000000] of ta;
var b: tb; begin writeln(b[3000000000]) end. ***
Gives:
$ gp packarr2 gp: warning: /Users/jonas/fpc/test/packarr2.pp: missing program header /Users/jonas/fpc/test/packarr2.pp:1: warning: missing program header /Users/jonas/fpc/test/packarr2.pp: In main program: /Users/jonas/fpc/test/packarr2.pp:10: error: could not split insn (insn 26 22 27 (set (reg:SI 0 r0 [129]) (const_int 2249981952 [0x861c0000])) 307 {*movsi_internal1} (nil) (nil)) /Users/jonas/fpc/test/packarr2.pp:10: internal compiler error: in final_scan_insn, at final.c:2429 Please submit a full bug report, with preprocessed source if appropriate. See URL:http://www.gnu-pascal.de/todo.html for instructions.
Jonas
Jonas Maebe wrote:
Another one I found: the program below produces an internal compiler error (at least on 32 bit systems, but I guess you can create a similar one on 64 bit systems, possibly by increasing the size of the array to high(longint)+1)
type ta = packed 0..63; tb = packed array[0..3000000000] of ta;
var b: tb; begin writeln(b[3000000000]) end.
Gives:
$ gp packarr2 gp: warning: /Users/jonas/fpc/test/packarr2.pp: missing program header /Users/jonas/fpc/test/packarr2.pp:1: warning: missing program header /Users/jonas/fpc/test/packarr2.pp: In main program: /Users/jonas/fpc/test/packarr2.pp:10: error: could not split insn (insn 26 22 27 (set (reg:SI 0 r0 [129]) (const_int 2249981952 [0x861c0000])) 307 {*movsi_internal1} (nil) (nil)) /Users/jonas/fpc/test/packarr2.pp:10: internal compiler error: in final_scan_insn, at final.c:2429
I can reproduce this one also
[P17:~/gpc/testgpc/adriaan] adriaan% gp packing2.p PC=gpc-ppc /Users/adriaan/gnu/gpc/testgpc/adriaan/packing2.p: In main program: /Users/adriaan/gnu/gpc/testgpc/adriaan/packing2.p:10: error: could not split insn (insn 26 22 27 (set (reg:SI 0 r0 [129]) (const_int 2249981952 [0x861c0000])) 307 {*movsi_internal1} (nil) (nil)) /Users/adriaan/gnu/gpc/testgpc/adriaan/packing2.p:10: internal compiler error: in final_scan_insn, at final.c:2429 Please submit a full bug report, with preprocessed source if appropriate. See URL:http://www.gnu-pascal.de/todo.html for instructions.
[P17:~/gpc/testgpc/adriaan] adriaan% gp packing2.p PC=gpc-i386 /Users/adriaan/gnu/gpc/testgpc/adriaan/packing2.p: In main program: /Users/adriaan/gnu/gpc/testgpc/adriaan/packing2.p:9: internal compiler error: in tree_low_cst, at tree.c:3315 Please submit a full bug report, with preprocessed source if appropriate. See URL:http://www.gnu-pascal.de/todo.html for instructions.
[P17:~/gpc/testgpc/adriaan] adriaan% gpc -v Reading specs from /Developer/Pascal/gpc345u2/lib/gcc/i386-apple-darwin8/3.4.5/specs Configured with: ../gcc-3.4.5/configure --enable-languages=pascal,c --enable-threads=posix --target=i386-apple-darwin8 --host=i386-apple-darwin8 --build=i386-apple-darwin8 --prefix=/Developer/Pascal/gpc345u2 --with-arch=pentium-m --with-tune=prescott Thread model: posix gpc version 20051116, based on gcc-3.4.5
Regards,
Adriaan van Os
On Fri, 4 Aug 2006, Russell Whitaker wrote:
On Sat, 5 Aug 2006, Jonas Maebe wrote:
Hello,
While working on packed array support for FPC and verifying against GPC, I discovered a bug in GPC's packed array support. The following program prints 0 as second line instead of 1540222835
type ta = packed 0..$7fffffff; tb = packed array[0..40] of ta;
function test(i: longint): integer; var b: tb; begin b[1] := $5BCDEF73; test := b[i]; end;
begin writeln(sizeof(tb)); writeln(test(1)); end.
I got: 160 126835
using: Reading specs from /usr/local/lib/gcc/i686-pc-linux-gnu/3.4.6/specs Configured with: ../gcc-3.4.6/configure --enable-languages=c,c++,pascal --enable-shared --enable-threads-posix --enable-__cxa_atexit Thread model: posix gpc version 20060325, based on gcc-3.4.6
russ
adding "writeln( b[1]," ",b[i] ):" to the function (just before "test := ...") and I got: 160 1540222835 126835 126835
hope this helps russ
Jonas Maebe wrote:
Hello,
While working on packed array support for FPC and verifying against GPC, I discovered a bug in GPC's packed array support. The following program prints 0 as second line instead of 1540222835
type ta = packed 0..$7fffffff; tb = packed array[0..40] of ta;
function test(i: longint): integer; var b: tb; begin b[1] := $5BCDEF73; test := b[i]; end;
begin writeln(sizeof(tb)); writeln(test(1)); end.
AFAICS it is an old known bug: packed arrays with elements bigger then 17 bits do not work on 32-bit machines (on 64-bit machines the biggest working size is 33-bits).
Waldek Hebisch wrote:
Jonas Maebe wrote:
Hello,
While working on packed array support for FPC and verifying against GPC, I discovered a bug in GPC's packed array support. The following program prints 0 as second line instead of 1540222835
type ta = packed 0..$7fffffff; tb = packed array[0..40] of ta;
function test(i: longint): integer; var b: tb; begin b[1] := $5BCDEF73; test := b[i]; end;
begin writeln(sizeof(tb)); writeln(test(1)); end.
AFAICS it is an old known bug: packed arrays with elements bigger then 17 bits do not work on 32-bit machines (on 64-bit machines the biggest working size is 33-bits).
Actually I think it's 18 and 34 bits respectively (as they'll always have 2-bit alignment, so will fit in 2*16/2*32 bits), likewise 20 and 24 or 36, 40 and 48 respectively will work, but let's not split hairs. ;-)
You once suggested to rewrite packed array support significantly, also for other reasons. What are your current plans? If you're not planning to do it now, perhaps we should implement a temporary solution, even if less efficient, to fix the bug.
Frank
On 25 aug 2006, at 01:09, Frank Heckenbach wrote:
You once suggested to rewrite packed array support significantly, also for other reasons. What are your current plans? If you're not planning to do it now, perhaps we should implement a temporary solution, even if less efficient, to fix the bug.
I have in the mean time implemented packed array support in FPC. I noticed GPC uses double precision shifts even in some cases where it is not needed at all, see e.g. http://www.freepascal.org/cgi-bin/viewcvs.cgi/trunk/tests/test/ tparray7.pp?rev=4444&view=markup
In the nested loop at the start of the test8bit procedure, GPC uses double precision shifts (64 bit shifts on 32 bit) and a lot of conditional code (on PowerPC at least, with -O2), although plain 8 bit loads/stores are enough.
If you want to have a look at how FPC does things, the code is in http://www.freepascal.org/cgi-bin/viewcvs.cgi/trunk/compiler/ cgobj.pas?rev=4491&view=markup
Search for a_load_subsetref_reg (load from an x-bitsized memory location into a register) and a_load_reg_subsetref (store from a register into an x-bitsized memory location). We use the same memory layout in both big and little endian as GPC.
The code there is 100% cpu-independent and should be fairly straightforward to understand. That said, I'm under the impression that GPC's bitpacked array support is implemented at a higher level (depending on lower level GCC support routines which may be similar to what in FPC above), so I don't know how useful this is for you.
The resulting code is fairly good in general (even when not optimized) and only uses basic operators (and, or, shl, shr, not).
Jonas
Frank Heckenbach wrote:
You once suggested to rewrite packed array support significantly, also for other reasons. What are your current plans? If you're not planning to do it now, perhaps we should implement a temporary solution, even if less efficient, to fix the bug.
I actually plan to do limited rewrite (use byte access in inline code and pass all sizes bigger then 18 bits to the runtime support). I would like to this simultaneusly with changes to set handling (to minimize number of releases in which we break binary compatiblity).
However ATM there are things that I began (argument passing cleanup (rewrite), 4.1 port, exception support and more) and I prefer to finish them before starting something new.
I thought about doing a simpler fix, but I am affraid that such a fix requires work comparable with (limited) rewrite.
Waldek Hebisch wrote:
Frank Heckenbach wrote:
You once suggested to rewrite packed array support significantly, also for other reasons. What are your current plans? If you're not planning to do it now, perhaps we should implement a temporary solution, even if less efficient, to fix the bug.
I actually plan to do limited rewrite (use byte access in inline code and pass all sizes bigger then 18 bits to the runtime support). I would like to this simultaneusly with changes to set handling (to minimize number of releases in which we break binary compatiblity).
However ATM there are things that I began (argument passing cleanup (rewrite), 4.1 port, exception support and more) and I prefer to finish them before starting something new.
I thought about doing a simpler fix, but I am affraid that such a fix requires work comparable with (limited) rewrite.
Couldn't we just, instead of the current two half-word accesses, do two full-word or two double-word accesses (more precisely, two accesses of either the largest integer type available or the next smaller one) when the field size requires it? I must admit I haven't looked closer at this code for quite a while, so I may be missing something important. And again, that's for a quick fix only -- of course, the code will be quite inefficient in some cases, but still better than buggy. But if it works, it should require small changes only.
Frank