Sidebar Sidebar
Become a MacRumors Supporter for $50/year with no ads, ability to filter front page stories, and private forums.

macOS debugging threads

F

farmerdoug

macrumors 6502a
Original poster
Sep 16, 2008
541
0
The threads aren't asked to do anything, yet the program crashes after getting part way.
0
thread 0, 175
1
thread 0, 175
2

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000


Code: 
   pthread_t qthread[Quad_Threads];
    long qt;
    
  for (quad = 0; quad < Quad_Threads; quad++)
                        {
                                qt = pthread_create(&qthread[quad], NULL, match(tmpimage, tmptarget,innerradius, outerradius,(int)quad, minarray, magx, xshift, yshift), NULL);
                                if (qt)
                                {
                                printf("ERROR; return code from pthread_create() is %ld\n", qt);
                                exit(-1);
                                }
                                else
                                printf("thread %ld, %d\n", qt, magx);
      

                        
                        }   


void * match(float *tmpimage,float* tmptarget,int innerradius, int outerradius, int quad, float * minarray, int magx, int xshift, int yshift)
{
        
    int rowstart, colstart, rowend, colend;
    float sd;
     printf("%d\n", quad);   
//    rowstart = colstart = innerradius + red_ctr;
//    rowend = colend = outerradius + red_ctr; 
   /*  sd = sd_of_difference(tmpimage, tmptarget,rowstart, rowend, colstart, colend);
    if (sd < minarray[quad*4])
            {  
            minarray[quad*4] = sd;
            minarray[quad*4 + 1] = magx;
            minarray[quad*4 + 2] = xshift;
            minarray[quad*4 + 3] = yshift;
            } 

    */                                          
return(0);

}

suggestions?
 
There's no such thing as a thread that doesn't do anything. When you create a thread, it starts running. It can start running code that causes the thread to pause, but that is code you must write and provide.

Looking at the man page for pthread_create(), you should pass the pointer to a function that the thread will begin running upon creation: its start_routine. Unfortunately, the code you've posted isn't passing a pointer to a function, it's passing a void* returned by calling the function match(). Looking at match(), we see it always returns 0. So you're creating a thread and telling it to run the start_routine NULL. Hence the crash at 0.

I recommend that you find an example that shows pthread_create() being used in known-working code. Then pattern your code after that.

It would also help if you learned more about pointers. Especially the meaning and uses of void*.
 
