Skip to content

C++: Emit InitializeDynamicAllocation instructions for NewExpr and NewArrayExpr #3171

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

Merged
merged 11 commits into from
Apr 2, 2020

Conversation

MathiasVP
Copy link
Contributor

The gist of the PR is pretty simple: #2797 only added support for the new InitializeDynamicAllocation instruction for calls to malloc style allocation, but did not do so for new or new[]. This PR fixes this.

The implementation ended up being quite a lot of refactoring because the class emitting the InitializeDynamicAllocation instruction was attached to an AST Call object, which new is not part of. So I refactored TranslatedSideEffects into an allocation side effect translation (TranslatedAllocationSideEffects) and call side effect translation (TranslatedCallSideEffects).

@MathiasVP MathiasVP added the C++ label Mar 31, 2020
@MathiasVP MathiasVP requested a review from rdmarsh2 March 31, 2020 12:02
@MathiasVP MathiasVP requested a review from a team as a code owner March 31, 2020 12:02
@MathiasVP MathiasVP mentioned this pull request Mar 31, 2020
@jbj
Copy link
Contributor

jbj commented Mar 31, 2020

I'd like to understand how this PR overlaps with or complements the forthcoming PR from @geoffw0 where operator new is modelled as an AllocationFunction. Let me know your perspective on that when Geoffrey's PR is up.

@jbj
Copy link
Contributor

jbj commented Mar 31, 2020

Geoffrey's PR is #3173

@geoffw0
Copy link
Contributor

geoffw0 commented Mar 31, 2020

My PR is #3173. Happy to make any changes that will make things easier here.

# 960| r960_2(unsigned long) = Constant[40] :
# 960| r960_3(void *) = Call : func:r960_1, 0:r960_2
# 960| mu960_4(unknown) = ^CallSideEffect : ~mu959_4
# 960| mu960_5(unknown) = ^InitializeDynamicAllocation : &:r960_3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why whitespace changed for this output. I recommend hiding whitespace changes when reviewing these changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

The whitespace between the result+type and the = is based on the longest result+type, which got longer by moving from r966_9 to r966_10

@MathiasVP
Copy link
Contributor Author

Currently, this PR generates incorrect IR for placement news. I'm looking at a fix for this now.

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Apr 1, 2020

Currently, this PR generates incorrect IR for placement news. I'm looking at a fix for this now.

It looks like the IR generation problems for placement new is also present on master, so I'll leave my PR as it is. I've opened https://github.com/github/codeql-c-analysis-team/issues/43 for it.

Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

One comment. Also, there's now a conflict with Geoffrey's PR. When resolving that, will you now be able to revert 119d4a4?

@MathiasVP
Copy link
Contributor Author

One comment. Also, there's now a conflict with Geoffrey's PR. When resolving that, will you now be able to revert 119d4a4?

Yes. The resolution is to revert it, which I have done locally. I'm running the library-tests now to confirm that it's the correct thing to do.

@jbj
Copy link
Contributor

jbj commented Apr 2, 2020

This is probably going to give us some CPP-Differences results to look at, so let's bump the submodule pointer tomorrow, as soon as we've got an email with the nightly CPP-Differences caused by #3173.

@jbj jbj merged commit 604731b into github:master Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants