Redesign IsDeallocator to obtain deallocation behaviour per param by keremsahn · Pull Request #1121 · compiler-research/CppInterOp

keremsahn · 2026-09-27T13:46:54Z

I would rather get this PR merged and rewrite compiler-research/cppjit#114

The former version of IsDeallocator was just looking for Clang attributes and ID check for free and returning a bool so index information was disappearing, but now it recognizes our keyword similar to IsAllocator:

cppDealloc`DeallocType`_`index`

for both annotation and APINotes, and also recognizes which index is given by ownership_attrs. And returns a bool which indicates whether attribute lookup was successful or not, and writes to the vector parameter it takes.
Details for usage:

Clang's convention of ownership attributes are followed, parameters are 1-indexed, therefore if user wants to label first parameter it should write cppDeallocDelete_1
If the function has an implicit this first parameter is this, therefore user should write cppDeallocDelete_2 to label first actual parameter. Also it is not allowed to label this parameter.
If user did not indicate the index for attribute, the attribute is assumed to be for first index
I am still not able to use modules in Cling, therefore APINotes tests are skipped for cling

Also only missing thing will be custom allocator-deallocator pairing after this one is merged, so we are close to have a properly working design :)

codecov · 2026-09-27T13:52:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.63%. Comparing base (2332f84) to head (0866403).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1121      +/-   ##
==========================================
+ Coverage   88.52%   88.63%   +0.10%     
==========================================
  Files          24       24              
  Lines        6851     6908      +57     
==========================================
+ Hits         6065     6123      +58     
+ Misses        786      785       -1

Files with missing lines	Coverage Δ
lib/CppInterOp/CppInterOp.cpp	`91.28% <100.00%> (+0.16%)`	⬆️

