IsAllocator redesigned and intercommunicating with GetAllocType - #1072
IsAllocator redesigned and intercommunicating with GetAllocType#1072keremsahn wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1072 +/- ##
==========================================
+ Coverage 87.74% 87.80% +0.06%
==========================================
Files 23 23
Lines 6429 6454 +25
==========================================
+ Hits 5641 5667 +26
+ Misses 788 787 -1
🚀 New features to boost your workflow:
|
|
|
||
| #include <optional> | ||
|
|
||
| namespace Cpp { |
There was a problem hiding this comment.
warning: included header optional is not used directly [misc-include-cleaner]
| namespace Cpp { | |
| usplus | |
| t |
| } | ||
|
|
||
| bool IsAllocator(ConstFuncRef Fn) { | ||
| std::optional<AllocType> IsAllocator(ConstFuncRef Fn) { |
There was a problem hiding this comment.
warning: no header providing "Cpp::AllocType" is directly included [misc-include-cleaner]
tr);
^| return RecursiveASTVisitor::TraverseDecl(D); | ||
| } | ||
|
|
||
| std::optional<AllocType> join(std::optional<AllocType> a, |
There was a problem hiding this comment.
warning: method 'join' can be made static [readability-convert-member-functions-to-static]
| std::optional<AllocType> join(std::optional<AllocType> a, | |
| );static |
| !COCE->isInfixBinaryOp()) | ||
| return handleExpr(COCE->getArg(0)); | ||
| } | ||
| return handleCall(const_cast<CallExpr*>(CE)); |
There was a problem hiding this comment.
warning: do not use const_cast to remove const qualifier [cppcoreguidelines-pro-type-const-cast]
}
^| @@ -0,0 +1,13 @@ | |||
| #ifndef UNITTESTS_CPPINTEROP_TESTATTRIBUTEMERGE_H | |||
| #define UNITTESTS_CPPINTEROP_TESTATTRIBUTEMERGE_H | |||
There was a problem hiding this comment.
warning: header guard does not follow preferred style [llvm-header-guard]
| #define UNITTESTS_CPPINTEROP_TESTATTRIBUTEMERGE_H | |
| #ifndef GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_APINOTES_TESTATTRIBUTEMERGE_H | |
| #define GITHUB_WORKSPACE_UNITTESTS_CPPINTEROP_APINOTES_TESTATTRIBUTEMERGE_H |
8f98ada to
fc0e7da
Compare
|
I found an important issue while developing recursion support in deallocation. In this PR attribute injection is done in static function not in the API itself, but in recursive cases function in cache can be evaluated wrong even though the main function is always evaluated true, and the false evaluation is injected to the recursively analyzed functions. Therefore attribute injection mechanism should be moved to API function, which is not called in recursive cases |
d5e66ef to
98115cd
Compare
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
Initial quick review.
This looks fine to me. But I have a few questions. I have not looked at the tests yet, nor at the record support part of the changes.
Is it possible for this PR to be independent of #1083? This PR was opened before that.
| AnalyzeAllocType(const clang::FunctionDecl* Fn, | ||
| AnalyzeAllocType(clang::FunctionDecl* Fn, |
There was a problem hiding this comment.
Why is this change required?
There was a problem hiding this comment.
First version of this PR was injecting attributes after analyzing, and I found serious problems with that and removed the feature, but forgot to change const and const_cast. I reversed them git diff is clear now.
Also it is not dependent to any other PR now, it would be better to merge this PR first
| if (attrName == "cppAllocUnknown") | ||
| return INTEROP_RETURN(AllocType::Unknown); |
There was a problem hiding this comment.
Not required. Why should we support "unknown" attribute?
There was a problem hiding this comment.
You are right, I put it there to be consistent with enum
There was a problem hiding this comment.
We also do not need Null, I guess? It is only useful in recursion, to not effect other returns
| for (const auto* attr : FD->specific_attrs<clang::AnnotateAttr>()) { | ||
| llvm::StringRef attrName = attr->getAnnotation(); |
There was a problem hiding this comment.
Can you check if this same code also works with APINotes? If not, what additional changes are required?
There was a problem hiding this comment.
No, I need another loop like
for (const auto* attr : FD->specific_attrs<clang::SwiftAttrAttr>())| } | ||
|
|
||
| bool IsAllocator(ConstFuncRef Fn) { | ||
| std::optional<AllocType> IsAllocator(ConstFuncRef Fn) { |
There was a problem hiding this comment.
Why introduce std::optional? Can we represent no attribute available with Unknown?
You can condition on Unknown in AnalyzeAllocType.
There was a problem hiding this comment.
You are also right about that, I put optional to distinguish no-attr functions with None-Unknown attred functions, for recursive base case in analyzer, but it makes sense to remove cppAllocUnknown and we can use this value for no-attr situation
| } | ||
|
|
||
| bool IsAllocator(ConstFuncRef Fn) { | ||
| AllocType IsAllocator(ConstFuncRef Fn) { |
There was a problem hiding this comment.
warning: no header providing "Cpp::AllocType" is directly included [misc-include-cleaner]
tr);
^0d585c4 to
f89d189
Compare
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
Can we have one test case for a templated function and one for methods?
One more test where the annotation is not present in the original function. For example:
int* foo() { return new int; }
__attribute__((annotate("cppAllocNew"))) int* foo();| // Nullopt is returned because when analyzer calls this API it should know | ||
| // whether an attribute injected before or FD has a meaningful attribute | ||
| return INTEROP_RETURN(AllocType::Unknown); |
There was a problem hiding this comment.
Is this comment relevant?
There was a problem hiding this comment.
No, I forgot to delete it after removing attribute injection
| void* __attribute__((cf_returns_retained)) CFAllocFunc(); | ||
|
|
||
| void __attribute__((annotate("cppAllocNone"))) NoneFunc(); | ||
| void __attribute__((annotate("cppAllocNew"))) NewFunc(); |
There was a problem hiding this comment.
A void function, that returns New!!
There was a problem hiding this comment.
😅 Since i did not implement or call any of them and the functions are just for attribute holding, I did not care but I can change it if you think it is required
I added one test for a method, but testing redeclaration is not responsibility of this function because this function is just there to recognize whether the given decl has the target attributes, but redeclaration is more about clang's merging mechanism and which decl you take and give to test, so lookup. These tests are already written in CppJIT and they pass because CppJIT's lookup mechanism takes the last decl, in despite of CppInterOp's GetNamed that takes the canonical decl which will not include the attribute if you redeclare the function after implementing it. And for the templates, I am aware they do not enter in both analyzer and IsAllocator because they are FunctionTemplateDecl, but CppJIT gives the instantiated version so both works fine except caching when you call function with different T parameters. We can discuss it but I do not add it for now because I know it will not pass |
| } | ||
|
|
||
| bool IsAllocator(ConstFuncRef Fn) { | ||
| AllocType IsAllocator(ConstFuncRef Fn) { |
There was a problem hiding this comment.
warning: no header providing "Cpp::AllocType" is directly included [misc-include-cleaner]
RETURN(nullptr);
^39499fb to
f8925d1
Compare
IsAllocator now returns std::optional<AllocType> instead of bool, and reads back cppAllocX clang::annotate attributes in addition to the native ones it already understood (RestrictAttr, OwnershipAttr, the CFReturnsRetained family). A hand-annotated or API-notes-annotated function is therefore recognised without walking its body at all. nullopt means no signal was found, which is deliberately distinct from AllocType::None -- the latter is a definite answer, the former tells the analyzer to keep looking. AllocationTraverser::handleCall consults IsAllocator on a callee before recursing into it, so a call to a declaration-only function that already carries an attribute short-circuits instead of degrading to Unknown. GetAllocType's parameter changed from ConstFuncRef to FuncRef and the .td was updated to match. Also fixes <optional> leaking into the C-only branch of CppInterOpTypes.h, which broke the plain-C build, and pins TracingTests.cpp to -std=c++17 since the interpreter otherwise defaults to C++14 and cannot parse std::optional in the public headers. Tests cover the full IsAllocator branch matrix, attribute merging across redeclarations (both a single function and an overload set, via a pragma clang attribute push block in a preincluded header), and the handleCall short-circuit on a body-less annotated callee.
…wnership attribute when our specific signature is inserted which is cppAlloc`EnumValue` and EnumValue can not be null or unknown, furthermore I changed return type to raw enum from optional wrapped, because we can use Unknown value to pass no-attr information
f8925d1 to
fd2ba8d
Compare
…ed and unnecessary comment lines are removed in IsAllocator
fd2ba8d to
3d7c271
Compare
IsAllocator now returns an AllocType value instead of bool, and it recognizes user added attrbiutes with specific keyword:
cppAlloc
AllocTypeEnumFor example
cppAllocMalloc attribute is added to mergeFunc with specific signature, and IsAllocator recognizes cppAllocMalloc attribute as AllocType::Malloc.
Also traverser now asks IsAllocator whether the callee is labeled with an attribute before recursively analyzing it
@Vipul-Cariappa @vgvassilev