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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
