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

[LLVM][DWARF] Fix for memory leak #81828

Merged
merged 1 commit into from
Feb 15, 2024
Merged

[LLVM][DWARF] Fix for memory leak #81828

merged 1 commit into from
Feb 15, 2024

Conversation

ayermolo
Copy link
Contributor

This is followup to #8120. Missed a
destuctor.

This is followup to llvm#8120. Missed a
destuctor.
@ayermolo ayermolo changed the title y [LLVM][DWARF] Fix for memory leak Feb 15, 2024
~Dwarf5AccelTableWriter() {
for (DebugNamesAbbrev *Abbrev : AbbreviationsVector)
Abbrev->~DebugNamesAbbrev();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just remove the allocator instead and use plain old unique pointers?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I'm not familiar with how the abbrev stuff is implemented, so can't speak as to how much time this allocator is saving)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am going to accept and land to fix the bot.
Please followup with @felipepiovezan .

BTW. std::destroy should work here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can.... I don't really have a strong opinion on this.
It will look something like

std::unique_ptr<DebugNamesAbbrev> NewAbbrev =
            std::make_unique<DebugNamesAbbrev>(std::move(Abbrev));
        NewAbbrev->setNumber(AbbreviationsVector.size() + 1);
        AbbreviationsVector.push_back(std::move(NewAbbrev));
        AbbreviationsSet.InsertNode(AbbreviationsVector.back().get(), InsertPos);
        Value->setAbbrevNumber(AbbreviationsVector.back()->getNumber());

Couple reasons for leaving it like this

  1. Bumpptr allocator seems to be a more standard way of dong things, although it can lead to situations like this.
  2. This is also how .debug_abbrev handles it. Since we are going for convergence at some point.

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

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

Happy with this for now, similar to the debug_info abbrevs - be good to converge the code futher at some point, and see if there's way to do this work better, perhaps.

@vitalybuka vitalybuka merged commit f905877 into llvm:main Feb 15, 2024
5 checks passed
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.

None yet

4 participants