fix: wrap std::thread to join in destructor in btop_collect.cpp

[omrash]

These changes prevent the IOT instruction core dump from NVML after waking up from suspend, specifically nvmlDeviceGetPcieThroughput(). This resolves #1050.

[omrash]

Apparently clang is lagging behind in their implementation of this C++20 language feature (you may need to enable the fexperimental-library flag). Would you prefer I stick with std::thread and wrap the thread function block in a try-catch instead?

[deckstose]

Would you prefer I stick with std::thread and wrap the thread function block in a try-catch instead?

Yes.

[omrash]

Would you prefer I stick with std::thread and wrap the thread function block in a try-catch instead?

Yes.

Just tested it and a try-catch doesn't work since the thread still terminates and doesn't join. You'd probably need to have a class to wrap std::thread to force it to join in it's destructor, which is basically reimplementing std::jthread. If that's the only way to fix it I'll do it but I think it makes more sense to use an existing implementation in the standard library.

[deckstose]

Isn't there another way to get cooperative cancellation of the threads?

[omrash]

Isn't there another way to get cooperative cancellation of the threads?

The only alternatives I can think of are less than ideal, like detaching the threads and using some kind of condition variable to signal the main thread. This is the cleanest solution I could come up with but I'm open to any ideas!

[deckstose]

Okay, it seems they have put this behind an experimental flag in LLVM 18 to reserve the possibility to change implementation details. However, they have not done this. So the implementation should be the same regardless of the experimental flag.

I'm fine with using std::jthread, but it would be great to limit whatever -fexperimental-library enables to jthread only.

This is relevant for LLVM 19 only, am I right?

[deckstose]

You should add a comment in the Makefile that the flag is only relevant for LLVM 19. I don't expect a full version check there, this would be madness, but this way it's easier to mark this line obsolete once we drop LLVM 19 support.

[omrash]

Okay, it seems they have put this behind an experimental flag in LLVM 18 to reserve the possibility to change implementation details. However, they have not done this. So the implementation should be the same regardless of the experimental flag.

I'm fine with using std::jthread, but it would be great to limit whatever -fexperimental-library enables to jthread only.

This is relevant for LLVM 19 only, am I right?

Yes, Clang 20 and up seem to build fine without it. std::jthread is implemented in <thread> so we're not pulling in any new includes.

[omrash]

@deckstose This has been sitting for a while, are there any other changes that need to be made?

[omrash]

@aristocratos @deckstose Could this fix please be merged in if nothing is blocking it?

[deckstose]

I'm fine with using std::jthread, but it would be great to limit whatever -fexperimental-library enables to jthread only.

[omrash]

I'm fine with using std::jthread, but it would be great to limit whatever -fexperimental-library enables to jthread only.

There's no real practical way to achieve this, but I can understand the apprehension in enabling the flag wholesale. I've opted to just wrap the threads in a struct that mimics std::jthread's join behavior instead, and can confirm that it still fixes the issue from local testing.