debugging threads

Discussion in 'Mac Programming' started by farmerdoug, Mar 11, 2010.

  1. farmerdoug macrumors 6502a

    Joined:
    Sep 16, 2008
    #1
    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?
     
  2. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #2
    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*.
     
  3. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #3
    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.
     
  4. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #4
    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?
     
  5. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #5
    Yes.

    Anything that is the same size as a single void*.

    Representation is left to you. Size is fixed.
     
  6. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #6
    So since sizeof (void *) == sizeof(struct {} *)
    I'm okay. Right?
     
  7. Cromulent macrumors 603

    Cromulent

    Joined:
    Oct 2, 2006
    Location:
    The Land of Hope and Glory
    #7
    Yeah. void* is defined as being big enough to hold any pointer. Unfortunately I don't have the C standard to hand to quote the passage.
     
  8. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #8
    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);
    
    }
    
    
     
  9. Cromulent macrumors 603

    Cromulent

    Joined:
    Oct 2, 2006
    Location:
    The Land of Hope and Glory
    #9
    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.
     
  10. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #10
    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
     
  11. mfram macrumors 65816

    Joined:
    Jan 23, 2010
    Location:
    San Diego, CA USA
    #11
    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.
     
  12. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #12
    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);
    
    }
    
     
  13. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #13
    You're sharing the same struct input across multiple threads. And you have no thread-safety interlocks.
     
  14. Cromulent macrumors 603

    Cromulent

    Joined:
    Oct 2, 2006
    Location:
    The Land of Hope and Glory
    #14
    Why are you casting match to void *? You should only be casting matchinput to void * in your pthread_create.
     
  15. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #15
    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.
     
  16. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #16
    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.
     
  17. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #17
    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.
     
  18. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #18
    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.
     
  19. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #19
    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);
    
    
    
     
  20. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #20
    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.
     
  21. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #21
    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);
    
     
  22. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #22
    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));
    
            }
    
     
  23. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #23
    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);
    
    }
    
    
    
     
  24. chown33 macrumors 604

    Joined:
    Aug 9, 2009
    #24
    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.


    Do you think it's a problem? Explain why.
     
  25. farmerdoug thread starter macrumors 6502a

    Joined:
    Sep 16, 2008
    #25
    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?
     

Share This Page