C: Weird characters on output

notsoevil · 06-23-2003, 08:55 AM

As I might have explained before, I am trying to write C while learning, so I decided to take a subroutine I've built a long time ago in Perl, and translate it to C (though, making it more than just a function, so I can test via the CLI).

Here is my code so far:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

void rollDice(char *pool);
void joinRolls(int *toJoin, char *joinedRolls, int numOfRolls);

int main() {
  char test[50];
  char quit[3] = "q\n";
  char blank[2] = "\n";
  
  int iQuit = 0;
  while (!iQuit) {
    printf("Enter a dice pool (or q for quit):  ");
    fgets(test,50,stdin);
    if (strcmp(test,quit) == 0) {
      iQuit = 1;
    }
    else if (strcmp(test,blank) == 0) {
      continue;
    }      
    else {
      rollDice(test);
    }
  }
    
  return 0;
}

void rollDice(char *pool) {
  char *dieNumber, *dieSides;
  char pool_copy[50];
  strcpy(pool_copy,pool);
  dieNumber = strtok(pool_copy,"d");
  dieSides = strtok(NULL,"d");
  
  int dieNumberAsInt = atoi(dieNumber);
  int dieSidesAsInt  = atoi(dieSides);
  
  int rolls[dieNumberAsInt];
  int i;
  for (i = 1; i <= dieNumberAsInt; i++) {
    srand(time(NULL)+rand());
    int roll = (rand() % dieSidesAsInt) + 1;
    rolls[i-1] = roll;
  }

  int j;
  for (j = 0; j < dieNumberAsInt; j++) {
    printf("roll %d is %d\n", j+1, rolls[j]);
  }
  char joinedRolls[1000];
  joinRolls(rolls,joinedRolls,dieNumberAsInt);
  printf("joinedRolls is %s\n",joinedRolls);
  
  //sprintf(pool, "dieNumber is %s\ndieSides is %s\n", dieNumber, dieSides);
}

void joinRolls (int *toJoin, char *joinedRolls, int numOfRolls) {
  int i;
  for (i=0; i < numOfRolls; i++) {
    if (strlen(joinedRolls) == 0) {
      char thisRoll[5];
      sprintf(thisRoll, "%d", toJoin[i]);
      strcat(joinedRolls,thisRoll);
    }
    else {
      char thisRoll[5];
      sprintf(thisRoll, "%d", toJoin[i]);
      strcat(joinedRolls,",");
      strcat(joinedRolls,thisRoll);
    }
  }
}

Things seem to be going okay, but then I noticed if I submitted dice pools three times, part of the cumulative joinedRolls gets odd characters in it, as the output below illustrates:

Code:

[anthony@ogre src]$ ./crpbs
Enter a dice pool (or q for quit):  5d10
roll 1 is 10
roll 2 is 3
roll 3 is 3
roll 4 is 9
roll 5 is 3
joinedRolls is 10,3,3,9,3
Enter a dice pool (or q for quit):  5d10
roll 1 is 3
roll 2 is 1
roll 3 is 1
roll 4 is 8
roll 5 is 3
joinedRolls is 10,3,3,9,3,3,1,1,8,3
Enter a dice pool (or q for quit):  5d10
roll 1 is 1
roll 2 is 3
roll 3 is 10
roll 4 is 5
roll 5 is 1
joinedRolls is 10,3,3,9,3,3,1,1àB þBìZ@,1,3,10,5,1
Enter a dice pool (or q for quit):  
[anthony@ogre src]$

I can't figure out what's wrong, but I'd take a guess it has to do something with me screwing up pointers somewhere, because I know they can point to garbage if not used properly.

Pointers are the one thing I still really don't have a grasp on when it comes to C. I'm reading the K&R book, but its all a bit esoteric. Any pointers (no pun intended) to good online tutorials would be appreciated.

Palin · 06-23-2003, 02:48 PM

If this is supposed to keep the previous rolls when it rolls again then it seems to be working correctly to me. I don't get the garbage.

tobythelard · 06-23-2003, 03:42 PM

Hi notsoevil,

I just had a quick look through your code, and without compiling it and running it I can't be sure this is your only problem....

The problem is one of memory usage, and how long that memory is available to your program or a function of your program. If you write a function like this:

void myfunction( boolean setvalue )
{
char mybuffer[ 50 ];

if( setvalue )
{
memset( mybuffer, 'A', 50 );
mybuffer[49] = '\0';
}

printf("my buffer is %s", mybuffer );
}

Call it once with a true, then repeatedly with false value and watch the output. You'll see that if true you should get a nice string of 'A's, but gradually things go pear shaped afterwards - how long this takes is down to chance, and how your compiler organises its stacks etc. but that's for further study....

Then when myfunction ends the value put into mybuffer is lost. If you call myfunction a second time the OS creates a new mybuffer that might be at the same location as the previous call, it might not, and if it is at the previous location it might or might not have been completely or partially overwritten during the time between calls. I think K&R covers this stuff somewhere.....

In case you're not with me any more, the villian in your code is the joinedRolls buffer and how it is handled. Each time you call rollDice you are assuming that joinedRolls has retained its previous value, which may or may not (probably not) be the case. When you enter a function you have to assume that all variables defined in that function contain a nonsense value until set. Sadly this often isn't the case for repeated calls to a function, you may have what looks like good data in a variable that is in fact invalid or old - this makes debugging a nightmare. The strlen at the top of joinRolls is also buggy for the same reason. A good syntax checker like Lint will probably pick this type of bug up, the compiler won't as it only performs basic type checking.

There are 3 solutions, all of which guarantee your memory remains yours between calls to JoinRolls:
1) The simplest, move the definition of joinedRolls out of the function rollDice to just below your function prototypes. This makes the buffer global and as such available for the complete runtime of the program. This is fine for a simple program like this, and here is the solution I would suggest, but in general global data should be avoided wherever possible.

2) Make the buffer inside joinRolls 'static', IE:
static char joinedRolls[1000];
This will also preserve the contents between calls, but also has the disadvantage that your function now has 'state' information contained in it. This makes debugging more difficult. It would work here but again use with caution.

3) The best solution of all, although it means changing your interfaces (again to be avoided in larger projects) is to define the buffer in the calling function - here you would have to do it in main( ) and pass it down through the others to ensure it stays valid. The next extension is to malloc the memory you need....

A last tip to avoid this in future: Whenever you define a variable in a function give it a value. IE:

int a = 0xdeadbeef; /* assuming 32bit ints.... */
char b = 'Z';
int buffer[1000]; memset( buffer, 0xdeadbeef, 1000);

Then you'll spot this sort of bug immediately, which incidentally the professionals still screw up from time to time.....

I hope this helps. Good luck,
Toby

notsoevil · 06-23-2003, 04:00 PM

tobythelard,

The code works just fine now after makign joinedRolls static, and I fully understand what you mean. I thought it had to do something with my misuse of pointers, as a problem I had just getting to this stage in the code was exactly the same -- the pointer was no longer in scope.

The reason I couldn't figure that out, was because it was saving -some- of the data from the previous rolls. I assumed it either did or didn't retain information from instance to instance, but if memory can be allocated "sorta" in the same spot, as seems to be happening, I believe I am going to have many future headaches with C. Though I suppose everyone does or has.

Again, thanks for the suggestions and the solution. I am going to change it now so that I am passing the variable down through the functions as this seems more appropriate to me -- and definitely better than making it global.