r194289 - Remove an incorrect optimization inside Clang's IRGen. Its check to determine

[Nick Lewycky]

Author: nicholas
Date: Fri Nov  8 17:00:12 2013
New Revision: 194289

URL: http://llvm.org/viewvc/llvm-project?rev=194289&view=rev
Log:
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.)

Added:
    cfe/trunk/test/CodeGenCXX/catch-undef-behavior2.cpp
Modified:
    cfe/trunk/lib/CodeGen/CGExprScalar.cpp

Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=194289&r1=194288&r2=194289&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprScalar.cpp Fri Nov  8 17:00:12 2013
@@ -3023,22 +3023,15 @@ Value *ScalarExprEmitter::VisitBinComma(
 /// 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<DeclRefExpr>(E))
-    if (const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl()))
-      if (VD->hasLocalStorage() && !(CGF.getContext()
-                                     .getCanonicalType(VD->getType())
-                                     .isVolatileQualified()))
-        return true;
+  return E->IgnoreParens()->isEvaluatable(CGF.getContext());
 
-  return false;
+  // 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.
 }
 
 

Added: cfe/trunk/test/CodeGenCXX/catch-undef-behavior2.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/catch-undef-behavior2.cpp?rev=194289&view=auto
==============================================================================
--- cfe/trunk/test/CodeGenCXX/catch-undef-behavior2.cpp (added)
+++ cfe/trunk/test/CodeGenCXX/catch-undef-behavior2.cpp Fri Nov  8 17:00:12 2013
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -std=c++11 -fsanitize=signed-integer-overflow,integer-divide-by-zero,float-divide-by-zero,shift,unreachable,return,vla-bound,alignment,null,vptr,object-size,float-cast-overflow,bool,enum,array-bounds,function -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+
+bool GetOptionalBool(bool *value);
+bool GetBool(bool default_value) {
+  // CHECK-LABEL: @_Z7GetBoolb
+  // CHECK-NOT: select
+  bool value;
+  return GetOptionalBool(&value) ? value : default_value;
+}