[clang-include-cleaner] Make cleanup attr report expr location

[DaanDeMeyer]

Instead of reporting the location of the attribute, let's report the location of the function reference that's passed to the cleanup attribute as the first argument. This is required as the attribute might be coming from a macro which means clang-include-cleaner skips the use as it gets attributed to the header file declaringt the macro and not to the main file.

To make this work, we have to add a fake argument to the CleanupAttr constructor so we can pass in the original Expr alongside the function declaration.

Fixes #140212

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-tools-extra

Author: Daan De Meyer (DaanDeMeyer)

Changes

Instead of reporting the location of the attribute, let's report the location of the function reference that's passed to the cleanup attribute as the first argument. This is required as the attribute might be coming from a macro which means clang-include-cleaner skips the use as it gets attributed to the header file declaringt the macro and not to the main file.

To make this work, we have to add a fake argument to the CleanupAttr constructor so we can pass in the original Expr alongside the function declaration.

Fixes #140212

---

Full diff: https://github.com/llvm/llvm-project/pull/140233.diff

4 Files Affected:

• (modified) clang-tools-extra/include-cleaner/lib/WalkAST.cpp (+1-1)
• (modified) clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp (+1-1)
• (modified) clang/include/clang/Basic/Attr.td (+4-2)
• (modified) clang/lib/Sema/SemaDeclAttr.cpp (+1-1)

diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index ba6eff49e9c98..a6f2559dd8e93 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -322,7 +322,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
   }
 
   bool VisitCleanupAttr(CleanupAttr *attr) {
-    report(attr->getLocation(), attr->getFunctionDecl());
+    report(attr->getExpr()->getExprLoc(), attr->getFunctionDecl());
     return true;
   }
 
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 19695a34bd63e..0de0b77f33daf 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -573,7 +573,7 @@ TEST(WalkAST, OperatorNewDelete) {
 
 TEST(WalkAST, CleanupAttr) {
   testWalk("void* $explicit^freep(void *p);",
-           "void foo() { __attribute__((^__cleanup__(freep))) char* x = 0; }");
+           "void foo() { __attribute__((__cleanup__(^freep))) char* x = 0; }");
 }
 
 } // namespace
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index ccd13a4cca4dd..e90d18d8435bf 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -226,7 +226,8 @@ class BoolArgument<string name, bit opt = 0, bit fake = 0> : Argument<name, opt,
 class IdentifierArgument<string name, bit opt = 0> : Argument<name, opt>;
 class IntArgument<string name, bit opt = 0> : Argument<name, opt>;
 class StringArgument<string name, bit opt = 0> : Argument<name, opt>;
