Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<syncstream>: fix std::osyncstream memory leak #2763

Merged
merged 7 commits into from
Jun 12, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Jun 6, 2022

Fixes #2760

@fsb4000 fsb4000 requested a review from a team as a code owner June 6, 2022 15:48
@fsb4000
Copy link
Contributor Author

fsb4000 commented Jun 6, 2022

https://docs.microsoft.com/en-us/visualstudio/debugger/finding-memory-leaks-using-the-crt-library?view=vs-2022#enable-memory-leak-detection

To enable all the debug heap functions, include the following statements in your C++ program, in the following order:

#define _CRTDBG_MAP_ALLOC
#include <stdlib.h>
#include <crtdbg.h>

It worked OK with clang-format order but maybe the order is important.

@CaseyCarter CaseyCarter added the bug Something isn't working label Jun 6, 2022
stl/inc/syncstream Outdated Show resolved Hide resolved
tests/std/tests/GH_002760_syncstream_memory_leak/env.lst Outdated Show resolved Hide resolved
tests/std/tests/GH_002760_syncstream_memory_leak/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_002760_syncstream_memory_leak/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_002760_syncstream_memory_leak/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_002760_syncstream_memory_leak/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I pushed significant test changes, verifying them in three ways:

  1. If I comment out the product code fix, the test fails with:

    Assertion failed: _CrtMemDifference(&diff, &start, &end) == 0, file D:\GitHub\STL\tests\std\tests\GH_002760_syncstream_memory_leak\test.cpp, line 25
    abort() has been called--
    

    This demonstrates that the leak checks are still working.

  2. Without the REQUIRES comment, 17 configurations pass. This demonstrates that the internal test harness will pass. [[maybe_unused]] was necessary to avoid release mode complaining about the variables.

  3. With the REQUIRES comment, 8 configurations are unsupported while 9 pass. The pattern of UNSUPPORTED and PASS configurations exactly corresponds to the release and debug pattern in the matrix.

stl/inc/syncstream Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I pushed a product code change for tidying consistently - the tests pass, and I'm reasonably confident it's correct, but lower confidence than usual since iostreams is my magical weak point. Please double check! 😸

stl/inc/syncstream Show resolved Hide resolved
stl/inc/syncstream Show resolved Hide resolved
@strega-nil-ms
Copy link
Contributor

Thanks!

@strega-nil-ms strega-nil-ms removed their assignment Jun 9, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jun 10, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 99cdfa8 into microsoft:main Jun 12, 2022
@StephanTLavavej
Copy link
Member

Thanks for finding the source of this memory leak and fixing it! 💧 🔧 😻

fsb4000 added a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<syncstream>: std::osyncstream memory leak
5 participants