Files with missing lines	Coverage Δ
lib/CppInterOp/CppInterOp.cpp	`91.28% <100.00%> (+0.16%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions

clang-tidy made some suggestions

github-actions · 2026-09-27T14:15:02Z

+// does not have self 0th index will be DeallocType::None.
+// So actual first parameter is located at 1st index it is
+// also compatible with Clang convension.
+std::vector<DeallocType> IsDeallocator(ConstFuncRef Fn) {


warning: no header providing "Cpp::DeallocType" is directly included [misc-include-cleaner]

h Clang convension. ^

Vipul-Cariappa

Initial round of review:
I should still look at the tests.

Vipul-Cariappa · 2026-09-30T11:13:57Z

+      else if (attrName == "cppDeallocNone")
+        kind = DeallocType::None;


Suggested change

else if (attrName == "cppDeallocNone")

kind = DeallocType::None;

I believe this is safe.

The reason I put None for both cppAlloc and cppDealloc is, if there is a vulnerability in analyzer and it causes a UB, analyzer can be supressed until it is fixed

Vipul-Cariappa · 2026-09-30T11:22:59Z

+        } else if (token.consume_front("cppDeallocNone_")) {
+          unsigned index;
+          if (!token.getAsInteger(10, index) && index < VPP.size())
+            VPP[index] = DeallocType::None;


Suggested change

} else if (token.consume_front("cppDeallocNone_")) {

unsigned index;

if (!token.getAsInteger(10, index) && index < VPP.size())

VPP[index] = DeallocType::None;

I believe this is safe to remove.

if (!token.getAsInteger(10, index)

This is necessary, because we know that an integer comes after CppDeallocDelete_ via getAsInteger, but

&& index < VPP.size()

may be removed, but i still prefer to not

I did not remove these checks, I believe having bound checks are necessary

github-actions

clang-tidy made some suggestions

github-actions · 2026-10-08T07:47:46Z

+// cppDeallocDelete_2 to labeling first actual parameter, also it is not allowed
+// to label implicit `this`. So actual first parameter is located at 1st index,
+// if implicit `this` exists.
+bool IsDeallocator(ConstFuncRef Fn, std::vector<DeallocType>& VPP) {


warning: no header providing "Cpp::DeallocType" is directly included [misc-include-cleaner]

icit `this` exists. ^

Vipul-Cariappa · 2026-10-09T13:46:30Z

+  if (VPP.size() != numParam + hasImplicitThis)
+    return INTEROP_RETURN(false);


We should not rewrite existing elements.

Vipul-Cariappa · 2026-10-09T13:58:01Z

+      if (index < 0 || (uint64_t)(index - 1) >= VPP.size() ||
+          (index == 1 && hasImplicitThis))
+        continue;


Suggested change

if (index < 0 || (uint64_t)(index - 1) >= VPP.size() ||

(index == 1 && hasImplicitThis))

continue;

if (index <= 0 || index > VPP.size() ||

(index == 1 && hasImplicitThis))

continue;

?

Vipul-Cariappa · 2026-10-09T14:09:37Z

+      if (!token.consume_front("_") || token.getAsInteger(10, index) ||
+          index < 1 || index - 1 >= VPP.size() ||
+          (hasImplicitThis && index == 1))
+        continue;


Cross-check this...

Vipul-Cariappa · 2026-10-09T14:10:12Z

+      - Name: releaseBoth
+        SwiftReturnOwnership: "cppDeallocDelete_1 cppDeallocFree_2"
+      - Name: releaseBare
+        SwiftReturnOwnership: cppDeallocDelete


Add a newline here.

IsDeallocator used to answer a single bool for the whole function, which could not say *which* argument is released, nor with which operation. It now appends one DeallocType per parameter to the given vector, preceded by one entry for the object parameter when the function has an implicit this. Whatever the vector already holds is left untouched, so the results of several functions can be collected into one vector. A slot carries the value of clang's source index one above it, following the convention of the ownership attributes: indexes are 1 based and count the implicit this. Labelling the implicit this is not supported, and such a label is ignored rather than diagnosed, as is an out of range one. The index may also be omitted, in which case the first actual parameter is meant. A parameter carrying no deallocation attribute is reported as Opaque rather than None, so that a later analyzer pass can tell "no information" apart from "labelled cppDeallocNone" and merge the two sources. False is returned only when the given declaration is not a function, in which case the vector is not modified at all. Three attribute channels feed the vector: - ownership_takes/holds: clang's own ParamIdx, read through getLLVMIndex(), which is the same zero origin encoding counting the implicit this. Sema validates these indexes, so no range check is needed here. - annotate("cppDeallocFree"/"Delete"/"DeleteArr"/"None"[, i...]): Sema does not validate the indexes, so out of range, zero, negative and non-integral arguments are dropped here. - swift_attr with space separated "cppDeallocFree[_1]" style tokens, which is the only shape API Notes can express through its single SwiftReturnOwnership string. On a conflicting index the last matching attribute wins.

github-actions

clang-tidy made some suggestions

github-actions · 2026-10-10T16:04:40Z

+// Function appends a DeallocType to the vector taken, for every argument in
+// the function including implicit this. If an argument does not have an
+// deallocation ownership, Opaque value is added.
+bool IsDeallocator(ConstFuncRef Fn, std::vector<DeallocType>& VPP) {


warning: no header providing "Cpp::DeallocType" is directly included [misc-include-cleaner]

que value is added. ^

Vipul-Cariappa

LGTM!

Vipul-Cariappa · 2026-10-14T13:36:56Z

+  std::vector<DeallocType> newVPP(numParam + hasImplicitThis,
+                                  DeallocType::Opaque);
+  if (FD->getBuiltinID() == Builtin::ID::BIfree) {
+    VPP.push_back(DeallocType::Free);
+    return INTEROP_RETURN(true);
+  }


Suggested change

std::vector<DeallocType> newVPP(numParam + hasImplicitThis,

DeallocType::Opaque);

if (FD->getBuiltinID() == Builtin::ID::BIfree) {

VPP.push_back(DeallocType::Free);

return INTEROP_RETURN(true);

}

if (FD->getBuiltinID() == Builtin::ID::BIfree) {

VPP.push_back(DeallocType::Free);

return INTEROP_RETURN(true);

}

std::vector<DeallocType> newVPP(numParam + hasImplicitThis,

DeallocType::Opaque);

The vector can be create after this if

github-actions Bot reviewed Sep 27, 2026

View reviewed changes

keremsahn force-pushed the isdealloc-vector branch from 57c0f0d to f229cff Compare September 27, 2026 15:39

Vipul-Cariappa self-requested a review September 30, 2026 10:55

Vipul-Cariappa reviewed Sep 30, 2026

View reviewed changes

keremsahn force-pushed the isdealloc-vector branch from f229cff to 75a4ff2 Compare October 8, 2026 07:35

github-actions Bot reviewed Oct 8, 2026

View reviewed changes

keremsahn force-pushed the isdealloc-vector branch from 75a4ff2 to dd4ff85 Compare October 8, 2026 08:12

Vipul-Cariappa requested changes Oct 9, 2026

View reviewed changes

keremsahn force-pushed the isdealloc-vector branch from 2c2c688 to 4a53d07 Compare October 10, 2026 15:44

keremsahn force-pushed the isdealloc-vector branch from 4a53d07 to 0866403 Compare October 10, 2026 15:49

keremsahn requested a review from Vipul-Cariappa October 10, 2026 15:51

github-actions Bot reviewed Oct 10, 2026

View reviewed changes

Vipul-Cariappa approved these changes Oct 14, 2026

View reviewed changes

Vipul-Cariappa merged commit 9e5fd7a into compiler-research:main Oct 14, 2026
32 checks passed

		else if (attrName == "cppDeallocNone")
		kind = DeallocType::None;

		if (VPP.size() != numParam + hasImplicitThis)
		return INTEROP_RETURN(false);

Conversation

keremsahn commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keremsahn Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keremsahn Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 10, 2026

Choose a reason for hiding this comment

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

keremsahn commented Sep 27, 2026 •

edited

Loading

codecov Bot commented Sep 27, 2026 •

edited

Loading

keremsahn Sep 30, 2026 •

edited

Loading

keremsahn Sep 30, 2026 •

edited

Loading