[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

@hokein Please have a look at https://reviews.llvm.org/D52888, maybe you can try it out already. The problem was that `?:` expressions are considered a branch point and when merging both branches the warning was emitted. Before this change, we couldn't look into `__builtin_expect` and so we didn't see that the result of `TryLock` is used to branch. Take this source:

  void foo() {
      if (mu.TryLock() ? 1 : 0)
        mu.Unlock();
  }

The CFG is:

  void foo()
   [B6 (ENTRY)]
     Succs (1): B5
  
   [B1]
     1: mu
     2: [B1.1].Unlock
     3: [B1.2]()
     Preds (1): B2
     Succs (1): B0
  
   [B2]
     1: [B5.3] ? [B3.1] : [B4.1]
     2: [B2.1] (ImplicitCastExpr, IntegralToBoolean, _Bool)
     T: if [B2.2]
     Preds (2): B3 B4
     Succs (2): B1 B0
  
   [B3]
     1: 1
     Preds (1): B5
     Succs (1): B2
  
   [B4]
     1: 0
     Preds (1): B5
     Succs (1): B2
  
   [B5]
     1: mu
     2: [B5.1].TryLock
     3: [B5.2]()
     T: [B5.3] ? ... : ...
     Preds (1): B6
     Succs (2): B3 B4
  
   [B0 (EXIT)]
     Preds (2): B1 B2

So we start in `B5`, then call `TryLock` and branch on `?:` into `B3` or `B4`. We find that in `B3` the lock is held and in `B4` it isn't, so when both merge into `B2` we complain that we don't know if the lock is held or not.

I think the proper solution is to not branch on conditionals, and rather introspect them when a "real" branch happens. There is a slight possibility that this might break other use cases, but I hope not.


Repository:
  rL LLVM

https://reviews.llvm.org/D52398