0

Solved

Problem ultimately stemmed from the design of the data structure. To remove the root element there had to be a heap allocated (new) pointer to it, which wasn't possible in the original case where the data was held directly by the tree. Now there is a node struct holding all the data, with the tree class containing all the methods and the root pointer.


I have a double pointer target_address initialized to NULL and I have to check whether *target_address is NULL later. In between, a function guarantees that *target_address points to an uninitialized value, but Valgrind keeps complaining that I'm reading an uninitialized value only when I remove the root element.

template <typename T>
int tree<T>::remove(T target) {
    if (!this)  // root is empty
        return EMPTY;

    tree<T>** target_address = NULL;

    if (tree_find(target, &target_address) == SUCCESS) {
        // expect there to be something in target_address
        if (*target_address == NULL) return EMPTY;  // <---- error happens

        tree_delete(target_address);
        return SUCCESS;
    }
    else return EMPTY;
}

tree_find that operates on target_address

template <typename T>
int tree<T>::tree_find(T key, tree<T>*** target_address_handle) {
    // find tree matched by key, or NULL pointer in correct location
    // give tree pointer address back
    tree<T>* root = this;   // <---- ensures *target_address points to valid value, maybe this is problematic?
    tree<T>** target_address = &root;
    while(*target_address) {
        tree<T>* current = *target_address;
        if(typeid(key) == typeid(current->data)) {  
            //  assume comparison operator exists
            if(key == current->data)
                break;
            else if(key < current->data)
                target_address = &current->left;
            else
                target_address = &current->right;
        }
        else return FAIL;
    }
    // if loop exited without breaking, will insert into an empty NULL position
    // else loop exited by matching/breaking, will delete non-NULL tree
    *target_address_handle = target_address;
    return SUCCESS;
}

tree_delete that also complains of uninitialized value; I'm pretty sure it's also complaining of *target_address

void tree<T>::tree_delete(tree<T>** target_address) {
    tree<T>* target = *target_address;
    // first case: no left subtree, replace with right subtree (or NULL for a leaf) 
    if (!target->left) 
        *target_address = target->right;

    // second case: no right subtree, replace with left subtree
    else if (!target->right) 
        *target_address = target->left;

    // third case: both subtrees, find second largest by taking rightmost left tree
    else {
        tree<T>** second_largest_address = &target->left;  // start at target's left tree
        while ((*second_largest_address)->right)  // keep going right
            second_largest_address = &((*second_largest_address)->right);  
        // reached the rightmost left
        tree<T>* second_largest = *second_largest_address;
        *target_address = second_largest;  // delete target by replacing it with second largest
        // second largest guranteed to not have a right subtree, so can treat as case 2 by shifting
        *second_largest_address = second_largest->left;  
        second_largest->left = target->left;
        second_largest->right = target->right;
    }
    delete target;
}

P.S. Tips for getting Valgrind to display line numbers would be appreciated, compilation flags = -g -Wall -Werror -std=c++11 and Valgrind's run under -q --track-origins=yes

I tried static linking -static as suggested in someone else's question, but that introduced many more problems and solved none...


Error messages

