-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C++: Support operator new and operator delete in models library #3173
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
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
b634b59
C++: Merge the two allocators tests.
geoffw0 0cb7d4c
C++: Add an explicit test of AllocationFunction and AllocationExpr.
geoffw0 aa49b35
C++: Add an explicit test of DeallocationFunction and DeallocationExp…
geoffw0 ef68bd6
C++: Add a test of direct calls to operator new / operator dedelete.
geoffw0 259f714
C++: Model operator new and operator new[].
geoffw0 254c877
C++: Deduplicate AllocationExprs.
geoffw0 3b12d1a
C++: Test getPlacementArgument().
geoffw0 18e60fa
C++: Model operator delete and operator delete[].
geoffw0 a75e249
C++: Autoformat test.
geoffw0 aa13257
C++: Correct QLDoc.
geoffw0 f430cf9
C++: Use hasGlobalName.
geoffw0 119d4a4
C++: Fix unintended consequence in IR.
geoffw0 d71098d
Merge branch 'master' into opnew
geoffw0 8d3d088
Merge branch 'master' into opnew
geoffw0 ead5feb
C++: Autoformat.
geoffw0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the overloads where this predicate holds (9 and 10 in https://en.cppreference.com/w/cpp/memory/new/operator_new), I don't think they should be
AllocationFunction
s. They are merely identity functions. The IR alias analysis assumes that an allocation function returns a fresh memory location that doesn't alias anything else reachable, and that's also my own understanding of what an allocation function is.The
requiresDealloc
predicate is currently overridden byalloca
, and that makes sense to me: it returns fresh memory but doesn't require deallocation. In theAllocationExpr
hierarchy,requiresDealloc
is overridden byNewAllocationExpr
andNewArrayAllocationExpr
, and I think the placement versions of those expressions should not beAllocationExpr
s for the same reasons. Unless the allocation classes are used in other queries where we do want to include the placement versions.Functions that always return their parameter can instead be modelled as
AliasFunction
.There may be some C++ standard rules about how it's forbidden to reference an old pointer value that's passed through placement new, which effectively means it behaves like it's allocating fresh memory, but I don't think programmers obey that rule. I also couldn't find the rule on https://en.cppreference.com/w/cpp/language/new, so I may be misremembering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a slightly dubious assumption when you're looking inside the implementation of an
operator new
, which presumably needs to keep hold of some data about the allocation for bookkeeping purposes (I'm not sure exactly how modern non-toy implementations do this). But that data should be well compartmentalized from user code, so I doubt this is something we need to model.Good idea. On balance, I think your distinction is clearer than mine (which is roughly 'Does this thing claim to be an allocator? Then it is one.').
However due to the existing behaviour in
AllocationExpr
(which is fairly widely used), I'd rather address this concern in a follow-up PR and give it the care and attention it deserves.It seems like you should be able to keep a reference for memory management purposes, e.g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.
That sounds right.