| Message ID | 20241111-b4-pks-leak-fixes-pt10-v2-21-6154bf91f0b0@pks.im (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Memory leak fixes (pt.10, final) | expand |
On Mon, Nov 11, 2024 at 11:38:50AM +0100, Patrick Steinhardt wrote: > There are two users of `UNLEAK()` left in our codebase: > > - In "builtin/clone.c", annotating the `repo` variable. That leak has > already been fixed though as you can see in the context, where we do > know to free `repo_to_free`. > > - In "builtin/diff.c", to unleak entries of the `blob[]` array. That > leak has also been fixed, because the entries we assign to that > array come from `rev.pending.objects`, and we do eventually release > `rev`. Yay, as the author of UNLEAK() I'm in favor of getting rid of it. > This neatly demonstrates one of the issues with `UNLEAK()`: it is quite > easy for the annotation to become stale. A second issue is that its > whole intent is to paper over leaks. And while that has been a necessary > evil in the past, because Git was leaking left and right, it isn't > really much of an issue nowadays where our test suite has no known leaks > anymore. I do agree that stale annotations are a weakness (they do not hurt the leak-checker if they exist, but they are an eye-sore). I'm not sure I would agree that the intent was to paper over leaks. The point was to avoid noise from the leak-checker about memory that was intentionally held until program exit and released by returning from main(). I think the main thing that made it obsolete is that we decided it was worth it to spend the cycles freeing that memory rather than ignoring it. But it's possible I'm just splitting hairs. :) At any rate, for whatever reason people did not seem to use it as intended, and _did_ end up annotating real leaks (papering over them). I guess I could have done a better job of explaining it. -Peff
On Tue, Nov 12, 2024 at 03:26:09AM -0500, Jeff King wrote: > On Mon, Nov 11, 2024 at 11:38:50AM +0100, Patrick Steinhardt wrote: > > This neatly demonstrates one of the issues with `UNLEAK()`: it is quite > > easy for the annotation to become stale. A second issue is that its > > whole intent is to paper over leaks. And while that has been a necessary > > evil in the past, because Git was leaking left and right, it isn't > > really much of an issue nowadays where our test suite has no known leaks > > anymore. > > I do agree that stale annotations are a weakness (they do not hurt the > leak-checker if they exist, but they are an eye-sore). > > I'm not sure I would agree that the intent was to paper over leaks. The > point was to avoid noise from the leak-checker about memory that was > intentionally held until program exit and released by returning from > main(). I think the main thing that made it obsolete is that we decided > it was worth it to spend the cycles freeing that memory rather than > ignoring it. > > But it's possible I'm just splitting hairs. :) Yeah, I know that this was also used to mark memory that intentionally leaks because we're about to exit anyway. I basically consider that as some form of "papering over" it, but I get your comment that this may be a bit too strongly worded. Do you want me to reformulate this, or do we just go with the current description? Patrick
On Tue, Nov 12, 2024 at 09:53:28AM +0100, Patrick Steinhardt wrote: > On Tue, Nov 12, 2024 at 03:26:09AM -0500, Jeff King wrote: > > On Mon, Nov 11, 2024 at 11:38:50AM +0100, Patrick Steinhardt wrote: > > > This neatly demonstrates one of the issues with `UNLEAK()`: it is quite > > > easy for the annotation to become stale. A second issue is that its > > > whole intent is to paper over leaks. And while that has been a necessary > > > evil in the past, because Git was leaking left and right, it isn't > > > really much of an issue nowadays where our test suite has no known leaks > > > anymore. > > > > I do agree that stale annotations are a weakness (they do not hurt the > > leak-checker if they exist, but they are an eye-sore). > > > > I'm not sure I would agree that the intent was to paper over leaks. The > > point was to avoid noise from the leak-checker about memory that was > > intentionally held until program exit and released by returning from > > main(). I think the main thing that made it obsolete is that we decided > > it was worth it to spend the cycles freeing that memory rather than > > ignoring it. > > > > But it's possible I'm just splitting hairs. :) > > Yeah, I know that this was also used to mark memory that intentionally > leaks because we're about to exit anyway. I basically consider that as > some form of "papering over" it, but I get your comment that this may be > a bit too strongly worded. > > Do you want me to reformulate this, or do we just go with the current > description? Nah, I think it is OK to leave it as-is. The important thing is that it is gone. :) (Thanks for all your work on this, btw. I was so happy to see the commit dropping all of the PASSING_SANITIZE_LEAK infrastructure). -Peff
diff --git a/builtin/clone.c b/builtin/clone.c index 59fcb317a68a77eee3ca96a60720c556e044c369..e851b1475d7be8f0af78c27d4ef15467a2769a74 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1586,7 +1586,6 @@ int cmd_clone(int argc, free(dir); free(path); free(repo_to_free); - UNLEAK(repo); junk_mode = JUNK_LEAVE_ALL; transport_ls_refs_options_release(&transport_ls_refs_options); diff --git a/builtin/diff.c b/builtin/diff.c index dca52d4221ed19d27950bee9022ae7efb1d2f17a..2fe92f373e9991489fcaeeba381bbfe017a5316a 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -628,6 +628,5 @@ int cmd_diff(int argc, release_revisions(&rev); object_array_clear(&ent); symdiff_release(&sdiff); - UNLEAK(blob); return result; }
There are two users of `UNLEAK()` left in our codebase: - In "builtin/clone.c", annotating the `repo` variable. That leak has already been fixed though as you can see in the context, where we do know to free `repo_to_free`. - In "builtin/diff.c", to unleak entries of the `blob[]` array. That leak has also been fixed, because the entries we assign to that array come from `rev.pending.objects`, and we do eventually release `rev`. This neatly demonstrates one of the issues with `UNLEAK()`: it is quite easy for the annotation to become stale. A second issue is that its whole intent is to paper over leaks. And while that has been a necessary evil in the past, because Git was leaking left and right, it isn't really much of an issue nowadays where our test suite has no known leaks anymore. Remove the last two users of this macro. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/clone.c | 1 - builtin/diff.c | 1 - 2 files changed, 2 deletions(-)