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

[Cpp] Fix cpp memory leaks #4617

Merged
merged 1 commit into from
May 31, 2024
Merged

[Cpp] Fix cpp memory leaks #4617

merged 1 commit into from
May 31, 2024

Conversation

njnobles
Copy link

@njnobles njnobles commented May 15, 2024

This PR fixes memory leaks caused by the StaticData objects associated with the generated Parser and Lexer classes.

Found using the CRT library in VS2022. With the demo project, it produces a leak report similar to this:

Detected memory leaks!
Dumping objects ->
{21126} normal block at 0x000001AAB1D11180, 32 bytes long.
 Data: <                > 00 95 CF B1 AA 01 00 00 A0 9A CF B1 AA 01 00 00 
{20999} normal block at 0x000001AAB1D141E0, 32 bytes long.
 Data: <`       `       > 60 1F D1 B1 AA 01 00 00 60 1F D1 B1 AA 01 00 00 
{20982} normal block at 0x000001AAB1D134C0, 32 bytes long.
 Data: <         1      > C0 A8 CD B1 AA 01 00 00 C0 31 D1 B1 AA 01 00 00 
{20981} normal block at 0x000001AAB1D20DF0, 128 bytes long.
 Data: <                > 00 19 D1 B1 AA 01 00 00 00 19 D1 B1 AA 01 00 00 
...<~7000 lines long>

Valgrind on Ubuntu produces a similarly large leak detection.

Switching the StaticData objects to be unique_ptr instead of raw pointers removes all but 1 leak.
The remaining leak is the ATNDeserializationOptions which is also fixed by a unique_ptr.

Signed-off-by: Nick Nobles <[email protected]>
@njnobles
Copy link
Author

Hi @parrt, would it be possible to get someone to review this PR? Thank you!

@parrt
Copy link
Member

parrt commented May 30, 2024

Unfortunately I'm not qualified to evaluate this :( Maybe @mike-lischke ? @jcking ?

@parrt parrt merged commit 7d4cea9 into antlr:dev May 31, 2024
42 checks passed
@parrt
Copy link
Member

parrt commented May 31, 2024

Thanks, @mike-lischke !

@njnobles
Copy link
Author

Thank you, @parrt and @mike-lischke!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants