0

I was wondering if anyone could help me with interpreting the Valgrind error to trouble shoot. There are no memory leaks, but I am getting this error "Conditional jump or move depends on uninitialized values" along with others. Apologies if it seems like a lot, I am just a little lost!

dictionary.c:45 refers to the line in the function check, starting with "if(strcasecmp(word, checktemp->word) !=0)"

line 32 is slightly before that "node *checktemp = malloc(sizeof(node))

Thank you very much, really appreciate it.

The Valgrind error is as follows, followed by my code:

==18498== Conditional jump or move depends on uninitialised value(s)
==18498==    at 0x520A60F: tolower (ctype.c:46)
==18498==    by 0x4C33629: strcasecmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4010D9: check (dictionary.c:45)
==18498==    by 0x400C89: main (speller.c:112)
==18498==  Uninitialised value was created by a heap allocation
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401071: check (dictionary.c:32)
==18498==    by 0x400C89: main (speller.c:112)
==18498== 
==18498== Use of uninitialised value of size 8
==18498==    at 0x520A623: tolower (ctype.c:46)
==18498==    by 0x4C33629: strcasecmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4010D9: check (dictionary.c:45)
==18498==    by 0x400C89: main (speller.c:112)
==18498==  Uninitialised value was created by a heap allocation
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401071: check (dictionary.c:32)
==18498==    by 0x400C89: main (speller.c:112)
==18498== 
==18498== Invalid read of size 8
==18498==    at 0x4010EB: check (dictionary.c:47)
==18498==    by 0x400C89: main (speller.c:112)
==18498==  Address 0x6743c70 is 48 bytes inside a block of size 56 free'd
==18498==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4012CE: load (dictionary.c:123)
==18498==    by 0x400964: main (speller.c:40)
==18498==  Block was alloc'd at
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401234: load (dictionary.c:99)
==18498==    by 0x400964: main (speller.c:40)
==18498== 
A
cat
is
not
a
caterpillar
==18498== Invalid read of size 8
==18498==    at 0x401344: unload (dictionary.c:147)
==18498==    by 0x400E09: main (speller.c:152)
==18498==  Address 0x6742ef0 is 48 bytes inside a block of size 56 free'd
==18498==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4012CE: load (dictionary.c:123)
==18498==    by 0x400964: main (speller.c:40)
==18498==  Block was alloc'd at
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401234: load (dictionary.c:99)
==18498==    by 0x400964: main (speller.c:40)
==18498== 
==18498== Invalid read of size 8
==18498==    at 0x401363: unload (dictionary.c:151)
==18498==    by 0x400E09: main (speller.c:152)
==18498==  Address 0x6742df0 is 48 bytes inside a block of size 56 free'd
==18498==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4012CE: load (dictionary.c:123)
==18498==    by 0x400964: main (speller.c:40)
==18498==  Block was alloc'd at
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401234: load (dictionary.c:99)
==18498==    by 0x400964: main (speller.c:40)
==18498== 

WORDS MISSPELLED:     6
WORDS IN DICTIONARY:  143091
WORDS IN TEXT:        6
TIME IN load:         1.77
TIME IN check:        0.04
TIME IN size:         0.00
TIME IN unload:       0.06
TIME IN TOTAL:        1.87

==18498== 
==18498== HEAP SUMMARY:
==18498==     in use at exit: 0 bytes in 0 blocks
==18498==   total heap usage: 143,102 allocs, 143,102 frees, 8,023,752 bytes allocated
==18498== 
==18498== All heap blocks were freed -- no leaks are possible
==18498== 
==18498== For counts of detected and suppressed errors, rerun with: -v
==18498== ERROR SUMMARY: 195546 errors from 5 contexts (suppressed: 0 from 0)

Asking for help...

==18498== Conditional jump or move depends on uninitialised value(s)

Looks like you're trying to use a variable that might not have a value? Take a closer look at line 45 of dictionary.c.
// Implements a dictionary's functionality

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <cs50.h>
#include <string.h>
#include <strings.h>
#include <ctype.h>
#include "dictionary.h"

// Represents a node in a hash table
typedef struct node
{
    char word[LENGTH + 1];
    struct node *next;
}
node;

// Number of buckets in hash table
const unsigned int N = 50; //change variable as needed

// Hash table
node *table[N];

int wordcount[1];

// Returns true if word is in dictionary else false
bool check(const char *word)
{
    int hcheck = hash(word);
    node *checktemp = malloc(sizeof(node));
    checktemp->next = NULL;

    if(table[hcheck] == NULL)
    {
        free(checktemp);
        return false;
    }
    else
    {
        checktemp->next = table[hcheck];
        while(checktemp->next != NULL)
        {
            if(strcasecmp(word, checktemp->word) != 0)
            {
                checktemp->next = checktemp->next->next;
            }
            else
            {
                return true;
            }
        }

    }
    
    free(checktemp);
    return false;
}

