2

This had a previous question regarding multi thread issues Here. Now the issue is that the program exits without any input. The program gets the input from a text file given as arguments with executing. It should only contain numbers separated by spaces and if theres any other character it should give an error as done in row_check functions. Can anyone suggest why it would exit without any error ?.

#include<pthread.h>
#include<stdio.h>
#include<stdlib.h>
#include<unistd.h>
#include<ncurses.h>
const unsigned int NUM_OF_THREADS = 9;
typedef struct thread_data_s {
char *ptr;
int row_num;
} thread_data_t;

void report(const char *s,int w,int q);

void* row_check(void* data)
{
    thread_data_t *my_data_ptr = data;
    int j, flag;

    flag=0x0000;

    for(j = 0; j < 9; j++)
{
    flag |= 1u << ( (my_data_ptr->ptr)[j] - 1 );

    if (flag != 0x01FF){
        report("row", my_data_ptr->row_num, j-1);
    }
}   

return NULL;
}

void report(const char *s,int w,int q)
{
   printf("\nThe sudoku is INCORRECT");
   printf("\nin %s. Row:%d,Column:%d",s,w+1,q+1);
   getchar();

   exit(0);
 }


 int main(int argc, char* argv[])
 {
     int i,j;
     char arr1[9][9];
     FILE *file = fopen(argv[1], "r");
     if (file == 0)
 {
    fprintf(stderr, "failed");
    exit(1);
 }
    int col=0,row=0;
    int num;

    while(fscanf(file, "%c ", &num) ==1) {
        arr1[row][col] = num;
        col++;

    if(col ==9)
    {
        row++;
        col = 0;
    }
 }

 fclose(file);

int n;

thread_data_t data[NUM_OF_THREADS];
pthread_t tid;
pthread_attr_t attr;


for(n=0; n < NUM_OF_THREADS; n++)
{
     data[n].ptr = &arr1[n][0];
     data[n].row_num = n;
     pthread_create(&tid, &attr, row_check, &data[n]);
}


for(n=0; n < NUM_OF_THREADS; n++)
{
     pthread_join(tid, NULL);
}


return 0;

}
Community
  • 1
  • 1
Joel Shevin
  • 103
  • 1
  • 2
  • 9
  • update : file reading works, threads are created as well. issue seems to be when row check is called ? – Joel Shevin Apr 17 '17 at 00:31
  • Suggest `fscanf(file, " %c", &num) ==1` (move space) – chux - Reinstate Monica Apr 17 '17 at 01:24
  • Have you tried debugging it? Depends on which os and compiler you are using on which debugger to use – lxx Apr 17 '17 at 08:43
  • for ease of readability and understanding: 1) consistently indent the code. indent after every opening brace '{'. unindent before every closing brace '}'. suggest using 4 spaces for each indent level. 2) follow the axiom: *only one statement per line and (at most) one variable declaration per statement.* – user3629249 Apr 17 '17 at 16:02
  • it is a poor programming practice to name various elements of the program the same, with only capitalization differences. I.E. `FILE` and `file` – user3629249 Apr 17 '17 at 16:03
  • When a system function (for instance `fopen()` returns an error indication, use the function:: `perror()` so both the enclosed text AND the reason the system thinks the error occurred are output to `stderr`. – user3629249 Apr 17 '17 at 16:06
  • the constant: `NUM_OF_THREADS` is declared as an `unsigned int` but it is being compared to the variable `n` which has type `int`. The variable `flag`, which has type `int` is being assigned from a type `unsigned int` Such mis-match between variable types will 'usually' work, but not always – user3629249 Apr 17 '17 at 16:14
  • regarding: `while(fscanf(file, "%c ", &num) ==1)` the '%c' is expecting a pointer to char parameter, but the parameter is pointer to int. There are several similar problems with the posted code. Always enable all the warnings when compiling, then fix those warnings. (for `gcc`, at a minimum use: -Wall -Wextra -pedantic-errors` I also use: `-Wconversion -std=gnu11` ) – user3629249 Apr 17 '17 at 16:17
  • never access beyond `argv[0]` without first checking `argc` to assure the command line parameter actually exists/ When the necessary command line parameter does not exist, then display on `stderr`, a USAGE statement showing how the program should be invoked. – user3629249 Apr 17 '17 at 16:25
  • the code block, beginning with: `while(fscanf(file, "%c ", &num) ==1)` has a logic error. This is because it has no limit on the number of rows. Therefore can overrun the `arrl[][]` array, Such overrun results in undefined behavior and can lead to a seg fault event. Suggest: `while( row < 9 && fscanf(file, "%c ", &num) ==1)` – user3629249 Apr 17 '17 at 16:30
  • the last two code blocks in `main()` both starting with: `for(n=0; n < NUM_OF_THREADS; n++)` contains a data error. all the thread `tid`s are being placed in the same variable `tid`, then the statement: `pthread_join(tid, NULL);` is only looking at a single thread tid. so the second time through the loop, the call to `pthread_join()` will never return because the exit of the specific `tid` thread has already exited and been handled. – user3629249 Apr 17 '17 at 16:39
  • the exit from the top thread function should be `pthread_exit()` rather than `return null;` although that may work, it will create a problem if any of the available macros are applied to the exit status after the call to `pthread_join()`. – user3629249 Apr 17 '17 at 16:42
  • note: in sudoku, also need to check the columns and each of the 9 3x3 blocks to assure the validity of the puzzle result – user3629249 Apr 17 '17 at 16:44

2 Answers2

1

The following in one of the issues in the code and it would explain why the application exists so soon...

The following code doesn't join all the threads it creates (so the application exits and terminates the threads before they finished running):

thread_data_t data[NUM_OF_THREADS];
pthread_t tid;
pthread_attr_t attr;

for(n=0; n < NUM_OF_THREADS; n++)
{
     data[n].ptr = &arr1[n][0];
     data[n].row_num = n;
     pthread_create(&tid, &attr, row_check, &data[n]);
}


for(n=0; n < NUM_OF_THREADS; n++)
{
     pthread_join(tid, NULL);
}

As you can see, the code is only saving the pointer to one of the threads (the value in tid is always replaced, overwriting the existing data) and joining that thread (instead of all of them).

This might be better constructed as:

thread_data_t data[NUM_OF_THREADS];
pthread_t tid[NUM_OF_THREADS];

for(n=0; n < NUM_OF_THREADS; n++)
{
     data[n].ptr = &arr1[n][0];
     data[n].row_num = n;
     pthread_create(tid + n, NULL, row_check, &data[n]);
}


for(n=0; n < NUM_OF_THREADS; n++)
{
     pthread_join(tid[n], NULL);
}

This way the application will wait for all the threads to complete their tasks (and report any errors) before returning.

Myst
  • 18,516
  • 2
  • 45
  • 67
1

Can anyone suggest why it would exit without any error ?.

Yes,

the posted code has no action when all 9 rows of the puzzle result are correct, it just gracefully exits.

and to further muddy the logic.

The posted code only checks the last thread created, and when that thread exits, the program exits, That does not mean the other threads have exited

One further serious detail. the call to pthread_create() is passing the address of the attr variable, but that variable contains what ever trash is/was on the stack where that variable was declared. Since the code is not setting any specific attributes for the threads, strongly suggest eliminate the variable and simply use NULL in the second parameter to pthread_create()

user3629249
  • 16,402
  • 1
  • 16
  • 17