Thread Safety Analysis "guarded_by" attribute doesn't work for struct fields in C code

[llvmbot]

Bugzilla Link	20403
Version	trunk
OS	Linux
Reporter	LLVM Bugzilla Contributor
CC	@AaronBallman,@benvanik,@LebedevRI,@mikaelholmen,@nashidau,@zygoloid

Extended Description

$ cat demo.c
struct mutex {
        int x;
};

struct guarded_int {
        struct mutex mu;
        int value __attribute__((guarded_by(mu))); 
};

$ clang -c demo.c
demo.c:7:38: error: use of undeclared identifier 'mu'
        int value __attribute__((guarded_by(mu)));
                                            ^
1 error generated.

Clang compiles the code okay with "-x c++", so it seems to be a bug in how Thread Safety Analysis is wired into the C frontend.

[llvmbot]

Looking into this briefly, I'm wondering if some code related to ExprArgument handling (e.g., within the Sema layer) is only considering scoped lookups for CXXRecordDecls, when it should be generalized to handle plain RecordDecls too?

[zygoloid]

In C, struct and union member names are simply never in scope; it's not surprising that name lookup doesn't find them here. The name space containing these names is only searched when the name appears on the right hand side of a . or ->.

It's not clear to me what, if anything, should be done about this. We probably don't want to introduce an implicit 'this' pointer into C.

[llvmbot]

That's fair, though it's unfortunate that it seems to limit the usefulness of Thread Safety Analysis for C code bases that can't be compiled as C++ without a lot of cleanup work first.

Is there at least a workaround that could be used here for C code?

[Benvanik]

Apologize for the necroing, but this would be a real quality of life improvement. My workaround is to force compilation of all code into C++ with -x c++ when building for ASAN however that then masks issues around C compatibility during normal development.

[AaronBallman]

In C, struct and union member names are simply never in scope; it's not
surprising that name lookup doesn't find them here. The name space
containing these names is only searched when the name appears on the right
hand side of a . or ->.

It's not clear to me what, if anything, should be done about this. We
probably don't want to introduce an implicit 'this' pointer into C.

While we probably don't want to introduce an implicit 'this' pointer into C, I think we still should consider doing that. We need a solution that works in both C and C++ with the same syntax (because the structure may be in a header file that's included from both .c and .cpp compilands) and this is the most obvious solution for users to reach for.

There's no reason why the thread safety analysis framework shouldn't work on this code in C aside from solving "how do you name the structure member" and that's entirely implementation-defined anyway because it's only a question inside the argument list of an attribute. So for the purposes of name lookup within this very specific context, it seems reasonable to support that. Lacking the ability to do this really devalues the otherwise very useful thread safety analyses we do by cutting out a somewhat common use case in C.

[zygoloid]

We should consider how to represent the case where a member of a struct is notionally guarded by acquisition of the a lock on the entire struct:

struct Lockable {
  int internal_mutex;
  int value [[clang::guarded_by(*this)]];
};
void lock(Lockable *p) [[clang::exclusive_lock_function(*p)]];
// ...

Allowing the attributes to name the struct members isn't enough to model this; we need to allow the attributes to name the struct itself.

We could give C++-like semantics in C by injecting into scope an identifier 'this' along with identifiers for all the struct members while parsing a thread safety annotation expression. I'm not sure that's the cleanest way forward, though. Perhaps instead we should provide an alternative attribute syntax that permits the 'this' parameter to be explicitly declared:

struct Lockable {
  int internal_mutex;
  int value [[clang::guarded_by(for Lockable *self: *self)]];
};

and

struct guarded_int {
        struct mutex mu;
        int value __attribute__((guarded_by(for guarded_int *self: self->mu))); 
};

