0

I have the following tree node struct that holds pointers to other tree nodes: 

struct node {
    // ...
    struct node* children[20];
}

The idea is that I want to check whether there is node* inside the children and based and that go deeper into the tree. So when I allocate the node I want to have children with 20 NULL values. Currently I am not doin

  1. How should I allocate this array in order to not get errors like Conditional jump or move depends on uninitialised value(s) (Valgrind)?
  2. Would it be better to use struct node** children and allocate fixed size each time I allocate a new node?

EDIT: Example of one place where Valgrind complains:

for(int i=0;i<20;i++)
  if(node->children[i] != NULL)
    do_something_with_the_node(node->children[i]);
Improve this question
4
  • 1
    If you allocate the structure dynamically, use calloc instead of malloc. Or use memset to set all contents to zero. Commented Apr 23, 2015 at 6:46
  • Conditional jump or move depends on uninitialised value(s) reported by valgrind means that you have an if statement, in which you're comparing node[x], right? Can you post that code too? Commented Apr 23, 2015 at 6:51
  • @g.tsh Yes. I've added an example. Commented Apr 23, 2015 at 7:01
  • @JoachimPileborg No, you can't portably use calloc() to set pointers to NULL. Commented Apr 23, 2015 at 7:25

3 Answers 3

Reset to default
1

When you allocate a new instance of struct node, you must set the contained pointers to NULL to mark them as "not pointing anywhere". This will make the Valgrind warning go away, since the pointers will no longer be uninitialized.

Something like this:

struct node * node_new(void)
{
  struct node *n = malloc(sizeof *n);
  if(n != NULL)
  {
    for(size_t i = 0; i < sizeof n->children / sizeof *n->children; ++i)
      n->children[i] = NULL;
  }
  return n;
}

You cannot portably use either memset() on n->children nor calloc(), since those will give you "all bits zero" which is not the same as "pointer NULL".

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Thanks, one more question: what is the difference between memset()/calloc() and setting pointer to NULL?
0

Your struct definition is valid (although it's hard to tell without more context if it fits your requirements).

Valgrind doesn't complain about your struct definition, it probably complains about how you instantiate variables of that type. Ensure that all of the array members get initialized and the complaints will most likely go away.

Improve this answer

2 Comments

Currently I don't use {c,m} alloc to allocate the memory for this array (as I understand, I don't have to in this context). What I do later though is something like: node->chlidren[10] = some_node_pointer; (where values at other indices may stay uninitialized).
which is what valgrind complains about. In your example you added, you call do_something_with_the_node() depending on whether it's NULL or something else. You seem to expect the array members would be initialized to NULL automatically when they aren't. You need to explicitely initialize the whole array to NULL (using memset() or calloc(), for example).
0

The problem is that you are using an unintialized value in an if condition.

When you instantiate a struct node, its member struct node* children[20]; is an array of 20 struct node *, all of which are uninitialized.

It would be no different from this:

char *x;

if (x == NULL) {
    /* Stuff */
}

At this point, x may have literally any value. In your example, any element of an array may have any value.

To fix this, you need to initialize the elements of an array before using them, for example like this:

for (int i = 0; i < 20; ++i) {
    node->children[i] = NULL;
}

Or shorter:

memset(node->children, 0, 20);

If you changed the member to, as you've suggested, node **children, the situation wouldn't be much different - you'll still need to initialize all the members, including array's elements. You could make it shorter by using calloc, which will initialize all bytes to 0; then again, you'll need some code for correct deallocation (and remember to do it), so I think the tradeoff's not worth it.

Improve this answer

1 Comment

Oh. IDK. Perhaps it's the issue mentioned by @unwind - that setting all zeroes instead of explicit NULL is not portable.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.