Incorrect thread safety analysis warning on accessing union member.

[hokein]

See godbolt

#define LOCKABLE                        __attribute__((lockable))
#define EXCLUSIVE_LOCK_FUNCTION(...)    __attribute__((exclusive_lock_function(__VA_ARGS__)))
#define UNLOCK_FUNCTION(...)            __attribute__((unlock_function(__VA_ARGS__)))
#define GUARDED_BY(x)                   __attribute__((guarded_by(x)))

class LOCKABLE Mutex {
 public:
  void Lock() EXCLUSIVE_LOCK_FUNCTION();
  void Unlock() UNLOCK_FUNCTION();
};

class Test {
  Mutex mu;

  union {
    int a GUARDED_BY(mu);
    int b GUARDED_BY(mu);
  } ;
  void test1() {  
    mu.Lock();
    a = 0;   // unexpected warning: writing variable 'a' requires holding mutex 'mu' exclusively
    mu.Unlock();
  }
};

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Haojian Wu (hokein)

See [godbolt](https://godbolt.org/z/bc81b64ej)

#define LOCKABLE                        __attribute__((lockable))
#define EXCLUSIVE_LOCK_FUNCTION(...)    __attribute__((exclusive_lock_function(__VA_ARGS__)))
#define UNLOCK_FUNCTION(...)            __attribute__((unlock_function(__VA_ARGS__)))
#define GUARDED_BY(x)                   __attribute__((guarded_by(x)))

class LOCKABLE Mutex {
 public:
  void Lock() EXCLUSIVE_LOCK_FUNCTION();
  void Unlock() UNLOCK_FUNCTION();
};

class Test {
  Mutex mu;

  union {
    int a GUARDED_BY(mu);
    int b GUARDED_BY(mu);
  } ;
  void test1() {  
    mu.Lock();
    a = 0;   // unexpected warning: writing variable 'a' requires holding mutex 'mu' exclusively
    mu.Unlock();
  }
};

[shafik]

Based on 5ff1644

it looks like we have precise diagnostics controlled -Wthread-safety-precise and -Wthread-safety is considered imprecise b/c it gets confused by aliasing. In this case I guess b/c they are members of a union.

So this feels like expected behavior.

[hokein]

Thanks for the link!

it looks like we have precise diagnostics controlled -Wthread-safety-precise and -Wthread-safety is considered imprecise b/c it gets confused by aliasing. In this case I guess b/c they are members of a union.

Yeah, -Wthread-safety-precise is stricter than the imprecise -Wthread-safety, but for this particular case, -Wthread-safety-precise seems better -- it doesn't emit the false positive.

I guess we could restruct the union structure to union { int a; int b;} my_union GUARDED_BY(mu); as a workaround, but accessing the member is more verbose my_union.a.

So this feels like expected behavior.

It looks like analysis not working well on aliases is a known issue/limitation, and is probably hard to fix.

(I keep this issue opened, as this is a false positive for aliases, it would be great to have it fixed).

[ilya-biryukov]

I believe the analysis simply does not handle members of the anonymous structs and unions well. E.g. there is no aliasing involved in this example:

class Test {
  Mutex mu;

  struct {
    int a GUARDED_BY(mu);
    int b GUARDED_BY(mu);
  };
  void test1() {  
    mu.Lock();
    a = 0;   // unexpected warning: writing variable 'a' requires holding mutex 'mu' exclusively
    mu.Unlock();
  }
};

https://godbolt.org/z/8fdxrxzdr

It is (roughly) equivalent to this code that does not show any warnings:

class Test {
  Mutex mu;

  int a GUARDED_BY(mu);
  int b GUARDED_BY(mu);
  
  void test1() {  
    mu.Lock();
    a = 0;   // unexpected warning: writing variable 'a' requires holding mutex 'mu' exclusively
    mu.Unlock();
  }
};

https://godbolt.org/z/a8397hK4P

[hokein]

Good catch. Even for regular nested structures, it doesn't work.

class Test {
  Mutex mu;

  struct ab {
    int a GUARDED_BY(mu);
    int b GUARDED_BY(mu);
  };

  ab ab_;
  void test1() {  
    mu.Lock();
    ab_.a = 0;   // unexpected warning: writing variable 'a' requires holding mutex 'mu' exclusively
    mu.Unlock();
  }
};

Interestingly, If we moved everything to namespace scope rather than Test structure scope, everything works as expected, https://godbolt.org/z/T578qqEve

[aaronpuchert]

The nested union has no way of referring to non-static members of the parent class, because semantically a nested record type doesn't get access to this of the parent record type. The original example is the special case of anonymous nested record types with implicit FieldDecl, and while in that case it would seem like every member of the nested record corresponds to a parent record, that is not true for anonymous nested record types in general. As soon as the FieldDecl is no longer implicit, we can get the type via decltype and create additional instances outside of the parent record.

We could special-case implicit fields of anonymous record types and allow them access to their parent's this, but I feel this would be working against our current modelling in the AST and might cause issues in the future. The AST models the nested record type as a regular anonymous record type and only adds IndirectFieldDecls to the parent. Further, there is actually a MemberExpr for the implicit projection onto the nested record type.

TranslationUnitDecl
[...]
`-CXXRecordDecl 0x55b3d5964680 class Test definition
  [...]
  |-FieldDecl 0x55b3d5964870 referenced mu 'Mutex'
  |-CXXRecordDecl 0x55b3d59648c0 union definition
  | |-[...]
  | |-FieldDecl 0x55b3d59649f0 referenced a 'int'
  | | `-GuardedByAttr
  | |   `-DeclRefExpr 'Mutex' lvalue Field 0x55b3d5964870 'mu' 'Mutex' non_odr_use_unevaluated
  | `-FieldDecl 0x55b3d5964a58 b 'int'
  |   `-GuardedByAttr
  |     `-DeclRefExpr 'Mutex' lvalue Field 0x55b3d5964870 'mu' 'Mutex' non_odr_use_unevaluated
  |-FieldDecl 0x55b3d5964b18 implicit referenced 'Test::(anonymous union at <stdin>:15:3)'
  |-IndirectFieldDecl 0x55b3d5964b78 implicit a 'int'
  | |-Field 0x55b3d5964b18 '' 'Test::(anonymous union at <stdin>:15:3)'
  | `-Field 0x55b3d59649f0 'a' 'int'
  |-IndirectFieldDecl 0x55b3d5964bd0 implicit b 'int'
  | |-Field 0x55b3d5964b18 '' 'Test::(anonymous union at <stdin>:15:3)'
  | `-Field 0x55b3d5964a58 'b' 'int'
  `-CXXMethodDecl 0x55b3d5964c70 test1 'void ()' implicit-inline
    `-CompoundStmt
      |-[...]
      |-BinaryOperator 'int' lvalue '='
      | |-MemberExpr 'int' lvalue .a 0x55b3d59649f0
      | | `-MemberExpr 'Test::(anonymous union at <stdin>:15:3)' lvalue ->Test::(anonymous union at <stdin>:15:3) 0x55b3d5964b18
      | |   `-CXXThisExpr 'Test *' implicit this
      | `-IntegerLiteral 'int' 0
      `-[...]

If there is any way to extract such a nested record (possibly by reference) from its parent, we get into trouble because we lose the parent's fields. Furthermore, we might need to invent an operation to go from this of the nested type to this of the parent type, or ignore the MemberExpr and hope that everything works out.

What I'd favor instead is putting the attribute on the (possibly implicit) struct member in the parent class. I think we should do this:

1. Disallow referring to non-static members of parent classes, because we don't have this for them. While you cannot add member functions to anonymous record types with an implicit field, it is possible with an explicit FieldDecl, and we don't allow referring to parent fields in that case: for a nested union { Mutex& getMutex() { return mu; } } x; we produce the error "use of non-static data member 'mu' of 'Test' from nested type '(unnamed union at ...)'". I think we should produce the same error when referring to members in attributes.
2. Allow placing the attribute on an anonymous nested record type with implicit FieldDecl. It should then apply to that FieldDecl, and thus warn on (implicit) projection onto that field. The latter part should probably work already, since the AST has a MemberExpr for this projection, as we saw.

For now we can focus on 2. First question would be: is that good enough for you, or do you see a use case for annotating individual nested record members individually? The second question: currently we warn on

  union {
    int a;
    int b;
  } GUARDED_BY(mu);

that the "'guarded_by' attribute only applies to non-static data members and global variables [-Wignored-attributes]". Can we syntactically attach it to the (implicit) non-static data member? Maybe @AaronBallman can chime on this.

[zygoloid]

See also CWG2767, which specifies that for

struct A {
  [[some_attr]] union { ... };
};

the attribute appertains to the implicit unnamed non-static data member of union type. That doesn't necessarily imply that's how GNU attributes should behave, but that seems logical.

[AaronBallman]

Can we syntactically attach it to the (implicit) non-static data member? Maybe @AaronBallman can chime on this.

This makes sense to me, especially due to the direction CWG2767 is potentially going.

the attribute appertains to the implicit unnamed non-static data member of union type. That doesn't necessarily imply that's how GNU attributes should behave, but that seems logical.

I think that also seems logical, but it would make me more comfortable if GCC was planning to do the same as well.

CC @erichkeane in case his opinion differs.