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.

Sign up for GitHub

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

Jump to bottom

Release pointers when we run out of memory in pewriter #50243

Closed
wants to merge 1 commit into from
Closed

Release pointers when we run out of memory in pewriter #50243

wants to merge 1 commit into from

Conversation

omajid
Copy link
Member

@omajid omajid commented Mar 25, 2021

No description provided.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

omajid
omajid commented Mar 25, 2021
View reviewed changes
src/coreclr/dlls/mscorpe/pewriter.cpp
else
{
delete[] TokInSymbolTable;
return E_OUTOFMEMORY;
Copy link
Member Author

Choose a reason for hiding this comment

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

We have also allocated reloc and strtab. Should we delete and set them to null too?

Copy link
Member

Choose a reason for hiding this comment

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

Is this method and other stuff related to fixups in this directory reachable code?

It all looks related to EMIT_FIXUPS that is never defined. It would be better to delete it all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this method and other stuff related to fixups in this directory reachable code?

This memory leak was flagged by a static analyzer. I am not sure it can tell if the code is reachable or not.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please try to delete this method, all code under EMIT_FIXUPS ifdefs, and see whether it will compile fine (maybe with a few other unrechable pieces deleted)?

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 tried removing #ifdef EMIT_FIXUPS (and the code they guard) completely, but this CeeFileGenWriter::fixup fails to build looking for this (now undefined) method:

runtime/src/coreclr/dlls/mscorpe/ceefilegenwriter.cpp

Line 518 in 400311b

IfFailRet(getPEWriter().fixup(pMapper));

That call is not guarded by an #ifdef EMIT_FIXUPS...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it was a bit less straightforward than I hoped. I have submitted #50259

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I am going to close this issue in favour of #50259 then

Copy link
Member

Choose a reason for hiding this comment

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

I think a few of the places you have touched will be still around even after my PR is merged. We can keep this one open, and then you can just resolve the conflict.

@omajid
Copy link
Member Author

omajid commented Mar 25, 2021

Should I try and use smart pointers here instead of these manually managed pointers that we sometimes forget to clean up?

@omajid omajid closed this Mar 25, 2021
@jkotas
Copy link
Member

jkotas commented Mar 25, 2021

Yes, we preffer to use the smart pointers.

@omajid omajid reopened this Mar 25, 2021
@jkotas
Copy link
Member

jkotas commented Mar 27, 2021

@omajid Could you please resolve the conflict?

@jeffhandley
Copy link
Member

Hi @omajid - Let us know if you'd like help resolving the conflicts on this PR so we can get it merged. Thanks!

@jeffhandley
Copy link
Member

I'm going to mark this as a draft until the conflicts are resolved. @omajid, let us know if you'd like some help getting them resolved. Thanks!

@jeffhandley jeffhandley marked this pull request as draft May 28, 2021 21:55
@ghost ghost closed this Jun 27, 2021
@ghost
Copy link

ghost commented Jun 27, 2021

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 27, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

Assignees
No one assigned
Labels
area-System.Reflection.Emit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@omajid @jkotas @jeffhandley