Re: Virtual Machine implementation problem, Please help me to spot the bug

On 7 May 2007 23:18:00 -0700, "weidongtom@xxxxxxxxx"
<weidongtom@xxxxxxxxx> wrote:
<snip>
The code was able to be compiled by gcc 3.4.2 <mingw-speical>. As I am
developing it on Windows (and I am a novice) I don't know if there's
any replacement on my platform for the library function ntohl()
defined in <netinet/arpa.h> found in Linux/Unix, so that I can use it

#if OFFTOPIC /* since this group is about portable C which doesn't
include sockets */ /* oops, preproc directives can't wrap lines <G> */

mingw uses the Windows libraries, and the Windows equivalent to the
BSD/Unix sockets library does contain the {ntoh,hton}[sl] functions
among many other things all declared in <winsock2.h> .

#else
OTOH doing it yourself, preferably in portable code, is also fine.
#endif

to changed the UM binary file data (which is big-endian) to my local-
intel's small-endian 32 bit data, so I came up with:
void change_endian(u32* value);
to handle the problem which seems to have worked.

So, there's no compilation problem, but there's a runtime error:
"Array index out of bound."
codex=c0000032, opcode=12, finger=0000000e
OP_LOAD, loading program into array0 from array[6] at 00000000
finger=0400005b
registers: 00000000d, 02000014, 0400005b, 06000035, 00000000, 0000000,
00000000, 00000000


To be clear, this appears to be two trace messages and an error
message generated by your program. ('Runtime error' is sometimes used
specifically to mean errors detected and/or reported by the
compiler-generated or library support code, not explicitly by user
code, although this is less often true among C programmers.)

when run with this UM binary file: http://www.boundvariable.org/sandmark.umz
. There's nothing wrong with the binary file, so I suspect there's
something wrong with the way I handle the array memory allocation, or
is there something wrong with how I change the endians (which seems to
be unlikely).

See below.

<snip>
typedef unsigned int u32;
typedef unsigned char u8;

Aside: int (signed or unsigned) isn't required to be 32 bits, and
there are systems, though fewer nowadays, where it is (they are) 16
bits, which would not work at all for your application. long (signed
and unsigned) is at least 32 bits, but sometimes more (which would
work, but be wasteful). Similarly char isn't required to be exactly 8
bits, but is at least 8, which is probably good enough for your usage.

<snip>
#define OP_CMOV 0
#define OP_INDEX 1
#define OP_AMAND 2
#define OP_ADD 3
#define OP_MUL 4
#define OP_DIV 5
#define OP_NAND 6
#define OP_HALT 7
#define OP_ALLOC 8
#define OP_FREE 9
#define OP_OUT 10
#define OP_IN 11
#define OP_LOAD 12
#define OP_ORTHO 13

Aside: while #define can give a name to any kind of literal (supported
by C) and even other things, where you have a series of names for
not-huge integers, especially consecutive integers as here, you can
instead use enum, and I would.

<snip>
/*Change local small-endian to UM's internal big-endian
* this is a dirty hack to replace the netinet/arpa.h's ntohl()
*/
void change_endian(u32* value){
u8 tmp1[4], tmp2[4];
memcpy((u32 *)tmp1, value, sizeof(u32));

You don't need to cast; if properly declared (by <string.h>, which you
do have) memcpy takes void*, and any pointer to data (not qualified as
const or volatile) will be automatically converted.

You would need to cast if you tried to do something like:
* (u32*) tmp1 = * value;
but that is a bad idea anyway, since C allows types other than
character to require alignment, and on some machines they do, and
there is no guarantee that array-4-of-u8 is correctly aligned for u32.
tmp2[0] = tmp1[3];
tmp2[1] = tmp1[2];
tmp2[2] = tmp1[1];
tmp2[3] = tmp1[0];
memcpy(value, (u32 *)tmp2, sizeof(u32));

Ditto.

}

Aside: It isn't actually necessary to copy in, swap, and then copy
out. However, as you apparently use this only when loading the initial
'program', probably a relatively modest amount of data, it probably
doesn't matter. Certainly it is more important to get the program
correct first, and only then IF performance is too low AND measurement
e.g. profiling confirms this is a (or the) problem THEN worry about
micro-optimization.

