[cfe-dev] thread safety annotations false negative

[RJ Ryan via cfe-dev]

Ah, thanks a lot for the explanation DeLesley! Makes sense.

So, if I define get_foo_copy in terms of get_foo_reference, no safety
warnings can be given because the GUARDED_BY was casted away when returning
foo_ from get_foo_reference.

Is there some way to eliminate the implicit cast by making the types match?
This naive approach didn't work:

int GUARDED_BY(foo_lock_)& get_foo_reference() { return foo_; }

warning: 'guarded_by' attribute only applies to fields and global variables
[-Wignored-attributes]


On Wed, May 4, 2016 at 8:47 AM, Delesley Hutchins <delesley at google.com>
wrote:

>
> This is expected behavior.  Returning foo by reference does not involve an
> actual read from memory; the read doesn't happen until later, when the
> reference is used.  Thus there is no warning in get_foo_reference().
>
> If thread safety annotations were integrated with the C++ type system,
> then we could do better.  What's actually happening is an implicit type
> cast from a guarded reference (int GUARDED_BY(...)&) to an unguarded
> reference (int&), which is kind of like a cast from a const to a non-const
> pointer.  Unfortunately, the attributes are NOT part of the type, so it's
> difficult to check such "implicit casts" in a comprehensive manner.  If you
> turn on -Wthread-safety-reference, it will catch some cases, but not all.
> Trying to track implicit casts also involves a lot of false positives (much
> like const), so Wthread-safety-reference is not on by default.
>
> Moreover, even with better type information, you could still get false
> negatives.  Assume that the analysis issued a warning if it detected an
> implicit cast when the lock wasn't held.  You could still acquire a lock,
> grab the reference (no warning), release the lock, and then use the
> reference to write to the underlying memory.
>
> In order to fix THAT case, you need to have pointer lifetime analysis and
> escape analysis (a-la Rust), combined with a type system where the
> thread-safety attributes are built-in.  An implicit cast should return a
> temporary reference, that cannot escape the scope of the lock()/unlock().
> That level of analysis is well beyond the capabilities of the current
> implementation.
>
>   -DeLesley
>
>
> On Wed, Apr 27, 2016 at 5:36 PM, RJ Ryan via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> +cfe-dev -- in case anyone knows why this happens.
>>
>> Thanks!
>>
>> On Sat, Apr 23, 2016 at 1:41 PM, RJ Ryan <rryan at alum.mit.edu> wrote:
>>
>>> Hi there,
>>>
>>> The following code only produces thread safety warnings for me in
>>> get_foo_copy, not get_foo_reference. Is this expected? Sorry if this is a
>>> FAQ / known issue.
>>>
>>> $ clang --version
>>> Apple LLVM version 7.0.2 (clang-700.1.81)
>>> Target: x86_64-apple-darwin15.3.0
>>> Thread model: posix
>>>
>>> The problem also seems present on clang 3.8.0:
>>> https://godbolt.org/g/37DqNy
>>>
>>> Best,
>>> RJ
>>>
>>> (with the usual thread annotation definitions)
>>>
>>> class CAPABILITY("mutex") Mutex {
>>>   public:
>>>     Mutex() {}
>>>     inline void lock() ACQUIRE() {}
>>>     inline void unlock() RELEASE() {}
>>> };
>>>
>>> class Foo {
>>>   public:
>>>     int& get_foo_reference() {
>>>         return foo_;
>>>     }
>>>
>>>     int get_foo_copy() {
>>>         return foo_;
>>>     }
>>>
>>>   private:
>>>     Mutex foo_lock_;
>>>     int foo_ GUARDED_BY(foo_lock_);
>>> };
>>>
>>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>
>
>
> --
> DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160504/a3541e18/attachment.html>