[clang][TSA] Make RequiresCapability a DeclOrType attribute #67095

tbaederr · 2023-09-22T07:26:38Z

This was much easier than expected actually.

I'm sure I'm missing a lot of changes and test coverage though.

@aaronpuchert I'm starting a new review here instead of continuing the one in Phab since this is a completely different attempt. I hope that's ok.

llvmbot · 2023-09-22T07:28:17Z

@llvm/pr-subscribers-clang

Changes

This was much easier than expected actually.

I'm sure I'm missing a lot of changes and test coverage though.

@aaronpuchert I'm starting a new review here instead of continuing the one in Phab since this is a completely different attempt. I hope that's ok.

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

2 Files Affected:

(modified) clang/include/clang/Basic/Attr.td (+3-3)
(modified) clang/test/Sema/warn-thread-safety-analysis.c (+11)

diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 21a3b5226623cf2..7eab87dac6921f1 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -3305,7 +3305,7 @@ def ReleaseCapability : InheritableAttr { let Documentation = [ReleaseCapabilityDocs]; } -def RequiresCapability : InheritableAttr { +def RequiresCapability : DeclOrTypeAttr { let Spellings = [Clang<"requires_capability", 0>, Clang<"exclusive_locks_required", 0>, Clang<"requires_shared_capability", 0>, @@ -3314,8 +3314,8 @@ def RequiresCapability : InheritableAttr { let LateParsed = 1; let TemplateDependent = 1; let ParseArgumentsAsUnevaluated = 1; - let InheritEvenIfAlreadyPresent = 1; - let Subjects = SubjectList<[Function]>; + /*let InheritEvenIfAlreadyPresent = 1;*/ + /*let Subjects = SubjectList<[Function]>;*/ let Accessors = [Accessor<"isShared", [Clang<"requires_shared_capability", 0>, Clang<"shared_locks_required", 0>]>]; let Documentation = [Undocumented]; diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index 642ea88ec3c96f7..cd852efac21cb81 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -136,6 +136,17 @@ int main(void) { // Cleanup happens automatically -> no warning. } + /// Function pointers + { + int __attribute__((requires_capability(&mu1))) (*function_ptr)(int) = Foo_fun1; + + function_ptr(5); // expected-warning {{calling function 'function_ptr' requires holding mutex 'mu1'}} + + mutex_exclusive_lock(&mu1); + int five = function_ptr(5); + mutex_exclusive_unlock(&mu1); + } + return 0; }

clang/test/Sema/warn-thread-safety-analysis.c

tbaederr · 2023-10-03T06:52:24Z

Ping

aaronpuchert

This was much easier than expected actually.

Making it a DeclOrType attribute is indeed a nice idea, this would allow existing attributes to stay where they are. Is it still inheritable, i.e. does it also apply to later redeclarations?

Of course I'm also wondering why we don't have to change anything in the analysis, aren't we currently only looking at declaration attributes, and wouldn't consider attributes on the type? Or is there some mechanism that gives us the combined set of declaration and type attributes?

I'm starting a new review here instead of continuing the one in Phab since this is a completely different attempt. I hope that's ok.

Yeah, since it's a completely different approach than https://reviews.llvm.org/D152246 we're not really losing any history.

aaronpuchert · 2023-10-08T01:44:29Z

clang/lib/Sema/SemaDeclAttr.cpp

This looks like the attribute sits on the Decl and not its type, or am I misunderstanding this?

Yeah we attach the attribute to the declaration later in this function:

RequiresCapabilityAttr *RCA = ::new (S.Context) RequiresCapabilityAttr(S.Context, AL, Args.data(), Args.size()); D->addAttr(RCA);

and in ThreadSafety.cpp we then check the decl for the attribute in VisitCallExpr():

auto *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl()); if(!D || !D->hasAttrs()) return;

for the function pointer case, D will be the VarDecl, which now has the attribute attached.

Once the Subjects list is corrected, I believe this code can be removed.

clang/test/Sema/warn-thread-safety-analysis.c

tbaederr · 2023-10-10T07:10:00Z

This was much easier than expected actually.

Making it a DeclOrType attribute is indeed a nice idea, this would allow existing attributes to stay where they are. Is it still inheritable, i.e. does it also apply to later redeclarations?

I haven't tested this explicitly, but DeclOrTypeAttr is-a InheritableAttr:
https://github.com/llvm/llvm-project/blob/5dd9266c4e0b0a02cbb02a29bf2606224ccf2820/clang/include/clang/Basic/Attr.td#L644-L647C1

Of course I'm also wondering why we don't have to change anything in the analysis, aren't we currently only looking at declaration attributes, and wouldn't consider attributes on the type? Or is there some mechanism that gives us the combined set of declaration and type attributes?

Let me check why this just works.

AaronBallman

The changes also need a release note at some point.

AaronBallman · 2023-10-11T17:39:06Z

clang/include/clang/Basic/Attr.td

Instead of removing it, can this be updated to be correct (we have FunctionPointer as a subject)?

AaronBallman · 2023-10-11T17:40:40Z

clang/lib/Sema/SemaDeclAttr.cpp

Once the Subjects list is corrected, I believe this code can be removed.

clang/test/Sema/warn-thread-safety-analysis.c

tbaederr · 2023-10-19T12:55:29Z

Ping

AaronBallman · 2023-10-19T13:24:02Z

Ping

Did you see #67095 (comment) ?

tbaederr · 2023-10-19T13:35:57Z

Ping

Did you see #67095 (comment) ?

Yes, I added the changes to processTypeAttrs() and added the subject list back. Is there something I'm missing?

But the tests fail too, I'll check that locally.

AaronBallman · 2023-10-19T13:41:21Z

Ping

Did you see #67095 (comment) ?

Yes, I added the changes to processTypeAttrs() and added the subject list back. Is there something I'm missing?

Nope, this was another lovely case of GitHub hiding details from me. The Files tab wasn't showing me your changes to that file... until I clicked it a second time and then new changes showed up. Sorry for the noise!

AaronBallman · 2023-10-19T13:43:33Z

clang/lib/Sema/SemaType.cpp

Yup, this needs to be done manually because the helper code automatically generated by tablegen isn't yet smart enough to generate logic for type attributes. I think this work should be done as part of this commit instead of left as follow-up work.

AaronBallman · 2023-10-19T13:44:55Z

clang/test/Sema/warn-thread-safety-analysis.c

We'll also need changes showing that we properly diagnose all the usual stuff from the decl form of the attribute but with the type form instead, plus additional coverage for new diagnostics like conversions between function pointer types with/out the attribute.

There are no such diagnostics when converting between function pointers. Are there supposed to be any, without adding special handling for this? I quickly checked and noreturn has the same behavior.

tbaederr · 2023-11-03T08:05:03Z

It's pretty easy to get an error like:

./tsa.c:77:18: error: incompatible function pointer types assigning to 'int (*)(int) __attribute__((requires_capability(blah)))' from 'int (void)' [-Wincompatible-function-pointer-types] 77 | function_ptr = foo2; | ^ ~~~~

but as soon as I change foo2 to also take an int parameter, the error vanishes completely.

clang/lib/AST/TypePrinter.cpp

AaronBallman · 2023-11-03T14:50:52Z

clang/lib/Sema/SemaType.cpp

+ case ParsedAttr::AT_RequiresCapability: {
+ HandleRequiresCapabilityAttr(state, type, attr);
+ attr.setUsedAsTypeAttr();
+ break;
+ }


Okay, maybe we do want to do this change separately. I think I was wrong about modeling after this; I think we want it to be done like handleFunctionTypeAttr() and FUNCTION_TYPE_ATTRS_CASELIST so that it becomes part of the FunctionType itself. That's what would get us the diagnostics on type mismatches (we look through AttributedType as sugar:

llvm-project/clang/include/clang/AST/Type.h

Line 4999 in 28b7e28

bool isSugared() const { return true; }

)

Hmmmm, my intuition is that if we don't do this work now, we'll forever be stuck with the current behavior where we don't diagnose pointer mismatches because someone will rely on that continuing to work. So maybe we do want to do this work now...

CC @erichkeane @aaronpuchert for opinions on whether we think this needs to diagnose mismatches in the initial offering of it as a type attribute.

I think I agree Aaron, if we don't diagnose now, it'll be pretty ugly to do it later.

Meh, it seems like adding a pointer to FunctionType::ExtInfo would blow it up a lot, and just a bit isn't enough to save the information.

There were a few changes there recently... i wonder if it is time to just accept that we're going all-in on trailing-storage for this type. We're now doing it for the ARM SVE types, plus the 'lesser used' structure, so perhaps the 'capability pointer' can end up there too. WDYT?

@AaronBallman Does this sound like the way to go to you as well?

No, we don't want to modify FunctionType::ExtInfo, I think what @erichkeane was talking about is that we added trailing objects to FunctionProtoType that we can squirrel lesser-used data into. That might be reasonable here because I cannot imagine anyone using thread safety attributes with K&R C interfaces, but that would be the downside to putting the data on FunctionProtoType.

It would kind of stink to have to do it in 2 places, but it could go in trailing-storage/etc on FunctionProtoType and FunctionNoProtoType, then just disambiguate it with a function. So it doesn't need to end up in FunctionType::ExtInfo or FunctionType itself.

clang/include/clang/Basic/Attr.td

tbaederr · 2024-01-22T16:02:50Z

Ping

erichkeane

Seems you pinged this without the commit to fix the things we've requested?

clang/include/clang/Basic/Attr.td

clang/lib/AST/TypePrinter.cpp

tbaederr · 2024-01-22T16:15:56Z

Seems you pinged this without the commit to fix the things we've requested?

Right, I was more looking for guidance what to do with FunctionType::ExtInfo. It seems like adding the requires capability to that would be needed to make it a function type attribute, ExtInfo currently only saves a few bits and we need a full pointer.

tbaederr · 2025-02-19T11:10:38Z

One thing I'm unsure about right now: This probably needs more work, especially regarding assigning function pointers, doesn't it? Even in the test case I added right now, Foo_fun2 is declared with SHARED_LOCKS_REQUIRED(mu2) EXCLUSIVE_LOCKS_REQUIRED(mu1), but then the function pointer I assign it to only requires mu1 - so calling it via that function pointer makes it possible to call the function without holding mu2.
Assigning it to the function pointer is only OK if the function pointer type needs a superset (or the same set) of the mutexes the function needs, otherwise it would ideally be an error. But not sure if we currently do anything similar for other TSA stuff.

aaronpuchert · 2025-02-26T01:19:48Z

Good question. Which AST nodes could we visit here? Let's see some examples of initializing function pointers/references:

void f(); void (*fp)() = f; void (*fp2)() = &f; void (*fp3)() = fp; void (&fr)() = f;

The (simplified) AST:

TranslationUnitDecl 0x55edc65d3778 |-FunctionDecl 0x55edc661d910 used f 'void ()' |-VarDecl 0x55edc661dac0 used fp 'void (*)()' cinit | `-ImplicitCastExpr 'void (*)()' <FunctionToPointerDecay> | `-DeclRefExpr 'void ()' lvalue Function 0x55edc661d910 'f' 'void ()' |-VarDecl 0x55edc661dbe8 fp2 'void (*)()' cinit | `-UnaryOperator 'void (*)()' prefix '&' cannot overflow | `-DeclRefExpr 'void ()' lvalue Function 0x55edc661d910 'f' 'void ()' |-VarDecl 0x55edc661dd48 fp3 'void (*)()' cinit | `-ImplicitCastExpr 'void (*)()' <LValueToRValue> | `-DeclRefExpr 'void (*)()' lvalue Var 0x55edc661dac0 'fp' 'void (*)()' `-VarDecl 0x55edc661dea0 fr 'void (&)()' cinit `-DeclRefExpr 'void ()' lvalue Function 0x55edc661d910 'f' 'void ()'

The third and fourth declarations are tricky. I don't see an AST node that we could attach to for finding this. Perhaps we should adapt Sema to add casts for the attributes? Then we could always attach to the ImplicitCastExpr. (Note that the LValueToRValue cast is orthogonal and not relevant for this, it simply means we're reading fp.)

tbaederr requested review from aaronpuchert and haoNoQ September 22, 2023 07:26

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 22, 2023

tbaederr commented Sep 22, 2023

View reviewed changes

clang/test/Sema/warn-thread-safety-analysis.c Outdated Show resolved Hide resolved

tbaederr force-pushed the tsa2 branch 2 times, most recently from 2d0d268 to 5dd9266 Compare September 26, 2023 07:58

aaronpuchert reviewed Oct 8, 2023

View reviewed changes

aaronpuchert requested a review from AaronBallman October 8, 2023 21:51

AaronBallman reviewed Oct 11, 2023

View reviewed changes

tbaederr force-pushed the tsa2 branch from 5dd9266 to 2fdd448 Compare October 13, 2023 08:24

AaronBallman requested a review from erichkeane October 19, 2023 13:23

AaronBallman reviewed Oct 19, 2023

View reviewed changes

tbaederr force-pushed the tsa2 branch from 2fdd448 to 8b52d23 Compare November 3, 2023 08:06

AaronBallman reviewed Nov 3, 2023

View reviewed changes

erichkeane reviewed Dec 6, 2023

View reviewed changes

clang/include/clang/Basic/Attr.td Outdated Show resolved Hide resolved

erichkeane reviewed Jan 22, 2024

View reviewed changes

clang/include/clang/Basic/Attr.td Outdated Show resolved Hide resolved

clang/lib/AST/TypePrinter.cpp Outdated Show resolved Hide resolved

tbaederr force-pushed the tsa2 branch from 4d0bc2c to 1bb6221 Compare February 19, 2025 07:45

tbaederr force-pushed the tsa2 branch from 1bb6221 to 354359e Compare February 19, 2025 11:12

tbaederr force-pushed the tsa2 branch from 354359e to 4f0f1b0 Compare April 23, 2025 06:37

[clang][TSA] Make RequiresCapability a DeclOrType attribute

62f499b

tbaederr force-pushed the tsa2 branch from 4f0f1b0 to 62f499b Compare April 23, 2025 08:24

[clang][TSA] Make RequiresCapability a DeclOrType attribute #67095

Are you sure you want to change the base?

[clang][TSA] Make RequiresCapability a DeclOrType attribute #67095

Uh oh!

Conversation

tbaederr commented Sep 22, 2023

llvmbot commented Sep 22, 2023

Uh oh!

tbaederr commented Oct 3, 2023

aaronpuchert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

tbaederr commented Oct 10, 2023

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

tbaederr commented Oct 19, 2023

AaronBallman commented Oct 19, 2023

tbaederr commented Oct 19, 2023

AaronBallman commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbaederr commented Nov 3, 2023

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

tbaederr commented Jan 22, 2024

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tbaederr commented Jan 22, 2024

tbaederr commented Feb 19, 2025

aaronpuchert commented Feb 26, 2025

Labels

5 participants

AaronBallman commented Oct 19, 2023 •

edited

Loading