Mark at_thread_exit list as CRT block

[AlexGuteniev]

STL/stl/src/xnotify.cpp

Lines 41 to 45 in c53ac59

	if (block->num_used == _Nitems) { // block is full; move to next block and allocate
	if (block->next == nullptr) {
	block->next = static_cast<_At_thread_exit_block*>(calloc(1, sizeof(_At_thread_exit_block)));
	}

[StephanTLavavej]

I strongly suspect, but haven't confirmed, that these allocations show up as leaks in the CRT debug heap leak tracking machinery.

[Arup-Chauhan]

I wrote a small test program to investigate the memory leak:

#include <crtdbg.h>
#include <cstdlib>
#include <thread>

void at_thread_exit_func() {
    int* simulated_leak = (int*) malloc(sizeof(int) * 100);
}

void thread_func() {
    at_thread_exit_func();
}

int main() {
    _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
    std::thread t1(thread_func);
    std::thread t2(thread_func);
    t1.join();
    t2.join();
    _CrtDumpMemoryLeaks();
    return 0;
}

The test showed a memory leak:

Detected memory leaks!
{178} normal block at 0x0000020BEE9884C0, 400 bytes long.
{177} normal block at 0x0000020BEE989AD0, 400 bytes long.
Object dump complete.

This confirms the leak in the at_thread_exit mechanism.

---

I wanted to know if I have a proper test code.

If it is ok, perhaps we can fix the leak by wrapping the allocations inside _CrtMemCheckpoint at the start and _CrtMemDumpAllObjectsSince at the end of the critical section in the at_thread_exit_func. This will track and free the allocated memory properly when the thread exits.

@StephanTLavavej @CaseyCarter @AlexGuteniev feedback please.

[AlexGuteniev]

I wanted to know if I have a proper test code.

No, it is not.

You need to use at_thread_exit, and see whether it leaks, instead of simulating similar leaks without using that machinery

[Arup-Chauhan]

Hi @AlexGuteniev,

Thank you for your feedback. Based on your suggestion, I have adjusted the approach and used the actual at_thread_exit machinery in xnotify.cpp rather than simulating similar leaks. Below is the modified section of xnotify.cpp where I have integrated memory tracking to detect potential memory leaks:

Modified xnotify.cpp

// Inside xnotify.cpp

while (block != nullptr) { // Loop through list of blocks
    if (block->num_used == _Nitems) { // Block is full; move to next block and allocate
        if (block->next == nullptr) {
            // Start tracking the memory allocation
            _CrtMemState memStateBefore, memStateAfter, memStateDifference;

            // Take memory checkpoint before allocation
            _CrtMemCheckpoint(&memStateBefore);

            block->next = static_cast<_At_thread_exit_block*>(calloc(1, sizeof(_At_thread_exit_block)));

            // Take memory checkpoint after allocation
            _CrtMemCheckpoint(&memStateAfter);

            // Compare memory states to detect memory leaks
            if (_CrtMemDifference(&memStateDifference, &memStateBefore, &memStateAfter)) {
                _CrtMemDumpStatistics(&memStateDifference); // Dump statistics for memory leak
            } else { // Added to suppress the warnings
                (void)memStateBefore;
                (void)memStateAfter;
                (void)memStateDifference;
            }
        }
    }
    block = block->next;
}

Updated Test Code

I’ve also updated the test code to validate these changes, ensuring memory tracking during thread operations.

#include <crtdbg.h>
#include <cstdlib>
#include <thread>

struct _At_thread_exit_block {
    int num_used;
    _At_thread_exit_block* next;
};

// Simulate thread exit machinery
void at_thread_exit_func() {
    // Simulate allocation similar to _At_thread_exit_block usage
    _At_thread_exit_block* block = (_At_thread_exit_block*) calloc(1, sizeof(_At_thread_exit_block));
    if (block) {
        block->num_used = 10; // Simulate some operations
        block->next = nullptr; // Simulate next block being empty
    }
}

void thread_func() {
    at_thread_exit_func();
}

int main() {
    // Enable memory leak detection
    _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);

    // Start threads
    std::thread t1(thread_func);
    std::thread t2(thread_func);

    // Wait for threads to finish
    t1.join();
    t2.join();

    // Check for memory leaks
    _CrtDumpMemoryLeaks();

    return 0;
}