(This is placeholder syntax rather than something I'm actually proposing.)

[AaronBallman]

We should consider how to represent the case where a member of a struct is
notionally guarded by acquisition of the a lock on the entire struct:

struct Lockable {
int internal_mutex;
int value [[clang::guarded_by(*this)]];
};
void lock(Lockable *p) [[clang::exclusive_lock_function(*p)]];
// ...

Allowing the attributes to name the struct members isn't enough to model
this; we need to allow the attributes to name the struct itself.

I think this could be useful, but it seems like a further extension to the minimal support for naming a field, too. I'd be curious if DeLesley thinks this is the correct model though. My understanding of the analysis is that we want to know what the actual lock object is, and a structure cannot act as a lock object by itself (it has to be some member of the structure that is the actual locking mechanism).

We could give C++-like semantics in C by injecting into scope an identifier
'this' along with identifiers for all the struct members while parsing a
thread safety annotation expression. I'm not sure that's the cleanest way
forward, though. Perhaps instead we should provide an alternative attribute
syntax that permits the 'this' parameter to be explicitly declared:

struct Lockable {
int internal_mutex;
int value [[clang::guarded_by(for Lockable *self: *self)]];
};

and

struct guarded_int {
struct mutex mu;
int value attribute((guarded_by(for guarded_int *self:
self->mu)));
};

(This is placeholder syntax rather than something I'm actually proposing.)

Using new syntax is an interesting option. I would be a little bit worried about injecting a 'this' identifier only because I've seen structures in the wild in C that have a 'this' member, and we'd have to figure out how to handle those beasts.

[melver]

As mentioned on cfe-dev, we're interested in using ThreadSafetyAnalysis for the Linux kernel.

Since this issue has existed for some years, what is required to get this unstuck? Any estimate on the implementation complexity to get this supported?

(Unfortunately I'm not at all familiar with frontend internals, so I wouldn't know where to start -- otherwise I would have tried to put together a prototype, as that feels like the best way to discuss further.)

[aaronpuchert]

Since this issue has existed for some years, what is required to get this unstuck? Any estimate on the implementation complexity to get this supported?

Best qualified to answer this is probably @AaronBallman, but I'll give my 2 cents.

While we probably don't want to introduce an implicit 'this' pointer into C, I think we still should consider doing that. We need a solution that works in both C and C++ with the same syntax (because the structure may be in a header file that's included from both .c and .cpp compilands) and this is the most obvious solution for users to reach for.

That's probably the easiest solution, but it's still not easy. C code generally doesn't have any CXXThisExpr, so there might be weird bugs or assertions. But you could just figure that out as you go along. (Perhaps we want to introduce a separate AST node type, but that comes with its own problems.)

At some point we have to insert the members of the struct into the lookup scope, as it's done in C++, and emit some MemberExpr with an implicit CXXThisExpr. The code is already there but probably guarded by a language check. We don't want to generally allow this in C, but only in the context of attributes though. A good place to start might be Parser::ParseAttributeArgsCommon in clang/lib/Parse/ParseDecl.cpp. The calls to ParseIdentifierLoc and ParseAssignmentExpression are where the magic happens. So I'd say we have to add some code before those calls.

My understanding is that the attributes are parsed in a "complete type" context, i.e. lookup and semantic analysis is deferred until we've seen all members. That's likely the same in C and C++, because why would we maintain two different ways to do it? But I'm just guessing here.

We should consider how to represent the case where a member of a struct is notionally guarded by acquisition of the a lock on the entire struct:

struct Lockable { int internal_mutex; int value [[clang::guarded_by(*this)]]; }; void lock(Lockable *p) [[clang::exclusive_lock_function(*p)]]; // ...

Allowing the attributes to name the struct members isn't enough to model this; we need to allow the attributes to name the struct itself.

Such self-referential behavior is probably not too common, at least I can't remember seeing a record containing members guarded by the record itself. It seems preferable for numerous reasons to have the mutex/capability a separate member from what it guards. So I agree with @AaronBallman, we can likely go without explicit this or special syntax.

[tbaederr]

I've looked into this a bit and it's kind of a weird issue where the reason this is broken is super general (there is nothing like a this pointer in C), but the fix is really only needed in this one case (as far as I know).

I have locally added something like __builtin_instance_member(membername), which the thread safety analysis then translates using Ctx->SelfArg, in other words, the same thing it does for CXXThisPtr.

Would this be an acceptable solution or is that still too general?

Edit: Actually, doesn't a new attribute guarded_by_instance_ptr or similar make the most sense?

[AaronBallman]

I've looked into this a bit and it's kind of a weird issue where the reason this is broken is super general (there is nothing like a this pointer in C), but the fix is really only needed in this one case (as far as I know).

Not the only case. The -fbounds-safety proposal also wants to use field names as an attribute argument in C.

[aaronpuchert]

I have locally added something like __builtin_instance_member(membername), which the thread safety analysis then translates using Ctx->SelfArg, in other words, the same thing it does for CXXThisPtr.

Would this be an acceptable solution or is that still too general?

Deferring to others here, but to me it sounds reasonable.

Edit: Actually, doesn't a new attribute guarded_by_instance_ptr or similar make the most sense?

So in the initial example, instead of __attribute__((guarded_by(mu))), which is equivalent to __attribute__((guarded_by(this->mu))), we would write __attribute__((guarded_by_instance_ptr(mu)))? It's probably not quite as powerful as introducing this through the backdoor like the builtin would do, because then you could write arbitrary expressions involving it. But it might be powerful enough.