The functioning being sent to each thread is only asked to return and I did look at code examples but none of the examples show code that was doing much more than printing. So perhaps I lost the thread. (sorry couldn't resist). Will try again, I think you have shown the way.

Thanks.
 
So I read the man page and went back to thread code I wrote some time ago


Code: 
void *pipeline(void *fname )
{ ... }

rc = pthread_create(&threads[i], NULL, pipeline, (void *)fitsfiles[j]);

Is it then true that I can only send a single argument to a thread? Do I need to use a structure to get it all in there?
 
So now what? The loop runs 3 times when it should run 4 and I get a Could not access memory error.
Code: 
struct input{
        float * varimage;
        float * fixedimage;
        int r1;
        int r2;
        int quart;
        float * factors;
        int magnif;
        int xs;
        int ys;
        } ;

 struct input * matchinput;
    matchinput = (struct input *)calloc(1, sizeof (matchinput) );


  for (quad = 0; quad < Quad_Threads; quad++)
           {
            qt = pthread_create(&qthread[quad], NULL, match(matchinput), (struct{} *) matchinput);
}


void * match(struct input *para) 
{        

     printf("%d %d\n", para->xs, para->quart);   

return(0);

}
 
Your pthread_create function is a bit off.

Code: 
qt = pthread_create(&qthread[quad], NULL, match, (void *) matchinput);

Edit: Oh and your match() function should probably return int not a void pointer, especially as you are returning 0 which wouldn't work with what you have anyway.

Edit 2: Bah I hate threads. Returning a value from your pthread_create() implicitly calls pthread_exit() with the return value so make sure that is what you want.
 
farmerdoug said:
So now what? The loop runs 3 times when it should run 4 and I get a Could not access memory error.
Click to expand...

You need to look more closely at the working example.

Your most recent code that fails:
Code: 
qt = pthread_create(&qthread[quad], NULL, match(matchinput), (struct{} *) matchinput);

Your earlier code that works (or so you said):
Code: 
rc = pthread_create(&threads[i], NULL, pipeline, (void *)fitsfiles[j]);

Note the differences very carefully.

In the failing one, you are calling the match function in the current thread and passing its return value as the start_routine function-pointer. In the working one, you are using the name of the pipeline function, which evaluates to a pointer to the function (standard C behavior, no magic here). These two things are completely different.

Second, the calloc()'ed struct is utterly the wrong size. You should print sizeof(matchinput) and see if it correlates to the expected size of the struct. You should also print the size of the struct, so you can see if they're the same. If not, think about why not, and what would be an appropriate size.
Code: 
 // This code is wrong.
  struct input * matchinput;
  matchinput = (struct input *)calloc(1, sizeof (matchinput) );

Third, the type-cast to struct {} is completely wrong. Probably harmless, but nevertheless completely wrong (cargo cult programming [1]). If you're going to cast it to anything, you should be casting it to void*.
Code: 
// Fixed code.
qt = pthread_create(&qthread[quad], NULL, match, (void *) matchinput);


[1] http://en.wikipedia.org/wiki/Cargo_cult_programming
 
Also, make sure anything you pass to the thread is not a stack variable. It must be global or static storage dedicated for each thread. The threads operate asynchronously to the creating thread. Thus, if you pass a stack variable to the thread, by the time the thread reads it the value may have been changed by the creating thread.
 
Guilty as charged: Cargo cult and shotgun programming.
But I do learn. I just forget by the time I have to do this again.
So I've made some changes. The code compiles with no warnings but runs somewhat strangely.
The mag values are right but the quad values should go from 0 to 3.
mag 175 quad 1
mag 175 quad 3
mag 175 quad 3
mag 175 quad 3
mag 176 quad 1
mag 176 quad 2
mag 176 quad 3
mag 176 quad 3
mag 177 quad 1
mag 177 quad 2
mag 177 quad 3
mag 177 quad 3
mag 178 quad 1
mag 178 quad 2
mag 178 quad 3
mag 178 quad 3
mag 179 quad 1
mag 179 quad 2
mag 179 quad 3
mag 179 quad 3

Code: 
struct input * matchinput; // defined globally

// within main 
matchinput = (struct input *)calloc(1, sizeof (*matchinput) );
  
    
    pthread_t qthread[Quad_Threads];
    long qt;

   for (quad = 0; quad < Quad_Threads; quad++)
                        {
                                
                                matchinput->quart = quad;
                                qt = pthread_create(&qthread[quad], NULL, (void*)match, (void*) matchinput);
                                if (qt)
                                {
                                printf("ERROR; return code from pthread_create() is %ld\n", qt);
                                exit(-1);
                                }
                            //    else
                            //    printf("thread for mag %d and quad %d\n", magx, quad);
                           }   

// the subroutine
void * match(struct input *para) 
{    
 printf("mag %d quad %d\n",para->magnif,  para->quart); 
    
    /*int rowstart, colstart, rowend, colend;
    float sd;
     
    rowstart = colstart = innerradius + red_ctr;
    rowend = colend = outerradius + red_ctr; 
   sd = sd_of_difference(tmpimage, tmptarget,rowstart, rowend, colstart, colend);
        if (sd < minarray[quad*4])
            {  
            minarray[quad*4] = sd;
            minarray[quad*4 + 1] = magx;
            minarray[quad*4 + 2] = xshift;
            minarray[quad*4 + 3] = yshift;
            } 

    */
 pthread_exit(NULL);

}
 
I'm casting match to void to avoid a warning about incompatible pointer type.

One item in the structure changes.
matchinput->quart = quad;
everything else is suppose to be the same. I'll look up interlocks.

I'm trying to send out an attachment in a private message. Can that be done?
It doesn't look like it. I'd like you guys to see what you are working on.
 
farmerdoug said:
One item in the structure changes.
matchinput->quart = quad;
everything else is suppose to be the same. I'll look up interlocks..
Click to expand...

You've completely missed the point. NOTHING in the structure can change once the thread is started. If anything changes it, then your new thread is no longer independent.

Interlocks won't solve this, except by adding complexity. Interlocked threads are not independent, thus defeating the purpose of multiple independent threads.

You need to think this through more carefully, instead of just throwing code at it.
 
Each thread gets exactly the same data except the quadrant number which sets what part of the data it uses; the results are put in thread specific locations in minarray.
 
farmerdoug said:
Each thread gets exactly the same data except the quadrant number which sets what part of the data it uses; the results are put in thread specific locations in minarray.
Click to expand...

You're using the same struct input variable (i.e. the same single memory location) to store multiple different quadrant numbers. This occurs in the main thread, right before it starts each sub-thread. This is wrong.

Each thread must have its own independently readable struct input. Nothing can write to the same memory location: not the individual sub-threads, nor the main thread. They can all read the same memory locations without ill effects.
 
Then something like this but its still not right.



Code: 
  for (quad = 0; quad < Quad_Threads; quad++)
                        {   
                        matchinput[quad]->magnif = magx;
                        matchinput[quad]->xs = matchinput[quad]->ys = 0;
                        matchinput[quad]->varimage = magnify2andshift(tmpshort, cys1 + slice*reduced_image, reduced_size ,reduced_size, (float) magx/reduced_size, (float) magx/reduced_size, xshift, yshift);  // magnified image
                        matchinput[quad]->fixedimage = cys1 + slice*reduced_image;

                            printf("thread for mag %d and quad %d\n", magx, quad);
                            matchinput[quad]->quart =  quad;
                            switch(quad)
                                {
                                case 0:  //upper right
                                    {
                                        matchinput[quad]->r1 = matchinput[quad]->c1 = innerradius + red_ctr;
                                        matchinput[quad]->r2 = matchinput[quad]->c2 = outerradius + red_ctr; 
                                                                          
                                        break;
                                    }   
                                case 1: //upper left
                                    {
                                        matchinput[quad]->r1 = innerradius + red_ctr;
                                        matchinput[quad]->r2 = outerradius + red_ctr;
                                        matchinput[quad]->c1 = -outerradius + red_ctr;
                                        matchinput[quad]->c2 = -innerradius + red_ctr;
                                                                                         
                                        break;
                                    }  
                                case 2: //lower left
                                    {
                                        matchinput[quad]->r1 = matchinput[quad]->c1 = -outerradius + red_ctr;
                                        matchinput[quad]->r2 = matchinput[quad]->c2 = -innerradius + red_ctr;
                                                                                               
                                        break;
                                    }  
                                case 3: // lower right
                                    {   
                                        matchinput[quad]->r1 = -outerradius + red_ctr;
                                        matchinput[quad]->r2 = -innerradius + red_ctr;
                                        matchinput[quad]->c1 = innerradius + red_ctr;
                                        matchinput[quad]->c2 = outerradius + red_ctr;
                                                                                          
                                        break;
                                    }  
                                }  // end switch
                                qt = pthread_create(&qthread[quad], NULL, (void*)match, (void*) matchinput[quad]);
                                if (qt)
                                {
                                printf("ERROR; return code from pthread_create() is %ld\n", qt);
                                exit(-1);
                                }                            
                        }   

 for (quad = 0; quad < Quad_Threads; quad++)
            printf("%d  %f\n",  quad, matchinput[quad]->sd);
 
Post the code that declares and initializes the matchinput array.

Post the output. Or whatever information you used to reach your conclusion that it's still not right.
 
Code: 
struct input{
        float * varimage;
        float * fixedimage;
        int r1;
        int r2;
        int c1;
        int c2;
        int quart;
        int magnif;
        int xs;
        int ys;
        float *sd;
        } ;

struct input ** matchinput;
// one structure for every quadrant
 matchinput = (struct input **)calloc(4, sizeof (void *) );
    for (i = 0; i < Quad_Threads; i ++)
        {
        matchinput[i] = (struct input*) calloc(1, sizeof (*matchinput));
        }
  
... // calling the thread
        
                                case 3: // lower right
                                    {   
                                        matchinput[quad]->r1 = -outerradius + red_ctr;
                                        matchinput[quad]->r2 = -innerradius + red_ctr;
                                        matchinput[quad]->c1 = innerradius + red_ctr;
                                        matchinput[quad]->c2 = outerradius + red_ctr;
                                                                                          
                                        break;
                                    }  
                                }  // end switch
                                qt = pthread_create(&qthread[quad], NULL, (void*)match, (void*) matchinput[quad]);
                                if (qt)
                                {
                                printf("ERROR; return code from pthread_create() is %ld\n", qt);
                                exit(-1);
                                }                            