<snip>
/* Duplicate the content in from to that of to.
* original data in to is discarded.
*/
void copy_array(u32 *to, u32 *from){
int size = get_array_size(from);
if(!to)
delete_array(to);
to = create_array(size);
memcpy(to, from, size*sizeof(u32));
}

This is most likely your actual problem, and is a FAQ: 4.8, at the
usual places and http://c-faq.com/ .

You allocate a new array and modify 'to' to point to it, but only
locally within copy_array. Any variable whose value is passed by the
caller, which in this case is um->program, is NOT modified, and still
points to the old memory which is now invalid; formally this causes
Undefined Behavior (anything may happen) and practically even if this
doesn't do something really crazy it won't do what you wanted.

void load_program(u32** membuffer, const char* filename){
<snip>
membuffer_size = sizeof(u8)*file_size/sizeof(u32) + 1;
if(!(*membuffer))
free((*membuffer));
(*membuffer) = create_array(membuffer_size);

Note that in this, similar, case, you got it right: you pass a pointer
to the pointer, and both use and modify indirectly. Another approach
is to instead pass back the new/modified pointer as the return value
and require (and allow) the caller to store it where desired.

size_read = fread((*membuffer), sizeof(u8), file_size, infile);
if(size_read != file_size && !feof(infile)){
fprintf(stderr, "Warning: failed to read all data.\n");
}

Small point: why check feof? Even if set it indicates that you somehow
didn't get the right size from <offtopic and snipped> stat() </> which
is still probably worth at least a warning.

<snip much that looks OK although I would likely choose to do it
somewhat differently>
int main(int argc, char *argv[]){
UM32 um;
u32 *program;
int run;

#ifndef NOT_GDB
argc = 2;
argv[1] = "sandmark.umz";
#endif

This is a little dubious. There is not actually an official guarantee
that you can modify argv[] pointers, although in practice you can.
(You _are_ allowed to modify the string contents pointed to by argv[]
pointers, up to their initial length but not longer.) Officially it is
not even guaranteed/required that there be a commandname argv[0] and
thus you can have argc == 0 and argv[0] == NULL and argv[1] doesn't
even exist, though on 'normal' implementations this won't happen.

if(argc < 2){
printf("Usage: %s filename\n", argv[0]);
exit(0);
}

Moreover, it isn't necessary. Usually better practice would be
something like:

const char * program_name;
#if TESTCASE
program_name = "sandmark.umz";
#else
if( argc < 2 ) { error as above }
/* else */ program_name = argv[1];
#endif
... now proceed to use program_name ...

Note that I find the double negative (if not not-GDB) confusing, and
also prefer to have the no-macro case be the (presumed) normal case,
and the debugging case should not normally be the normal case.

<offtopic> Also, if by GDB you really do mean GDB the GNU debugger,
you _can_ give a program arguments when you run it within GDB, so you
don't need this kind of kludge in the program itself. </>

- formerly david.thompson1 || achar(64) || worldnet.att.net
.

Relevant Pages

  • Re: Im trying to install Net::SSH::Perl on a Windows Box.
    ... Subject: I'm trying to install 'Net::SSH::Perl' on a Windows Box. ... around C libraries. ...
    (perl.beginners)
  • Re: Aqua is a Plastic Cube!
    ... if WPF were put into Apple's old Mac OS X ... the problem this platform has is that it's rather vaguely defined, ... better than it is on X Windows or Windows XP. ...
    (comp.sys.mac.advocacy)
  • Re: Great SWT Program
    ... using computers was with Windows. ... Moving the cursor can be done in a number of ways (arrow keys plus ... But vim does. ...
    (comp.lang.java.programmer)
  • Re: Drag it to the trash...
    ... Seems to have worked for everyone that uses VMS anyways. ... He finds *lots* of bugs in Windows. ... is NTFS, but you might chose to use VFAT (say, for Mac OS X ...
    (comp.sys.mac.advocacy)
  • Re: Drag it to the trash...
    ... VMS was a minicomputer OS, and was aimed at much more ... DOS batch files are at least much simpler than DCL. ... He finds *lots* of bugs in Windows. ...
    (comp.sys.mac.advocacy)