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

[Quuxplusone]

Bugzilla Link	PR20403
Status	NEW
Importance	P normal
Reported by	Matthew Dempsky (matthew@dempsky.org)
Reported on	2014-07-22 16:57:11 -0700
Last modified on	2021-05-04 10:38:04 -0700
Version	trunk
Hardware	PC Linux
CC	aaron@aaronballman.com, benvanik@google.com, delesley@google.com, lebedev.ri@gmail.com, llvm-bugs@lists.llvm.org, mikael.holmen@ericsson.com, nash@nash.id.au, richard-llvm@metafoo.co.uk, sstewartgallus00@mylangara.bc.ca
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

$ 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.

[Quuxplusone]

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?

[Quuxplusone]

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.

[Quuxplusone]

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?

[Quuxplusone]

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.

[Quuxplusone]

(In reply to Richard Smith from comment #2)
> 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.

[Quuxplusone]

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.)

[Quuxplusone]

(In reply to Richard Smith from comment #6)
> 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.