{"id":1069,"date":"2022-08-30T15:11:52","date_gmt":"2022-08-30T15:11:52","guid":{"rendered":"https:\/\/unknownerror.org\/index.php\/2013\/11\/09\/recursive-fib-with-threads-segmentation-fault-collection-of-common-programming-errors\/"},"modified":"2022-08-30T15:11:52","modified_gmt":"2022-08-30T15:11:52","slug":"recursive-fib-with-threads-segmentation-fault-collection-of-common-programming-errors","status":"publish","type":"post","link":"https:\/\/unknownerror.org\/index.php\/2022\/08\/30\/recursive-fib-with-threads-segmentation-fault-collection-of-common-programming-errors\/","title":{"rendered":"Recursive Fib with Threads, Segmentation Fault?-Collection of common programming errors"},"content":{"rendered":"<p>I tried to run your code, and came across several surprises:<\/p>\n<pre><code>printf(\"The number is: %d\\n\", finalFib);\n<\/code><\/pre>\n<p>This line has a small error: <code>%d<\/code> means <code>printf<\/code> expects an <code>int<\/code>, but is passed a <code>long int<\/code>. On most platforms this is the same, or will have the same behavior anyways, but pedantically speaking (or if you just want to stop the warning from coming up, which is a very noble ideal too), you should use <code>%ld<\/code> instead, which will expect a <code>long int<\/code>.<\/p>\n<p>Your <code>fib<\/code> function, on the other hand, seems non-functional. Testing it on my machine, it doesn&#8217;t crash, but it yields <code>1047<\/code>, which is not a Fibonacci number. Looking closer, it seems your program is incorrect on several aspects:<\/p>\n<pre><code>void *fib(void *fibToFind)\n{\n    long retval; \/\/ retval is never used\n\n    long newFibToFind = ((long)fibToFind);\n\n    long returnMinusOne; \/\/ variable is read but never initialized\n    long returnMinustwo; \/\/ variable is read but never initialized\n\n    pthread_t minusone; \/\/ variable is never used (?)\n    pthread_t minustwo; \/\/ variable is never used\n\n    if(newFibToFind == 0 || newFibToFind == 1)\n        \/\/ you miss a cast here (but you really shouldn't do it this way)\n        return newFibToFind;        \n    else{\n        long newFibToFind1 = ((long)fibToFind) - 1; \/\/ variable is never used\n        long newFibToFind2 = ((long)fibToFind) - 2; \/\/ variable is never used\n        \/\/ reading undefined variables (and missing a cast)\n        return returnMinusOne + returnMinustwo;\n\n    }\n}\n<\/code><\/pre>\n<p>Always take care of compiler warnings: when you get one, usually, you <em>really<\/em> are doing something fishy.<\/p>\n<p>Maybe you should revise the algorithm a little: right now, all your function does is returning the sum of two undefined values, hence the 1047 I got earlier.<\/p>\n<p>Implementing the Fibonacci suite using a recursive algorithm means you need to call the function again. As others noted, it&#8217;s quite an inefficient way of doing it, but it&#8217;s easy, so I guess all computer science teachers use it as an example.<\/p>\n<p>The regular recursive algorithm looks like this:<\/p>\n<pre><code>int fibonacci(int iteration)\n{\n    if (iteration == 0 || iteration == 1)\n        return 1;\n\n    return fibonacci(iteration - 1) + fibonacci(iteration - 2);\n}\n<\/code><\/pre>\n<p>I don&#8217;t know to which extent you were supposed to use threads\u2014just run the algorithm on a secondary thread, or create new threads for each call? Let&#8217;s assume the first for now, since it&#8217;s a lot more straightforward.<\/p>\n<p>Casting integers to pointers and vice-versa is a bad practice because if you try to look at things at a higher level, they should be widely different. Integers do maths, and pointers resolve memory addresses. It happens to work because they&#8217;re represented the same way, but really, you shouldn&#8217;t do this. Instead, you might notice that the function called to run your new thread accepts a <code>void*<\/code> argument: we can use it to convey both <em>where<\/em> the input is, and <em>where<\/em> the output will be.<\/p>\n<p>So building upon my previous <code>fibonacci<\/code> function, you could use this code as the thread main routine:<\/p>\n<pre><code>void* fibonacci_offshored(void* pointer)\n{\n    int* pointer_to_number = pointer;\n    int input = *pointer_to_number;\n    *pointer_to_number = fibonacci(input);\n    return NULL;\n}\n<\/code><\/pre>\n<p>It expects a pointer to an integer, and takes from it its input, then writes it output there.<sup>1<\/sup> You would then create the thread like that:<\/p>\n<pre><code>int main()\n{\n    int value = 15;\n    pthread_t thread;\n\n    \/\/ on input, value should contain the number of iterations;\n    \/\/ after the end of the function, it will contain the result of\n    \/\/  the fibonacci function\n    int result = pthread_create(&amp;thread, NULL, fibonacci_offshored, &amp;value);\n\n    \/\/ error checking is important! try to crash gracefully at the very least\n    if (result != 0)\n    {\n        perror(\"pthread_create\");\n        return 1;\n    }\n\n    if (pthread_join(thread, NULL)\n    {\n        perror(\"pthread_join\");\n        return 1;\n    }\n\n    \/\/ now, value contains the output of the fibonacci function\n    \/\/ (note that value is an int, so just %d is fine)\n    printf(\"The value is %d\\n\", value);\n    return 0;\n}\n<\/code><\/pre>\n<p>If you need to call the Fibonacci function from new distinct threads (please note: that&#8217;s not what I&#8217;d advise, and others seem to agree with me; it will just blow up for a sufficiently large amount of iterations), you&#8217;ll first need to merge the <code>fibonacci<\/code> function with the <code>fibonacci_offshored<\/code> function. It will considerably bulk it up, because dealing with threads is heavier than dealing with regular functions.<\/p>\n<pre><code>void* threaded_fibonacci(void* pointer)\n{\n    int* pointer_to_number = pointer;\n    int input = *pointer_to_number;\n\n    if (input == 0 || input == 1)\n    {\n        *pointer_to_number = 1;\n        return NULL;\n    }\n\n    \/\/ we need one argument per thread\n    int minus_one_number = input - 1;\n    int minus_two_number = input - 2;\n\n    pthread_t minus_one;\n    pthread_t minus_two;\n    \/\/ don't forget to check! especially that in a recursive function where the\n    \/\/ recursion set actually grows instead of shrinking, you're bound to fail\n    \/\/ at some point\n    if (pthread_create(&amp;minus_one, NULL, threaded_fibonacci, &amp;minus_one_number) != 0)\n    {\n        perror(\"pthread_create\");\n        *pointer_to_number = 0;\n        return NULL;\n    }\n    if (pthread_create(&amp;minus_two, NULL, threaded_fibonacci, &amp;minus_two_number) != 0)\n    {\n        perror(\"pthread_create\");\n        *pointer_to_number = 0;\n        return NULL;\n    }\n\n    if (pthread_join(minus_one, NULL) != 0)\n    {\n        perror(\"pthread_join\");\n        *pointer_to_number = 0;\n        return NULL;\n    }\n\n    if (pthread_join(minus_two, NULL) != 0)\n    {\n        perror(\"pthread_join\");\n        *pointer_to_number = 0;\n        return NULL;\n    }\n\n    *pointer_to_number = minus_one_number + minus_two_number;\n    return NULL;\n}\n<\/code><\/pre>\n<p>Now that you have this bulky function, adjustments to your <code>main<\/code> function are going to be quite easy: just change the reference to <code>fibonacci_offshored<\/code> to <code>threaded_fibonacci<\/code>.<\/p>\n<pre><code>int main()\n{\n    int value = 15;\n    pthread_t thread;\n\n    int result = pthread_create(&amp;thread, NULL, threaded_fibonacci, &amp;value);\n\n    if (result != 0)\n    {\n        perror(\"pthread_create\");\n        return 1;\n    }\n    pthread_join(thread, NULL);\n\n    printf(\"The value is %d\\n\", value);\n    return 0;\n}\n<\/code><\/pre>\n<p>You might have been told that threads speed up parallel processes, but there&#8217;s a limit somewhere where it&#8217;s more expensive to set up the thread than run its contents. <strong>This is a very good example of such a situation<\/strong>: the threaded version of the program runs much, much slower than the non-threaded one.<\/p>\n<p>For educational purposes, this program runs out of threads on my machine when the number of desired iterations is 18, and takes a few seconds to run. By comparison, using an iterative implementation, we never run out of threads, and we have our answer in a matter of milliseconds. It&#8217;s also considerably simpler. This would be a great example of how using a better algorithm fixes many problems.<\/p>\n<p>Also, out of curiosity, it would be interesting to see if it crashes on your machine, and where\/how.<\/p>\n<p><sub>1. Usually, you should try to avoid to change the meaning of a variable between its value on input and its value after the return of the function. For instance, here, on input, the variable is the number of iterations we want; on output, it&#8217;s the result of the function. Those are two very different meanings, and that&#8217;s not really a good practice. I didn&#8217;t feel like using dynamic allocations to return a value through the <code>void*<\/code> return value.<\/sub><\/p>\n<p id=\"rop\"><small>Originally posted 2013-11-09 23:21:11. <\/small><\/p>","protected":false},"excerpt":{"rendered":"<p>I tried to run your code, and came across several surprises: printf(&#8220;The number is: %d\\n&#8221;, finalFib); This line has a small error: %d means printf expects an int, but is passed a long int. On most platforms this is the same, or will have the same behavior anyways, but pedantically speaking (or if you just [&hellip;]<\/p>\n","protected":false},"author":1,"featured_media":0,"comment_status":"closed","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[1],"tags":[],"class_list":["post-1069","post","type-post","status-publish","format-standard","hentry","category-uncategorized"],"_links":{"self":[{"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/posts\/1069","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/comments?post=1069"}],"version-history":[{"count":0,"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/posts\/1069\/revisions"}],"wp:attachment":[{"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/media?parent=1069"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/categories?post=1069"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/unknownerror.org\/index.php\/wp-json\/wp\/v2\/tags?post=1069"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}