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

Fix memory leak in Java runfiles tests #220

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

fmeum
Copy link
Member

@fmeum fmeum commented Nov 24, 2022

No description provided.

}

extern "C" void LLVMFuzzerInitialize(int *argc, char ***argv) {
std::string error;
runfiles = Runfiles::Create((*argv)[0], &error);
runfiles.reset(Runfiles::Create((*argv)[0], &error));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're trying to follow the Google C++ style guide, which prohibits non-trivially destructible globals: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

How about declaring the runfiles global static? This should prevent the leak detector from firing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the test again on master, leak detection only (rightfully) complains about the Java test. I thus dropped the change to runfiles_fuzz_test.cc from this PR.

@fmeum fmeum force-pushed the fix-runfiles-memory-leak branch from d4354f6 to 2704df5 Compare December 5, 2022 16:06
@fmeum fmeum changed the title Fix memory leak in runfiles tests Fix memory leak in Java runfiles tests Dec 5, 2022
@fmeum fmeum requested a review from stefanbucur December 5, 2022 16:07
@stefanbucur stefanbucur merged commit 22a866a into bazel-contrib:master Dec 5, 2022
@fmeum fmeum deleted the fix-runfiles-memory-leak branch December 5, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants