[llvm-bugs] [Bug 38904] thread-safety-analysis doesn't know about std::optional

[via llvm-bugs]

https://bugs.llvm.org/show_bug.cgi?id=38904

Aaron Puchert <aaronpuchert at alice-dsl.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |WONTFIX
                 CC|                            |aaronpuchert at alice-dsl.net
             Status|NEW                         |RESOLVED
          Component|-New Bugs                   |Frontend

--- Comment #1 from Aaron Puchert <aaronpuchert at alice-dsl.net> ---
The thread safety analysis works on the source-level control flow graph, so all
it sees is that we create a temporary of type AutoLock (B1.5), which acquires
g_lock, then pass it as Rvalue reference into emplace (B1.10), and then it's
destructed (B1.11), which is assumed to release g_lock. So by the time we reach
the next line, the analysis thinks the lock isn't held anymore.

[Add --analyze -Xanalyzer -analyzer-checker=debug.DumpCFG as compiler options]
void test2()
 [B2 (ENTRY)]
   Succs (1): B1
 [B1]
   1:  (CXXConstructExpr, [B1.2], std::optional<AutoLock>)
   2: std::optional<AutoLock> maybe_autolock;
   3: maybe_autolock
   4: [B1.3].emplace
   5: g_lock
   6: [B1.5] (CXXConstructExpr, [B1.7], [B1.9], class AutoLock)
   7: [B1.6] (BindTemporary)
   8: AutoLock([B1.7]) (CXXFunctionalCastExpr, ConstructorConversion, class
AutoLock)
   9: [B1.8]
  10: [B1.4]([B1.9])
  11: ~AutoLock() (Temporary object destructor)
  12: g_value
  13: 1
  14: [B1.12] += [B1.13]
  15: [B1.2].~optional() (Implicit destructor)
   Preds (1): B2
   Succs (1): B0
 [B0 (EXIT)]
   Preds (1): B1

The idea behind scoped capabilities is that they are not movable (and obviously
copyable) and live in function or block scope. We don't know what emplace does,
because this is on the source-level and no inlining has happened yet. Usually
this can be solved by annotating the functions that are called, but here this
is not an option.

I guess the code is using std::optional to do conditional locking, which isn't
supported anyway. [1] This is a "normal" warning and not part of the Clang
Static Analyzer, so we don't have a constraint solver that's telling us which
code paths are possible. "Because the analysis is not path-sensitive, it cannot
handle control-flow situations in which a mutex might or might not be held,
depending on which branch was taken. [...] Although this seems like a serious
limitation, we have found that conditionally held locks are relatively
unimportant in practical  programming. Reading or writing to a guarded location
in memory requires that the mutex be held unconditionally, so attempting to
track locks that might be held has little benefit in practice, and usually
indicates overly complex or poor-quality code." [2]

When trying to use the analysis on another large code basis, I found that code
using conditional locking is often easily refactored into code without
conditional locking, which is often better readable. So I'd recommend you try
that, because I don't think we can reasonably support this given the deliberate
limitations of the thread safety analysis.

[1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#conditional-locks
[2] Hutchins, Ballman, Sutherland: C/C++ Thread Safety Analysis
(https://static.googleusercontent.com/media/research.google.com/en/us/pubs/archive/42958.pdf)

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-bugs/attachments/20180918/ca177fe7/attachment.html>