Segmentation fault because of std:: thread launching through a method

Question

#include <thread>
#include <SDL2/SDL.h>
#include <SDL2/SDL_image.h>
void pool_executor()
{
    return;
}
class nn_thread
{
    std::thread *threads;

  public:
    nn_thread(int thread_ct)
    {
        threads = (std::thread *)malloc(sizeof(std::thread) * thread_ct);
        threads[1] = std::thread(pool_executor);
    }
    ~nn_thread()
    {
        threads[1].join();
        free(threads);
    }
};
int main(int argc, char *argv[])
{
    nn_thread model_threads(4);
    return 0;
}

As soon as i add this graphic library SDL2, my program starts throwing Segmentation fault. Can someone explain me the reason?

I tried searching if SDL2 has some restrictions for multi-threading or something but i didn't find any.

Generally, any time you need to use a C-style cast (like you do here with the pointer that `malloc` returns) you should take that as a sign you're probably doing something wrong. Yes `malloc` allocates memory, but it doesn't initialize it. The objects that `threads` points to are not constructed. Your assignment in the next line will lead to *undefined behavior*. — Some programmer dude, Jul 23 '23 at 16:17
If you need a dynamic array, the first choice should be `std::vector`; The second choice should be `std::unique_ptr`. The third choice is using `new[]`. As a beginner you should never use anything but a `std::vector` for all your dynamic array needs. — Some programmer dude, Jul 23 '23 at 16:19
If you are going to say "but it is constructed, I called the constructor!" Look carefully at what `threads[1] = std::thread(pool_executor);` does: it constructs a new temporary `std::thread` (that part is fine) and then calls `operator=` on the `threads[1]` object, which itself has not been constructed. Also note that `threads[1]` is never destructed either. — Nate Eldredge, Jul 23 '23 at 17:29
C-style cast - no. `malloc` to allocate storage for C++ objects - No, *no*, *Hell NO*. — Jesper Juhl, Jul 23 '23 at 18:26
Do std::thread threads [10]; works same as malloc? Do i just allocate memory or also calls the constructor like the new keyword — Suryansh Dey, Jul 25 '23 at 11:01
@Nate Eldredge why threads[1] won't destruct? I have called join() on threads[1] so after my nn_thread class is destructed, threads [1] should also destruct, isn't it? By the way can you tell how to construct threads [1] explicitly after i have allocated using malloc since i dont want to creat a new object then copy it into thread [1]. — Suryansh Dey, Jul 25 '23 at 11:19
@NateEldredge in this tutorial https://www.javatpoint.com/initialise-an-array-of-objects-with-parameterised-constructors-in-cpp he also did the same thing with malloc but didn't said it will give undefined behaviour or anything. — Suryansh Dey, Jul 25 '23 at 12:12
No, it won't destruct. The compiler will automatically call the destructors of your member objects. But your member is not a `std::thread`, it's a `std::thread *`, and the destructor of a pointer doesn't do anything - in particular it doesn't destruct the object pointed to. (The compiler can't know how many valid objects the pointer points to, or if the pointer would be valid to dereference at all.) — Nate Eldredge, Jul 26 '23 at 01:27
The question linked as duplicate contains many explanations of why you should not use `malloc` here in the first place. I suggest reading them carefully. If you still insist on using it, they also explain how to do it properly with placement new or `construct_at`. — Nate Eldredge, Jul 26 '23 at 01:28
In the javatpoint tutorial, they get away with it because `class Test` has the [default assignment operator](https://stackoverflow.com/questions/5096464/default-assignment-operator-in-c-is-a-shallow-copy) and all its members are [PODS](https://stackoverflow.com/questions/146452/what-are-pod-types-in-c). Those are not true of `std::thread`. Even in the `class Test` example it might still technically be UB, I'm not sure. It's certainly bad practice for the reasons already explained. You can't trust everything you read on the Internet! — Nate Eldredge, Jul 26 '23 at 01:33

Superlokkus · Accepted Answer · 2023-07-23T18:19:58.937

1

To not bear you to much confusion, I like to give this more basic solution to you:

#include <thread>
#include <vector>

void pool_executor()
{
    return;
}
class nn_thread
{
    std::vector<std::thread> threads;

  public:
    nn_thread(int thread_ct)
    {
        for (int i = 0; i < thread_ct; ++i) {
            threads.emplace_back(&pool_executor);
        }
    }
    ~nn_thread()
    {
        for (auto& thread : threads) {
            thread.join();
        }
    }
};
int main(int argc, char *argv[])
{
    nn_thread model_threads(4);
}

In C++ you don't have to use malloc, even when dealing with the SDL2. Containers are your friends as you can see, one could even replace that vector with an std::array but then I would have to template nn_thread. So I hope this more basic code helps you.

edited Jul 23 '23 at 18:19

answered Jul 23 '23 at 17:37

Superlokkus

4,731
1
25
57

There's no need to test for `joinable()`; all of the threads that the class creates are joinable. And there's no need for the mental overhead of `for_each` and that lambda. Write the loop explicitly, just like in the constructor; having them look the same makes it much easier to see what the code is doing. (I won't recommend rewriting the constructor will an algorithm, to look more like that destructor; that's an area where the amount of code you have to write is counterproductive) – Pete Becker Jul 23 '23 at 17:50
@PeteBecker Well my intention was to show a bit what things C++ offers, but I guess you right, it overloads a beginner, so I guess a for and a range for are already enough new things additional with the rest. The test was to make the code more robust if hey would change it, but yeah right now not needed. – Superlokkus Jul 23 '23 at 18:19
Problem solved, thanks to chat GPT, it's all just because of using malloc for declaring an array of object, std::thread. Just replaced malloc with new and free to delete and it worked! – Suryansh Dey Jul 24 '23 at 00:59
@SuryanshDey That's the difference the marked duplicate Question/Answer addresses. – Superlokkus Jul 24 '23 at 11:23
Why did u pass &pool_executor while std::thread constructor takes just pool_executor? – Suryansh Dey Jul 25 '23 at 11:20
@Superlokkus no one is that beginner here, every one knows about joinable and all – Suryansh Dey Jul 25 '23 at 11:34
@SuryanshDey Its a habit of me to always add the explicit `&` i.e. address of operator when using any function like that, since in some cases you need it, and quite recently I now know why and when: https://stackoverflow.com/q/76777630/3537677 – Superlokkus Jul 27 '23 at 16:40

Segmentation fault because of std:: thread launching through a method

1 Answers1