0

Here I am trying to solve the infamous eight queens problem. When I finally thought I got it, a segmentation fault hits me out of nowhere. The code:

#include<unistd.h>
#include<stdio.h>
int isLastBoard(int *chessBoard){
  for(int i = 0 ; i < 8 ; ++i){
    if(chessBoard[i] != 7)return 0;
  }
  return 1;
}
int isFit(int *tab){
  for(int i = 1; i < 8 ; ++i){
    for(int j = 0 ; j < i ; ++j){
      if(tab[i] == tab[j] || tab[i] == tab[j] + i -j|| tab[i] == tab[j] - i + j)return 0;
    }
  }
  return 1;
}

int *getNextBoard(int chessBoard[], int n){
  if(n == 0 && chessBoard[n] == 7)return NULL;
  chessBoard[n] = (chessBoard[n] + 1) % 8;
  if(chessBoard[n]==0)return getNextBoard(chessBoard,n-1);
  return chessBoard;
}

int countPossibilities(int *chessBoard,int n){
  for(int i = 0 ; i <8 ; ++i){
    printf("%d",chessBoard[i]);
  }
  printf("     %d\n",n);
  if(isLastBoard(chessBoard))return n;
  if(isFit(chessBoard))return countPossibilities(getNextBoard(chessBoard,7),n+1);
  return countPossibilities(getNextBoard(chessBoard,7), n);
}
int ft_eight_queens_puzzle(void){
  int chessBoard[8] = {0,0,0,0,0,0,0,0};
  return countPossibilities(chessBoard, 0);
}
int main(void){
  printf("%d", ft_eight_queens_puzzle());
  return 0;
}

The program executes and counts up to the board set 00377455, and then gives a segmentation fault. Any help would be appreciated.

I have tried to use a table instead of an int so that i don't surpass the int size limit, and because it's easier. I tried to check if any of my fuctions aren't working, but all seems good.

I use gcc to compile and visual studio code to debug.

~edit:

I would also appreciate any comments or criticisms on my code that would help to improve it.

Mark Adler
  • 101,978
  • 13
  • 118
  • 158
imranbout
  • 1
  • 2
  • In the recursive function `getNextBoard()` should `if(n == 0 && chessBoard[n] == 7)` be `if(n == 0 || chessBoard[n] == 7)` ? The `&&` requirement means it can recurse with `n == -1`. Aside: make your conditional tests as inclusive as possible, so for example `n <= 0` instead of `n == 0`. – Weather Vane Jul 08 '23 at 16:38
  • that if statement is meant for the last case, which is for in to equal the first row and for the value of that row to be 7 which is the last possible case "77777777", also thanks and i will make it n <= 0 instead!. – imranbout Jul 08 '23 at 16:42
  • So it's OK to pass a negative `n`? This is *crucial* to prevent the recursion running away. `n` must never be `< 0`, regardless of anything else. – Weather Vane Jul 08 '23 at 16:43
  • 1
    Asides: 1) please don't make running corrections to your code. Just post your actual code. 2) I thought the classic 8 queens problem uses a 2D chess board. – Weather Vane Jul 08 '23 at 16:46
  • what are running corrections? I copied the whole code and pasted it here, and the classical problem did require a 2D chess board, but i didn't have the time to make it so i thought it would be easier to have an imaginary chess board, the collums are the table's collums and the rows are their values – imranbout Jul 08 '23 at 16:48
  • 3
    You changed the code from `n == 0` to `n <= 0` after my suggestion. Please don't make your code "shifting sand". You might benefit by reading the [Tour](https://stackoverflow.com/tour) and [How do I ask a good question?](https://stackoverflow.com/help/how-to-ask) – Weather Vane Jul 08 '23 at 16:48
  • Are you saying that 1 bit of each array value gives the other dimension? – Weather Vane Jul 08 '23 at 16:53
  • 1
    [What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) – n. m. could be an AI Jul 08 '23 at 16:54
  • yes, instead of having two dimentional array that will be filled with zeros except for the queens' positions, i thought it'd be easier and more efficient to just have a one dimentional array, since two queens can't share the same row anyway, and the value of the column is the position of the queen in that column (the row) – imranbout Jul 08 '23 at 16:56
  • OK I get it thanks. Please revisit my comments about the testing `n` alongside the count. If `n <= 0` the recursion must not proceed whatever other condition there might be. – Weather Vane Jul 08 '23 at 16:59
  • also just wanted to note that the program is not totally not working, i have tried to give it a board set that is very close to a solution and the n printed in the `countPossibilities` function did increase, but is still gave me a segmentation fault after a few rounds, i thought i might have overflowed the stack because of the recursive function but that should have outuputted a stack overflow instead – imranbout Jul 08 '23 at 17:03
  • I never get "stack overflow" error from a runaway recursion. it is always segfault or hang. – Weather Vane Jul 08 '23 at 17:08
  • so do you think the segmentation fault was because of the function `counPossibilities` recursed too much? – imranbout Jul 08 '23 at 17:10
  • I have said it twice already. The test to prevent further recursion is faulty. – Weather Vane Jul 08 '23 at 17:14
  • Have you tried running your code line-by-line in a debugger while monitoring the control flow and the values of all variables, in order to determine in which line your program stops behaving as intended? If you did not try this, then you may want to read this: [What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/q/25385173/12149471) Note that most debuggers allow you to set "conditional breakpoints", so that the execution of the program will be automatically paused when a certain condition is met, for example when you reach "board set 00377455". – Andreas Wenzel Jul 08 '23 at 20:01
  • @WeatherVane There is nothing wrong with the test. It works if it has enough stack space. The code is horrible, but correct. – Mark Adler Jul 08 '23 at 20:03
  • @AndreasWenzel Interestingly, that wouldn't help in this case. The code behaves as intended, right up until it runs out of stack space. More useful would be to run it with an address sanitizer, to immediately detect that that's what's happening. – Mark Adler Jul 08 '23 at 20:04
  • @MarkAdler in function `getNextBoard()` the test `if(n == 0 && chessBoard[n] == 7) return NULL;` will continue when `n < 0` and the next line `chessBoard[n] = (chessBoard[n] + 1) % 8;` will index out of bounds. Or when `n == 0` but `chessBoard[n] != 7` the next recursion will also index out of bounds. – Weather Vane Jul 08 '23 at 20:18
  • @WeatherVane Your first case can't happen. `isLastBoard()` prevents `getNextBoard()` from ever being called with the last board (77777777). So it will never work its way back to incrementing the first row from 7 to 0. `n` can't ever be less than zero. This also prevents your second case for the same reason, in that it can't get back to having wrapped `chessBoard[0]` to zero. – Mark Adler Jul 08 '23 at 20:42
  • @WeatherVane The code in the question, with no changes, and with enough stack space, runs to completion, finds the correct answers, and has no out-of-bounds accesses. – Mark Adler Jul 08 '23 at 20:50