==25887== Conditional jump or move depends on uninitialised value(s)
==25887==    at 0x401764: tree<int>::remove(int) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x4013AA: main (in /home/johnson/Code/tree/treetest)
==25887==  Uninitialised value was created by a stack allocation
==25887==    at 0x40175A: tree<int>::remove(int) (in /home/johnson/Code/tree/treetest)
==25887== 
After reading *target tree address
Before read
==25887== Use of uninitialised value of size 8
==25887==    at 0x401A75: tree<int>::tree_delete(tree<int>**) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x40178E: tree<int>::remove(int) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x4013AA: main (in /home/johnson/Code/tree/treetest)
==25887==  Uninitialised value was created by a stack allocation
==25887==    at 0x401A46: tree<int>::tree_delete(tree<int>**) (in /home/johnson/Code/tree/treetest)
==25887== 
==25887== Use of uninitialised value of size 8
==25887==    at 0x401AA5: tree<int>::tree_delete(tree<int>**) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x40178E: tree<int>::remove(int) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x4013AA: main (in /home/johnson/Code/tree/treetest)
==25887==  Uninitialised value was created by a stack allocation
==25887==    at 0x401A46: tree<int>::tree_delete(tree<int>**) (in /home/johnson/Code/tree/treetest)
==25887== 
==25887== Use of uninitialised value of size 8
==25887==    at 0x401AF2: tree<int>::tree_delete(tree<int>**) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x40178E: tree<int>::remove(int) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x4013AA: main (in /home/johnson/Code/tree/treetest)
==25887==  Uninitialised value was created by a stack allocation
==25887==    at 0x401A46: tree<int>::tree_delete(tree<int>**) (in /home/johnson/Code/tree/treetest)
==25887== 
==25887== Invalid read of size 8
==25887==    at 0x401AF5: tree<int>::tree_delete(tree<int>**) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x40178E: tree<int>::remove(int) (in /home/johnson/Code/tree/treetest)
==25887==    by 0x4013AA: main (in /home/johnson/Code/tree/treetest)
==25887==  Address 0x801f0fc36d is not stack'd, malloc'd or (recently) free'd
LemonPi
  • 1,026
  • 9
  • 22
  • can you show the definition of `tree::data` ? – M.M Jul 15 '14 at 02:42
  • 2
    `tree_find` returns a pointer to a stack variable if the data matches the first node... `tree* root` is stack variable, `tree** target_address = &root;` - address of stack variable, `*target_address_handle = target_address;` . - sends address of nonexistent variable back to caller. I'm not quite sure why you are returning the address of a pointer to the desired node, instead of just a pointer to the desired node! – M.M Jul 15 '14 at 02:44
  • Tree data's just `T data`, with the other attributes being `tree *left, *right` – LemonPi Jul 15 '14 at 02:44
  • `typeid(T) == typeid(T)` is always true then! – M.M Jul 15 '14 at 02:45
  • @MattMcNabb Ah right, that confirms my suspicion xD Hmm... Come to think of it, I don't think I need the triple pointer; I'll try getting rid of it and see if it still works And yeah, typeid(T) == typeid(T)... Forgot C++ is strongly typed and thought maybe someone could try search on an different typed tree haha – LemonPi Jul 15 '14 at 02:59
  • There's no such thing as `!this` in C++. Invoking a member function on a `nullptr` causes undefined behavior. Compilers typically optimize by assuming that undefined behavior cannot happen. I conjecture that your compiler is therefore removing the `if (!this)` conditional as dead code. – Casey Jul 15 '14 at 03:05
  • @Casey Thanks, didn't know that! @MattMcNabb Changing my code so that `tree_find(T key, tree** target_address)` still the problem of uninitialized parameter `target_address`; how would I properly initialize `target_address` before passing it to tree_find? – LemonPi Jul 15 '14 at 03:17
  • Is it possible to remove the root element given the current parameters (no root pointer)? In the C implementation I had to pass the root pointer to each function since there weren't class methods, but do I still have to pass it? I thought something could be done with `this` – LemonPi Jul 15 '14 at 03:50
  • @LemonPi It seems to me you have a design problem, as if `remove` finds that the first node is the one with the data in it, then `tree_delete` cannot work because it actually rejigs the tree and then `delete`s the node being deleted - but you can't `delete` the topmost node in the tree because you must have an external pointer to that somewhere so you know where your tree is. (NB. `tree_delete` actually does not use `this` at all; if you keep your current design then it could be a static function) – M.M Jul 15 '14 at 04:29
  • @MattMcNabb Yup, I also realized that soon afterwards; I'm refactoring by introducing a `struct node` holding the actual data with the tree class holding all methods and the root node pointer – LemonPi Jul 15 '14 at 04:31
  • My suggestion would be to stop using the technique of working with the address of the node to delete. Instead of `delete`ing a node and making the pointer that pointed to that node now point to the replacement node; just copy the replacement node by value and `delete` where the replacement came from. Hope that makes sense! – M.M Jul 15 '14 at 04:33

1 Answers1

2

Code like if (!this) return EMPTY; is highly suspicious. It's a no-op, and indicative of Undefined Behavior elsewhere. It's no wonder that Valgrind reports problems with your code.

Even worse, look at the code inside tree_find :

tree<T>* root = this;   // <-- ensures *target_address points to stack variable !
tree<T>** target_address = &root; // Set out pointer to stack variable

Obviously root doesn't exist then tree_find returns. There's no doubt that is an error.

MSalters
  • 173,980
  • 10
  • 155
  • 350