// the function to be thread
void * match(struct input *para) 
{    
    
    int quad;
    float sd;
    para->sd = (float*)calloc(1, sizeof(float));
        quad = para->quart;
        sd = sd_of_difference(tmpimage, tmptarget, para->r1, para->r2, para->c1, para->c2);
        *para->sd = sd;
        printf("mag %d quad %d %f %f \n",para->magnif,  para->quart, *para->sd, sd); 

  // printf out varies depending on whether I print *para->sd, sd or both.  
free(para->sd);
pthread_exit(NULL);

}

// can't access memory error here
 for (quad = 0; quad < Quad_Threads; quad++)
            printf("%d  %f\n",  quad, *matchinput[quad]->sd);
 
Almost . I am moving forward in the code before the last thread finishes and I still get different values for sd each time I run (not depending on how I print). Will look at initialization.

Code: 
matchinput = (struct input **)calloc(4, sizeof (void *) );
    for (i = 0; i < Quad_Threads; i ++)
        {
        matchinput[i] = (struct input*) calloc(1, sizeof (*matchinput));
        for (j = 0; j < Quad_Threads; j ++)
         matchinput[i]->sd = (float*) calloc(4, sizeof (float));

        }
 
Match calls sd_of_difference. Is that a problem?


Code: 
void * match(struct input *para) 
{    
    
    int quad;
    float sd;
         quad = para->quart;
        sd = sd_of_difference(tmpimage, tmptarget, para->r1, para->r2, para->c1, para->c2);
        para->sd[para->quart] = sd;
        printf("mag %d quad %d %f %f\n",para->magnif,  para->quart,    para->sd[para->quart] , sd); 

    /*    if (sd < minarray[quad*4])
            {  
            minarray[quad*4] = sd;
            minarray[quad*4 + 1] = para->magnif;
            minarray[quad*4 + 2] = para->xs;
            minarray[quad*4 + 3] = para->ys;
            } 
*/
pthread_exit(NULL);

}

