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.)

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@194289 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/CodeGen/CGExprScalar.cpp b/lib/CodeGen/CGExprScalar.cpp
index f3fd60498ca76073f246818169ce11edf0e7bc66..f3a5387c58d3c7df7efdb05371f3cc9533212e96 100644 (file)
--- a/lib/CodeGen/CGExprScalar.cpp
+++ b/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<DeclRefExpr>(E))
-    if (const VarDecl *VD = dyn_cast<VarDecl>(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.
 }
 
 

diff --git a/test/CodeGenCXX/catch-undef-behavior2.cpp b/test/CodeGenCXX/catch-undef-behavior2.cpp
new file mode 100644 (file)
index 0000000..b8b31ca
--- /dev/null
+++ b/test/CodeGenCXX/catch-undef-behavior2.cpp
@@ -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;
+}