Add IsAllocator and IsDeallocator API functions#1020
Conversation
@Vipul-Cariappa this PR is a prototype, I just searched for most obvious attributes there are more attributes should be added, please provide me feedback. Also it seems not to be possible to detect allocation-deallocation type via attributes. So it may not to be a good idea to build query functions based on attribute searching |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1020 +/- ##
==========================================
+ Coverage 86.62% 86.71% +0.08%
==========================================
Files 23 23
Lines 5946 5977 +31
==========================================
+ Hits 5151 5183 +32
+ Misses 795 794 -1
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
3b0f128 to
4b90462
Compare
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
This looks good. Nice job. Some minor comments.
| assert(Fn && "Not func"); | ||
| auto* D = static_cast<const Decl*>(Fn); | ||
| assert(D && "Not func"); | ||
| auto* FD = dyn_cast<FunctionDecl>(D); | ||
| assert(FD && "Not func"); |
There was a problem hiding this comment.
We don't want to crash the application if Fn = nullptr, instead if return a sensible default. In this case, false. You should remove the the asserts.
Asserts are used to detect bugs within the CppInterOp codebase itself. In this case, it is the user providing a null value.
| assert(Fn && "Not func"); | ||
| auto* D = static_cast<const Decl*>(Fn); | ||
| assert(D && "Not func"); | ||
| auto* FD = dyn_cast<FunctionDecl>(D); | ||
| assert(FD && "Not func"); |
There was a problem hiding this comment.
Please remove the asserts. As explained above.
| enum class AllocType : unsigned char { | ||
| New, | ||
| NewArr, | ||
| Malloc, | ||
| None, | ||
| Unknown // Could not detect type, for now | ||
| }; | ||
|
|
||
| enum class DeallocType : unsigned char { | ||
| Delete, | ||
| DeleteArr, | ||
| Free, | ||
| None, | ||
| Unknown // Could not detect type, for now | ||
| }; |
There was a problem hiding this comment.
These are not yet used. So let's remove them from this PR. But this looks good. We should model based on this. Are there attributes that differentiate between malloc and new?
Also looking at https://clang.llvm.org/docs/AttributeReference.html#ownership-holds-ownership-returns-ownership-takes-clang-static-analyzer. There is ownership_holds. I believe the DeallocType should also express that.
There was a problem hiding this comment.
Yeah this is a problem we can not differentiate allocation-deallocation types from attributes. I will change the PR by your suggestions next week.
There was a problem hiding this comment.
These are not yet used. So let's remove them from this PR. But this looks good. We should model based on this. Are there attributes that differentiate between
mallocandnew? Also looking at https://clang.llvm.org/docs/AttributeReference.html#ownership-holds-ownership-returns-ownership-takes-clang-static-analyzer. There isownership_holds. I believe theDeallocTypeshould also express that.
I did not handle OwnershipHolds in current commit but this can be another ownership category, probably cppyy will handle this same as a OwnershipTakes but it may be better to distinguish them. What do you think
|
Please consider rebasing the PR before continuing the work. |
| return INTEROP_RETURN(false); | ||
| } | ||
|
|
||
| bool IsDeallocator(TCppConstFunction_t Fn) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I added for malloc and free, we may extend it for realloc etc. later
34e247f to
e11ab32
Compare
| if (Fn) { | ||
| if (auto* D = unwrap<clang::Decl>(Fn)) { | ||
| if (auto* FD = dyn_cast<FunctionDecl>(D)) { | ||
| if (FD->getBuiltinID() == Builtin::ID::BImalloc) |
There was a problem hiding this comment.
warning: no header providing "clang::Builtin::ID" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOp.cpp:15:
+ #include <clang/Basic/Builtins.h>6968612 to
b4e1fd8
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@vgvassilev @aaronj0 @Vipul-Cariappa Hello all, can you review the PR, it is getting larger and harder to debug for me |
| @@ -0,0 +1,7 @@ | |||
| #ifndef UNITTESTS_CPPINTEROP_APINOTES_TESTHEADER_H | |||
| #define UNITTESTS_CPPINTEROP_APINOTES_TESTHEADER_H | |||
There was a problem hiding this comment.
warning: header guard does not follow preferred style [llvm-header-guard]
| #define UNITTESTS_CPPINTEROP_APINOTES_TESTHEADER_H | |
| #ifndef GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_APINOTES_TESTHEADER_H | |
| #define GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_APINOTES_TESTHEADER_H |
| if (FDA->getSemanticSpelling() != RestrictAttr::Declspec_restrict) | ||
| return INTEROP_RETURN(true); |
There was a problem hiding this comment.
| if (FDA->getSemanticSpelling() != RestrictAttr::Declspec_restrict) | |
| return INTEROP_RETURN(true); | |
| return INTEROP_RETURN(true); |
Shouldn't this be the following?
I am looking at the definition of RestrictAttr::getSemanticSpelling.
There was a problem hiding this comment.
https://learn.microsoft.com/en-us/cpp/cpp/restrict?view=msvc-170
The definition of declspec(restrict) is kinda different from gnu's restrict. It implies pointer returned by the function has no alias, so it is not a neccesity for function to allocate memory.
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
LGTM! Just a small comment. You also need to rebase, so that I can merge.
| // FIXME: These attributes are not currently compatible with our memory | ||
| // representation, they are added for APINotes' test |
There was a problem hiding this comment.
| // FIXME: These attributes are not currently compatible with our memory | |
| // representation, they are added for APINotes' test |
Note required here. This FIXME should be part of the other PR.
Adds enum classes for allocation and deallocation types, creates API functions for queries.