float  sd_of_difference(float *tmpimage,float* tmptarget,int rowstart,int rowend,int colstart,int colend)
{
    float sum = 0, sum2 = 0, var, diff;
  
    int irad, orad ,radius, r2, i = 0;
    irad = rowstart*rowstart;
    orad = rowend*rowend;
      for (row = 0; row <= rowend; row++)
        {
            r2 = row*row;
            for (col = 0; col <= colend; col++)
                {   i++;
                   radius = r2 + col*col;
                    if  ( ( radius > irad)  && (radius <= orad))
                        {
                        diff = (tmptarget[row*reduced_size + col]  -  tmpimage[row*reduced_size + col]);
                        sum  += diff;
                        sum2 += diff*diff;
                        }
                }
        }
      
  var = sum2/i - sum*sum/(i*i);      
     
return (var);

}
 
farmerdoug said:
Code: 
matchinput = (struct input **)calloc(4, sizeof (void *) );
    for (i = 0; i < Quad_Threads; i ++)
        {
        matchinput[i] = (struct input*) calloc(1, [COLOR="Red"]sizeof (*matchinput)[/COLOR]);
[COLOR="Green"]        for (j = 0; j < Quad_Threads; j ++)
         matchinput[i]->sd = (float*) calloc(4, sizeof (float));
[/COLOR]        }
Click to expand...

The red-highlighted code is very wrong, given the type of matchinput. As before, think about it, maybe even print the sizeof expression.

The green-highlighted code makes no sense. It's unclear what it's intended to do, but I'm certain that making multiple callocs and storing into the same variable (matchinput->sd) is wrong, simply because it leaks memory. Sadly, matchinput[j]->sd would also be wrong, because when the code is executed, not all matchinput pointers have been calloc()'ed yet.

Frankly, I think you need to focus on exactly how parameters are passed in, and exactly how results are passed out. You need to do this without making any assumptions about when the thread executes after being created. That is, you need to take asynchronous execution and indeterminate scheduling fully into account. I can tell by the code and the design that you're still "thinking synchronously".

The use of sd as an array (or even a pointer) is unnecessarily complex, if I correctly understand the expected output value: one float value per call to match().

I think you should test your code by not using threads. Simply call match() on each parameter block sequentially, in a single thread (i.e. "synchronous thinking"). If the overall outputs aren't what you expect, then you know how to debug it. Once you have the expected outputs when run sequentially, you can spawn threads.


farmerdoug said:
Match calls sd_of_difference. Is that a problem?
Click to expand...

Do you think it's a problem? Explain why.
 
The previous sizeof (*matchinput) was right before I changed the definition of matchinput and delcaring sd a point was some what silly.


matchinput = (struct input **)calloc(4, sizeof (void *) );
for (i = 0; i < Quad_Threads; i ++)
{
matchinput = (struct input*) calloc(1, sizeof (**matchinput));
}

But the code did run before I tried to thread it and each call gets a separate matchinput so I'm not sure where the issue of synchronicity comes in yet. Do I need four different subroutines, one for each thread?
 
You must log in or register to reply here.
Register on MacRumors! This sidebar will go away, and you'll see fewer ads.
Top Bottom
Back