2 Answers2

3

Well, looks like you've come to the right place. This is a stack overflow problem.

Your function countPossibilities() is a tail recursion, and so does not need to call itself at all! It is really just a loop. If you write it that way, then no more stack overflow.

int countPossibilities(int *chessBoard, int n) {
    for (;;) {
        for (int i = 0; i < 8; ++i)
            printf("%d", chessBoard[i]);
        printf("     %d\n", n);
        if (isLastBoard(chessBoard))
            return n;
        if (isFit(chessBoard))
            n++;
        getNextBoard(chessBoard, 7);
    }
}

You were trying to have that function call itself 16,777,216 times (pointlessly), which will blow even the maximum stack that my linker will let me allocate, which is 512MB.

Your getNextBoard() is also a tail recursion, and can also be written as a loop. No recursion is needed for your brute-force solution. You are simply stepping through all possible boards with one queen per row.

As for suggestions, a little pruning goes a long way. Do your isFit() check for the preceding rows as you add each piece to the board. Don't proceed with that piece in that position if it doesn't fit. Then you will find that you only need to look at less than 2000 intermediate boards, as opposed to 16,777,216 completed boards. This is when you will find recursion useful.

Mark Adler
  • 101,978
  • 13
  • 118
  • 158
0

for anyone from the future who somehow ends up having to deal with the same problem here's a solution:

#include<unistd.h>
#include<stdio.h>
int isLastBoard(int *chessBoard){
  for(int i = 0 ; i < 8 ; ++i){
    if(chessBoard[i] != 7)return 0;
  }
  return 1;
}
int isFit(int *tab){
  for(int i = 1; i < 8 ; ++i){
    for(int j = 0 ; j < i ; ++j){
      if(tab[i] == tab[j] || tab[i] == tab[j] + i -j|| tab[i] == tab[j] - i + j)return 0;
    }
  }
  for (int i = 0; i < 8; i++)
  {
    printf("%d",tab[i]+1);
  }
  printf("\n");
  
  return 1;
}

int *getNextBoard(int chessBoard[], int n){
  if(n <= 0 && chessBoard[n] == 7)return chessBoard;
  chessBoard[n] = (chessBoard[n] + 1) % 8;
  if(chessBoard[n]==0)return getNextBoard(chessBoard,n-1);
  return chessBoard;
}

int countPossibilitiesIn100(int *chessBoard,int n, int stop){
  if(stop >= 100)return n;
  if(isLastBoard(chessBoard))return n;
  if(isFit(chessBoard))return countPossibilitiesIn100(getNextBoard(chessBoard,7),n+1,stop+1);
  return countPossibilitiesIn100(getNextBoard(chessBoard,7), n, stop + 1);
}
int ft_eight_queens_puzzle(void){
  int chessBoard[8] = {0,0,0,0,0,0,0,0};
  int n = 0;
  while (!isLastBoard(chessBoard))
  {
    n = countPossibilitiesIn100(chessBoard,n,0);
  }
  return n;
}
int main(void){
  printf("%d", ft_eight_queens_puzzle());
  return 0;
}

all i had to do is make the function countPossibilities count only 100 possibilities at a time, so that i don't overflow the stack with recursive returns, and then make it run until it reaches the final case which is 77777777

imranbout
  • 1
  • 2
  • 1
    Then what if you make it `stop >= 1` instead, recursing only once? Then you have effectively reduced your recursions to simply your `while` loop. So you didn't need the recursion there in the first place! See my answer. – Mark Adler Jul 08 '23 at 17:57