views:

787

answers:

4

Due to my feeble understanding of allocating type memory to pointers, the following causes a bus error on the call to barrier_create ("hi" is never printed).

typedef struct barrier barrier_t;
typedef struct barrier *barrier_p;

barrier_p test_barrier_p;

int main(int argc, char *argv[]) {
    barrier_create(*test_barrier_p);
}

int barrier_create(barrier_p *barrier_pointer) {
printf("hi\n");
    barrier_p old_barrier, new_barrier;
    int count;
    old_barrier = (barrier_p) *barrier_pointer;
    new_barrier = (barrier_p) malloc(sizeof(*new_barrier));
    count = pthread_mutex_init(&new_barrier->lock, NULL);
    new_barrier->is_valid = VALID_BARRIER;
    new_barrier->counter = 0;
    new_barrier->release_flag = 0;
    *barrier_pointer = new_barrier;
    return HAPPY_HAPPY_JOY_JOY;
}

What am I missing or mis-typing?

A: 

The test_barrier_p variable is a pointer to a barrier structure which is never being initialized, so it's set to NULL (since it's at file scope).

You're de-referencing it at the call from main() to barrier_create().

For help beyond that, you'll need to tell us, in English, what you're trying to achieve.

paxdiablo
+7  A: 

You're dereferencing a bad pointer in your main function. To get the address of the variable, you use the address & operator, not the dereferencing * operator. Rewrite main as:

barrier_create(&test_barrier_p);
Jason Coco
Jason is correct, you've got your indirection backwards.
Miquella
Thanks for the succinct answer. I'm giving it to feonixrift since he gave the bonus of pointing out my silly use of buffered print.
Noel
Also, thanks Miquella for that comment. That actually nailed down the problem exactly for me mentally.
Noel
+7  A: 
barrier_create(*test_barrier_p);

Since barrier_create takes address of a barrier_p, this should be &test_barrier_p, not *test_barrier_p.

printf("hi\n");

Inaccurate test of code reachability because stdout is likely buffered; I'd recommend fprintf(stderr, "hi\n"); instead.

new_barrier = (barrier_p) malloc(sizeof(*new_barrier));

I'd say sizeof(barrier_t). Again a * in an odd place, the _p notation may not be helping your type manipulation clarity.

For pedanticism, I would check the return value of malloc. I see little point in keeping the old value unless to recover in some way from a malloc error.

What is the purpose of count?

Kim Reece
Also correct, but a little more complete a description. But I don't think it would be sizeof(barrier_p), wouldn't it be sizeof(barrier_t)?
Miquella
excellent catch; editing to reflect.
Kim Reece
yes, definitely sizeof(barrier_t). sizeof(barrier_p) will just return the size of a pointer (pretty much 4 or 8 these days).
Jason Coco
yeah, old_barrier seems to just be leaking memory... maybe it was originally meant so that the barrier could be free()d after the global variable was set to the new barrier?
Jason Coco
yeah, could be designed for freeing, assuming it's never passed an uninitialized mess. this looks extracted from a broader api and adjusted to fit, to me.
Kim Reece
Since you're curious, I'm cutting down a sample synchronization barrier implementation for testing using pthreads. This is the one method out of the larger implementation that I confused myself on. count is there for debugging purposes. [[http://docs.hp.com/en/B3909-90015/apas04.html]]
Noel
Thanks for indulging my curiosity.
Kim Reece
Another thought you might consider as you are delving through things like this: if you temporarily compile it as C++, the extra type checking in C++ will tell you what the problem is. I did this to double check myself before I commented earlier.
Miquella
I disagree with using `sizeof(barrier_t)` instead of `sizeof *new_barrier` in general. It's a common idiom in C, particularly if the casts to `malloc` are removed. It prevents a silent bug at runtime if the pointee type changes without updating the `malloc` size.
jamesdlin
A: 

The function int barrier_create(barrier_p *barrier_pointer) takes a pointer as an argument. However, you a passing in an actual barrier_p in your main since you dereference it - barrier_create(*test_barrier_p). I think you should be passing the address like barrier_create(&test_barrier_p)

MeThinks