Output

Here’s the output from running the test:

Detected memory leaks!
C:\Open Source\MSVC STL\STL\tests\memory_tests\test_thread_exit.cpp(13) : {178} normal block at 0x0000019477D9BBD0, 16 bytes long.
...
Object dump complete.

Hope the test code is proper now, it detects the memory leak, confirming that the tracking is working as expected. I hope this aligns with the approach.

I am piecing the logic together, hope I am in the right direction.
Looking forward to your thoughts on this!

@CaseyCarter , @StephanTLavavej your inputs are appreciated too :D

[AlexGuteniev]

No, you should test the original unaltered atexit machinery, instead of repeating that code in a test, and also should not alter its code yet.
I'm afraid I'm not able to explain this to you.

[Arup-Chauhan]

No, you should test the original unaltered atexit machinery, instead of repeating that code in a test, and also should not alter its code yet.

Hey @AlexGuteniev, thanks for responding. I’ll continue working on this. I wanted to clarify a few things to ensure I’m approaching the issue correctly:

To investigate the potential memory leak, I initially altered the internal code in xnotify.cpp because the at_thread_exit mechanism is internal to the library, making it inaccessible from an external test case which I added to simulate the behavior of at_thread_exit and incorporate memory leak tracking directly.

Is there a specific technique to expose or simulate this mechanism in an external test scenario while keeping the original code unaltered?

Regarding your mention of atexit, could using it help us understand the behavior of at_thread_exit? For example, could we register cleanup functions with atexit and observe their effects to draw parallels with the deferred cleanup in at_thread_exit, or was it only intended as a point of reference?

I'm afraid I'm not able to explain this to you.

I appreciate your patience and guidance throughout this process; it means a lot. This is a bit new for me, so your pointers are helping a great deal

[AlexGuteniev]

We are looking into _Cnd_register_at_thread_exit function.

It is called from condition_variable::_Register

That one is called via notify_all_at_thread_exit and also from <future> machinery.

std::notify_all_at_thread_exit is a standard function, so that you can call it from your test program directly.
For <future> it is harder to track, but I assume the corresponding standard functions are std::promise::set_value_at_thread_exit and std::promise::set_exception_at_thread_exit. You may need to figure out how to use them, if you have never used them before.

The test program should correctly test these scenarios (all three of them), and report memory leaks. If the program does not have its own leaks, but the leak from at least one of these functions is seen, then this leak exists, and a PR needed to fix and test the fix.

[Arup-Chauhan]

Thanks @AlexGuteniev Alex, I will work on this and get back, once again, thank you for all the help!

[Arup-Chauhan]

Hi @AlexGuteniev, hope you are well, thank you for your patience on this one

I tested the scenarios involving std::notify_all_at_thread_exit, std::promise::set_value_at_thread_exit, and std::promise::set_exception_at_thread_exit as outlined. Here is my code implementation:

#include <condition_variable>
#include <crtdbg.h>
#include <cstdlib>
#include <future>
#include <iostream>
#include <mutex>
#include <thread>

void test_notify_all_at_thread_exit() {
    std::mutex mtx;
    std::unique_lock<std::mutex> lock(mtx);
    std::condition_variable cv;
    std::thread([&] {
        std::notify_all_at_thread_exit(cv, std::move(lock));
    }).join();
}

void test_set_value_at_thread_exit() {
    std::promise<void> promise;
    std::future<void> future = promise.get_future();
    std::thread([&] { promise.set_value_at_thread_exit(); }).join();
    future.wait();
}

void test_set_exception_at_thread_exit() {
    std::promise<void> promise;
    std::future<void> future = promise.get_future();
    std::thread([&] {
        try {
            throw std::runtime_error("Test exception");
        } catch (...) {
            promise.set_exception_at_thread_exit(std::current_exception());
        }
    }).join();
    try {
        future.get();
    } catch (const std::exception& e) {
        std::cerr << "Caught exception: " << e.what() << std::endl;
    }
}

