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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D52398#1255218, @aaronpuchert wrote:

> In https://reviews.llvm.org/D52398#1255148, @hokein wrote:
>
> > Hi, @aaronpuchert, the patch has caused many new warnings in our internal codebase, below is a reduced one. Do you mind reverting the patch?
> >
> >   // if the condition is not true, CHECK will terminate the program.
> >   #define CHECK(condition) (__builtin_expect((condition), true)) ? 1 : 0
> >  
> >   void foo() {
> >       CHECK(mu.TryLock()); 
> >       mu.Unlock();
> >   }
> >
>


Thanks for looking into it!

> Even before there was another warning in this code: "releasing mutex 'mu' that was not held" on `mu.Unlock()`. Is there an example that didn't show any warnings before?

I think the warning of `Unlock` is not critical here, what matters is the warning on `CHECK(mu.TryLock()); `. The above example I provided may be not totally correct, I reduced it from a relatively complicated source. I'm confirm that the warning doesn't appear before the patch.

> And can you confirm that the warning also appeared before if you replace `__builtin_expect(x, *)` by `x`? Both are functionally equivalent.
>  I will have a closer look, but my first feeling is that the problem is //not// that we unwrap __builtin_expect, but instead that we don't consider the ternary operator `?:` in `getTrylockCallExpr`. I'll see if I can fix that.

Yeah, your feeling is right. The warning does appear if we replace `__builtin_expect` by `x` even before the patch. It seems that `getTrylockCallExpr` doesn't consider `?:`.

You patch is not the root cause of the issue, but our internal integration blocked by the warning, do you mind reverting this patch temporarily until we also fix the `getTrylockCallExpr`? Thanks very much for the understanding!


Repository:
  rL LLVM

https://reviews.llvm.org/D52398