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

2018-10-05 Thread Jordan Rupprecht via Phabricator via cfe-commits
rupprecht added a comment. In https://reviews.llvm.org/D52398#1257133, @aaronpuchert wrote: > In https://reviews.llvm.org/D52398#1257092, @rupprecht wrote: > > > I patched the proposed fix-forward and it seems to have things building > > again. Is there an ETA on landing that? If it's going to

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

2018-10-05 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. In https://reviews.llvm.org/D52398#1257092, @rupprecht wrote: > I patched the proposed fix-forward and it seems to have things building > again. Is there an ETA on landing that? If it's going to take a bit, is there > any chance we could revert this patch until

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

2018-10-05 Thread Jordan Rupprecht via Phabricator via cfe-commits
rupprecht added a comment. In https://reviews.llvm.org/D52398#1255290, @aaronpuchert wrote: > @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

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

2018-10-04 Thread 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

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

2018-10-04 Thread 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] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

2018-10-04 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. 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

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

2018-10-04 Thread Haojian Wu via Phabricator via cfe-commits
hokein added a comment. 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),

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

2018-10-03 Thread Aaron Puchert via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL343681: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr (authored by aaronpuchert, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM

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

2018-10-02 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D52398#1251894, @aaronpuchert wrote: > @delesley Any objections to this? > > It's certainly useful for our code base, because our `assert`-like macros use > `__builtin_expect`, but I'm not sure if that applies to the general public. I

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

2018-10-01 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. @delesley Any objections to this? It's certainly useful for our code base, because our `assert`-like macros use `__builtin_expect`, but I'm not sure if that applies to the general public. Repository: rC Clang https://reviews.llvm.org/D52398

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

2018-09-24 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D52398#1244074, @aaronpuchert wrote: > No problem. Thanks for reviewing! I'm terribly sorry to be bombarding the two > of you with so many review requests lately, and I hope it'll soon be over. No apologies necessary -- I love and

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

2018-09-24 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added a comment. No problem. Thanks for reviewing! I'm terribly sorry to be bombarding the two of you with so many review requests lately, and I hope it'll soon be over. Repository: rC Clang https://reviews.llvm.org/D52398 ___

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

2018-09-24 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, but you should give @delesley a chance to weigh in before you commit. Repository: rC Clang https://reviews.llvm.org/D52398

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

2018-09-22 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added subscribers: cfe-commits, kristina. When people are really sure they'll get the lock they sometimes use __builtin_expect. It's also used by some assertion implementations. Asserting that