int main() {
    _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);

    test_notify_all_at_thread_exit();
    test_set_value_at_thread_exit();
    test_set_exception_at_thread_exit();

    _CrtDumpMemoryLeaks();

    return 0;
}

When running this code, I am encountering an abort() call. Below is the console output:

'test_thread_exit.exe' (Win32): Loaded 'C:\Open Source\MSVC STL\STL\out\x86\test_thread_exit.exe'. Symbols loaded.
'test_thread_exit.exe' (Win32): Loaded 'C:\Windows\SysWOW64\ntdll.dll'. Symbol loading disabled by Include/Exclude setting.
...
The thread 24160 has exited with code 0 (0x0).
Debug Error!

Program: C:\Open Source\MSVC STL\STL\out\x86\test_thread_exit.exe

abort() has been called

(Press Retry to debug the application)
'test_thread_exit.exe' (Win32): Loaded 'C:\Windows\SysWOW64\user32.dll'. Symbol loading disabled by Include/Exclude setting.
...
Detected memory leaks!
Dumping objects ->
{168} normal block at 0x00AD3100, 8 bytes long.
 Data: <h o   o > 68 FD 6F 00 98 FD 6F 00 
Object dump complete.
The program '[21232] test_thread_exit.exe' has exited with code 3 (0x3).

The code does detect memory leaks when I continue past the abort() call.

However, I'm currently unable to understand the root cause of the abort() function being triggered and how to fix it effectively.
Any guidance on this issue would be greatly appreciated.

@StephanTLavavej , @CaseyCarter it would be great if you can also provide your input as well.

[AlexGuteniev]

However, I'm currently unable to understand the root cause of the abort() function being triggered and how to fix it effectively.

You move the lock into a thread in test_notify_all_at_thread_exit. This attempts to unlock mutex in a different thread from the one where it was locked. This is undefined behavior.

[Arup-Chauhan]

Hi @AlexGuteniev since it is an undefined behaviour and I can see you raised a PR for it, should I wait until this PR work is done?

Please suggest.

[AlexGuteniev]

My PR only improves diagnostic for this problem, you still have to fix your code.

It is also already merged.

[CaseyCarter]

I tested the scenarios involving std::notify_all_at_thread_exit, std::promise::set_value_at_thread_exit, and std::promise::set_exception_at_thread_exit as outlined. Here is my code implementation:

This isn't going to work well. You're setting up the allocations that we want to test on temporary threads. When those temporary threads complete, there will be a bit of extra processing in each case ("at thread exit", as designed) which consumes the allocation. By the time the program gets to _CrtDumpMemoryLeaks() the allocations will have been freed, and so have no chance to show up as leaks. Unless there's a bug in the library and we are actually leaking them, of course =)

I think you need to restructure the program so it creates all necessary objects, and then calls _CrtDumpMemoryLeaks() repeatedly while interleaving each of the three setup steps all on the main thread like:

_CrtDumpMemoryLeaks();
std::cout << "\n\n";
std::notify_all_at_thread_exit(cv, std::move(lock));
_CrtDumpMemoryLeaks();
std::cout << "\n\n";
promise.set_value_at_thread_exit();
_CrtDumpMemoryLeaks();
std::cout << "\n\n";
// ...

Comparing the differences in the output of each call to _CrtDumpMemoryLeaks will indicate which (if any) of the "setup steps" contributes to the reported leaks. It's not an amazing programmatic test to add to the STL's test suite, but running it now should confirm if any of the allocations show up as leaks, and running it with a change in place should tell us if the change corrects the issue. Our target state is for all the _CrtDumpMemoryLeaks calls to produce identical output so none of these allocations managed by the STL confuse user's looking for actual leaks in their programs that use these facilities.

[Arup-Chauhan]

Hello@CaseyCarter and @AlexGuteniev , hope you guys are doing well and the new year is going good. Thanks for the inputs. I was on vacation and came back last month.

I will be restarting work on this and will seek feedback as I move forward, thanks for being helpful so far.