From 22e55a059097772d0294769622751a943b249ca2 Mon Sep 17 00:00:00 2001 From: Nick Lewycky Date: Fri, 8 Nov 2013 23:00:12 +0000 Subject: Remove an incorrect optimization inside Clang's IRGen. Its check to determine whether we can safely lower a conditional operator to select was insufficient. I've left a large comment in place to explaining the sort of problems that this transform can encounter in clang in the hopes of discouraging others from reimplementing it wrongly again in the future. (The test should also help with that, but it's easy to work around any single test I might add and think that your particular implementation doesn't miscompile any code.) llvm-svn: 194289 --- clang/lib/CodeGen/CGExprScalar.cpp | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) (limited to 'clang/lib/CodeGen/CGExprScalar.cpp') diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp index f3fd60498ca..f3a5387c58d 100644 --- a/clang/lib/CodeGen/CGExprScalar.cpp +++ b/clang/lib/CodeGen/CGExprScalar.cpp @@ -3023,22 +3023,15 @@ Value *ScalarExprEmitter::VisitBinComma(const BinaryOperator *E) { /// flow into selects in some cases. static bool isCheapEnoughToEvaluateUnconditionally(const Expr *E, CodeGenFunction &CGF) { - E = E->IgnoreParens(); - // Anything that is an integer or floating point constant is fine. - if (E->isEvaluatable(CGF.getContext())) - return true; - - // Non-volatile automatic variables too, to get "cond ? X : Y" where - // X and Y are local variables. - if (const DeclRefExpr *DRE = dyn_cast(E)) - if (const VarDecl *VD = dyn_cast(DRE->getDecl())) - if (VD->hasLocalStorage() && !(CGF.getContext() - .getCanonicalType(VD->getType()) - .isVolatileQualified())) - return true; - - return false; + return E->IgnoreParens()->isEvaluatable(CGF.getContext()); + + // Even non-volatile automatic variables can't be evaluated unconditionally. + // Referencing a thread_local may cause non-trivial initialization work to + // occur. If we're inside a lambda and one of the variables is from the scope + // outside the lambda, that function may have returned already. Reading its + // locals is a bad idea. Also, these reads may introduce races there didn't + // exist in the source-level program. } -- cgit v1.2.3