-class ExprArgument<string name, bit opt = 0> : Argument<name, opt>;
+class ExprArgument<string name, bit opt = 0, bit fake = 0> : Argument<name, opt,
+                                                                      fake>;
 class DeclArgument<DeclNode kind, string name, bit opt = 0, bit fake = 0>
     : Argument<name, opt, fake> {
   DeclNode Kind = kind;
@@ -1351,7 +1352,8 @@ def OSConsumesThis : InheritableAttr {
 
 def Cleanup : InheritableAttr {
   let Spellings = [GCC<"cleanup">];
-  let Args = [DeclArgument<Function, "FunctionDecl">];
+  let Args = [DeclArgument<Function, "FunctionDecl">,
+              ExprArgument<"Expr", /*fake=*/1>];
   let Subjects = SubjectList<[LocalVar]>;
   let Documentation = [CleanupDocs];
 }
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 377595639bef1..6b1a42bcc392f 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3620,7 +3620,7 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
     return;
   }
 
-  D->addAttr(::new (S.Context) CleanupAttr(S.Context, AL, FD));
+  D->addAttr(::new (S.Context) CleanupAttr(S.Context, AL, FD, E));
 }
 
 static void handleEnumExtensibilityAttr(Sema &S, Decl *D,

[kadircet]

changes in include-cleaner LG, but I am not sure about the canonical way of tracking this location in clang (i.e. if we're going to track an expression within the attribute, do we still need to tract the underlying decl as well? can't we just extract it from expr when needed?) so cc @AaronBallman, can you redirect to relevant folks if it isn't obvious?

[AaronBallman]

I'm not keen on this solution. There are three approaches that would be better:

1. Change to accept a DeclRefExpr instead, add an additional method here to get the FunctionDecl out of the expression.
2. Add an extra SourceLocation member here, then update handleCleanupAttr() to set the source location to the location of the expression argument.
3. Generically expose expression arguments on the Attr subclass.

I think #3 is where we'd ultimately like to go, but is also probably a decent amount of work given how many attributes we have. I think #1 may be the most reasonable solution here, while #2 is likely the easiest.

CC @erichkeane

[erichkeane]

If we're doing 1, we need to make sure we do this at the DeclArgument level, which means modifying quite a bit of other attributes potentially. PERHAPS we could just have it cause us to store a pair of DeclRefExpr and FunctionDecl, that way it minimizes the amount of work.

As far as the extra source-location (2): that is reasonable as well. I don't think we have an automagic fake argument to do here, so it would have to just be in AdditionalMembers.

I would lean AGAINST 3, unless we have a REALLY good reason to. This would require that Attr have a trailing-objects, and thus store that much extra information. We COULD potentially change the entire attributes infrastructure to just store the ArgsUnion type (which is a union of Expr*/IdentifierInfo* IIRC) then make every access just do the decoding from those, but that is a LOT of work.

IN THE END, I think 2 is easiest, so hackiest. 1 is IMO the 'best' situation here (and is only a medium amount of work, with some talbegen/minor other changes). We might have other argument types that should do something similar, but no reason we couldn't do that in the future.

[AaronBallman]

I would lean AGAINST 3, unless we have a REALLY good reason to.

Being able to iterate over arguments generically would mean removing a ton of specific logic in places like pretty printing, AST matching, etc. Basically, if Decl and Stmt let you iterate over its children() generically, Attr would be good to handle for the same reasons.

This would require that Attr have a trailing-objects,

Wha? ParsedAttr already tracks this generically, doesn't it? So I was thinking of lifting that onto AttributeCommonInfo so that Attr gets the benefit too. But yeah, if this requires a bunch of extra overhead, I'm also opposed -- we want attributes to be cheap rather than memory-hungry given how many of them show up in system headers.

[erichkeane]

I would lean AGAINST 3, unless we have a REALLY good reason to.

Being able to iterate over arguments generically would mean removing a ton of specific logic in places like pretty printing, AST matching, etc. Basically, if Decl and Stmt let you iterate over its children() generically, Attr would be good to handle for the same reasons.

Hmm... that could definitely be useful to do. A bit of memory pressure though.

This would require that Attr have a trailing-objects,

Wha? ParsedAttr already tracks this generically, doesn't it? So I was thinking of lifting that onto AttributeCommonInfo so that Attr gets the benefit too.

It does so as trailing-objects, which we then remove/pass to the *Attr ctor's, which are auto generated to be the 'right' type. BUT we use manual checking to do the conversion between teh generic ArgsUnion and this.

But yeah, if this requires a bunch of extra overhead, I'm also opposed -- we want attributes to be cheap rather than memory-hungry given how many of them show up in system headers.

sigh, yeah, its a bit of a frustrating balance.

[AaronBallman]

Ah good call on those already being trailing objects on ParsedAttr, so it's an even bigger ask than I originally was thinking. Another good reason not to go that route yet, that's likely to be a complicated patch because of how much code changes (ClangAttrEmitter.cpp, SemaDeclAttr.cpp, probably the parser as well) and because of performance concerns (would definitely need to be tested for perf changes).

[erichkeane]

Ah good call on those already being trailing objects on ParsedAttr, so it's an even bigger ask than I originally was thinking. Another good reason not to go that route yet, that's likely to be a complicated patch because of how much code changes (ClangAttrEmitter.cpp, SemaDeclAttr.cpp, probably the parser as well) and because of performance concerns (would definitely need to be tested for perf changes).

Yep. I'm somewhat talking myself into 2 above being acceptalbe, but I'll hate it :D

(I am very much a drive by contributor and this discussion is making me very scared of ever getting this landed :p)

sigh I don't think we can accept it as-is. But I think with some mild work to how DeclArgument works in tablegen (in a way that causes us to need exposes the Expr as well) would be completely acceptable to us as well.

I THINK I would be OK having us have this Attr store the SourceLocation in AdditionalMembers for this one, as long as we had some sort of FIXME esque comment on it that results in pointing out the above.

[AaronBallman]

It's safe to ignore us for the moment because we're just discussing what way to go forward so we can give you actual advice. :-D

(1) Basically boils down to changing clang/utils/TableGen/ClangAttrEmitter.cpp so that DeclArgument generates code differently. Currently it requires the attribute subclasses to pass in a declaration pointer, and now we'd want it to either take a declaration pointer and an expression pointer, or take a const DeclRefExpr * and automatically find the declaration from it as-needed.

(2) Basically boils down to adding AdditionalMembers for a source location, so it's pretty hacky but very direct.

I think our preference is for (1) because that's a general solution that will help all attributes. I think we can probably live with (2) though.

[DaanDeMeyer]

For 1) in handleCleanupAttr there's code to handle the case where the expr is not a declrefexpr. How does that work with passing in a DeclRefExpr to DeclArgument?

[AaronBallman]

Good point, we'd likely need to handle unresolved lookups and other kinds of expressions, so maybe it would take a const Expr * instead.

[DaanDeMeyer]

@erichkeane @AaronBallman I reworked this to use 2). I had a look at 1) but given how thinly stretched I already am, I don't have time to make such a change at this time.

[kadircet]

LG for include-cleaner changes.

[AaronBallman]

LGTM!