// Hashes word to a number
unsigned int hash(const char *word)
{
    // Hashes word to a number
    // Source: djib2 by Dan Bernstein (http://www.cse.yorku.ca/~oz/hash.html)
    
    unsigned long hash = 5381;
    int c = *word;
    c = tolower(c);

    while (*word != 0)

    {
        hash = ((hash << 5) + hash) + c;
        c = *word++;
        c = tolower(c);
    }

    return hash % N;
}

// Loads dictionary into memory, returning true if successful else false
bool load(const char *dictionary)
{
    // TODO

    FILE *file = fopen(dictionary, "r");
    if (file == NULL)
    {
        unload();
        return false;
    }

    char wordcopy[LENGTH+1];
    int count = 0;

    while(fscanf(file, "%s", wordcopy) != EOF)
    {
        node *node1 = malloc(sizeof(node));
        
        if(node1 == NULL)
        {
            unload();
            return false;
        }
        else
        {
            
            strncpy(node1->word, wordcopy, sizeof(wordcopy));

            int hash1 = hash(wordcopy);

            if(table[hash1] == NULL)
            {
                table[hash1] = node1;
            }
            else
            {
                node1->next = table[hash1];
                table[hash1] = node1;
            }
            
            free(node1);
            count++;
        }
    }

    fclose(file);
    wordcount[0] = count;
    return true;
}

// Returns number of words in dictionary if loaded else 0 if not yet loaded
unsigned int size(void)
{
    // TODO
    return wordcount[0];
}

// Unloads dictionary from memory, returning true if successful else false
bool unload(void)
{
    for(int i = 0; i < N; i++)
    {
        if(table[i] != NULL)
        {
            node *cursor = table[i]->next;
            while(cursor != NULL)
            {
                node *temp = cursor;
                cursor = cursor->next;
            }
        }
    }
    
    return true;
}
0
if(strcasecmp(word, checktemp->word) !=0)

This means that you are checking that if both words are not equal,

 // do something

Infact you should try to look if both words are matched,

 // do something

If you wanna prefer first style (mentioned above) then you need to return false so that function won't proceed further and word is not in the dictionary

If you wanna perfere second style (mentioned above) then you need to check if,

 if(strcasecmp(words_to_be_matched) == 0)

{
// do something return true; }

If this helps be sure to tick the answer to keep other smiling too :)

3
  • Hi, I'm sorry, but it does not seem to help. I have tried it both ways, with strcasecmp testing for true and for false. – leeetthad Jul 22 '20 at 8:33
  • Ok wait let me reply you in few moments – Shahroz Ali Jul 22 '20 at 8:56
  • The usage of if(strcasecmp(word, checktemp->word) !=0) is correct, even if it isn't the standard usage. (Best practice would have been to check for equality, with the if code block and the else code block switched.) If they do not match, a non-zero number will be returned and the code should move on to the next word. If they do match, 0 will be returned and the success code in the else clause will execute. However, the loop should be looking at checktemp == NULL, not checktemp->next == NULL to continue. By checking ->next, it will not check the last word in the linked list. – Cliff B Jul 22 '20 at 17:44
0

So, couple of bugs I noted.

  • Why are you declaring array of word count? You just need to update the global word_count when a new word from the dictionary has been successfully hashed in the linked list
  • Why are you calling unload() function inside your load function? Suppose that no list is created then unload function will fail to execute and return NULL
  • Why are you freeing the node after a successful hash? If you free the node then linked list won't be updated with new words and unload function can unload from the memory because everytime you hashed the node in the table, you are also freeing the node
  • Why are you mallocing the space for temporary cursor when its work is only to point at head of linked list to check the word? It should be a node type pointer to point to the head of the hash table
6
  • Hi thanks for the comments. I'll see if I can address them in order – leeetthad Jul 22 '20 at 11:00
  • Hi thanks for the comments. I'll see if I can address them in order. 1) Not sure if it matters, I made it an int global variable. same valgrind problem 2) Noted, thanks! Yeah, not sure why I put unload there. For the last two comments, I wasn't sure exactly what it meant, but I rewrote my unload function and I found out that the problem was with my unload function. It wasn't quite working the way I intended, and the valgrind errors were actually not because of the load or check functions, but the freeing of allocated memory. Thank you! – leeetthad Jul 22 '20 at 11:06
  • No problem mate 👍 Just be sure to tick the answer to keep other smiling too 😃 – Shahroz Ali Jul 22 '20 at 12:55
  • There are two calls to unload() in load. The first one is not needed because no nodes have been created at this point. However, the second one is appropriate. Unless it fails on the first word, there will be words in the tree that need to be freed. But it doesn't matter whether there are no nodes. unload is a cleanup process that will execute even if there are no nodes, and will always return true. Oh, and it never returns NULL. As for the temp pointer checktemp, initialize it to NULL. He's right that you should not malloc memory to it. – Cliff B Jul 22 '20 at 17:40
  • @ShahrozAli In the future, can you please only post one answer to a question? If necessary, edit the answer and add to it. thanks. – Cliff B Jul 22 '20 at 17:42

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .