[clang][CFG] Fix assertion failure in checkIncorrectLogicOperator #142897

ziqingluo-90 · 2025-06-05T04:45:12Z

checkIncorrectLogicOperator checks if an expression, for example x != 0 || x != 1.0, is always true or false by comparing the two literals 0 and 1.0. But in case x is a 16-bit float, the two literals have distinct types---16-bit float and double, respectively. Directly comparing APValues extracted from the two literals results in an assertion failure because of their distinct types.

This commit fixes the issue by doing a conversion from the "smaller" one to the "bigger" one. The two literals must be compatible because both of them are comparing with x.

rdar://152456316

`checkIncorrectLogicOperator` checks if an expression, for example `x != 0 || x != 1.0`, is always true or false by comparing the two literals `0` and `1.0`. But in case `x` is a 16-bit float, the two literals have distinct types---16-bit float and double, respectively. Directly comparing `APValue`s extracted from the two literals results in an assertion failure because of their distinct types. This commit fixes the issue by doing a conversion from the "smaller" one to the "bigger" one. The two literals must be compatible because both of them are comparing with `x`. rdar://152456316

llvmbot · 2025-06-05T04:45:48Z

@llvm/pr-subscribers-clang-analysis

Author: Ziqing Luo (ziqingluo-90)

Changes

checkIncorrectLogicOperator checks if an expression, for example x != 0 || x != 1.0, is always true or false by comparing the two literals 0 and 1.0. But in case x is a 16-bit float, the two literals have distinct types---16-bit float and double, respectively. Directly comparing APValues extracted from the two literals results in an assertion failure because of their distinct types.

This commit fixes the issue by doing a conversion from the "smaller" one to the "bigger" one. The two literals must be compatible because both of them are comparing with x.

rdar://152456316

Full diff: https://github.com/llvm/llvm-project/pull/142897.diff

2 Files Affected:

(modified) clang/lib/Analysis/CFG.cpp (+22)
(added) clang/test/Sema/warn-unreachable_crash.cpp (+8)

diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 7f37a8b18d46e..c9610cc2888ad 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -1261,6 +1261,28 @@ class CFGBuilder {
         L2Result.Val.getKind() == APValue::Float) {
       llvm::APFloat L1 = L1Result.Val.getFloat();
       llvm::APFloat L2 = L2Result.Val.getFloat();
+      // Note that L1 and L2 do not necessarily have the same type.  For example
+      // `x != 0 || x != 1.0`, if `x` is a float16, the two literals `0` and
+      // `1.0` are float16 and double respectively.  In this case, we should do
+      // a conversion before comparing L1 and L2.  Their types must be
+      // compatible since they are comparing with the same DRE.
+      int8_t Order = Context->getFloatingTypeOrder(NumExpr1->getType(),
+                                                   NumExpr2->getType());
+      bool convertLoseInfo = false;
+
+      if (Order > 0) {
+        // type rank L1 > L2:
+        if (L2.convert(L1.getSemantics(), llvm::APFloat::rmNearestTiesToEven,
+                       &convertLoseInfo))
+          return {};
+      } else if (Order < 0)
+        // type rank L1 < L2:
+        if (L1.convert(L2.getSemantics(), llvm::APFloat::rmNearestTiesToEven,
+                       &convertLoseInfo))
+          return {};
+      if (convertLoseInfo)
+        return {}; // If the conversion loses info, bail
+
       llvm::APFloat MidValue = L1;
       MidValue.add(L2, llvm::APFloat::rmNearestTiesToEven);
       MidValue.divide(llvm::APFloat(MidValue.getSemantics(), "2.0"),
diff --git a/clang/test/Sema/warn-unreachable_crash.cpp b/clang/test/Sema/warn-unreachable_crash.cpp
new file mode 100644
index 0000000000000..7b23f30c6a214
--- /dev/null
+++ b/clang/test/Sema/warn-unreachable_crash.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -verify -Wunreachable-code %s
+
+static void test(__fp16& x) {
+  if (x != 0 || x != 1.0) { // expected-note{{}}
+      x = 0.9;
+    } else
+      x = 0.8; // expected-warning{{code will never be executed}}
+}

llvmbot · 2025-06-05T04:45:48Z

@llvm/pr-subscribers-clang

Author: Ziqing Luo (ziqingluo-90)

Changes

checkIncorrectLogicOperator checks if an expression, for example x != 0 || x != 1.0, is always true or false by comparing the two literals 0 and 1.0. But in case x is a 16-bit float, the two literals have distinct types---16-bit float and double, respectively. Directly comparing APValues extracted from the two literals results in an assertion failure because of their distinct types.

This commit fixes the issue by doing a conversion from the "smaller" one to the "bigger" one. The two literals must be compatible because both of them are comparing with x.

rdar://152456316

Full diff: https://github.com/llvm/llvm-project/pull/142897.diff

2 Files Affected:

(modified) clang/lib/Analysis/CFG.cpp (+22)
(added) clang/test/Sema/warn-unreachable_crash.cpp (+8)

diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 7f37a8b18d46e..c9610cc2888ad 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -1261,6 +1261,28 @@ class CFGBuilder {
         L2Result.Val.getKind() == APValue::Float) {
       llvm::APFloat L1 = L1Result.Val.getFloat();
       llvm::APFloat L2 = L2Result.Val.getFloat();
+      // Note that L1 and L2 do not necessarily have the same type.  For example
+      // `x != 0 || x != 1.0`, if `x` is a float16, the two literals `0` and
+      // `1.0` are float16 and double respectively.  In this case, we should do
+      // a conversion before comparing L1 and L2.  Their types must be
+      // compatible since they are comparing with the same DRE.
+      int8_t Order = Context->getFloatingTypeOrder(NumExpr1->getType(),
+                                                   NumExpr2->getType());
+      bool convertLoseInfo = false;
+
+      if (Order > 0) {
+        // type rank L1 > L2:
+        if (L2.convert(L1.getSemantics(), llvm::APFloat::rmNearestTiesToEven,
+                       &convertLoseInfo))
+          return {};
+      } else if (Order < 0)
+        // type rank L1 < L2:
+        if (L1.convert(L2.getSemantics(), llvm::APFloat::rmNearestTiesToEven,
+                       &convertLoseInfo))
+          return {};
+      if (convertLoseInfo)
+        return {}; // If the conversion loses info, bail
+
       llvm::APFloat MidValue = L1;
       MidValue.add(L2, llvm::APFloat::rmNearestTiesToEven);
       MidValue.divide(llvm::APFloat(MidValue.getSemantics(), "2.0"),
diff --git a/clang/test/Sema/warn-unreachable_crash.cpp b/clang/test/Sema/warn-unreachable_crash.cpp
new file mode 100644
index 0000000000000..7b23f30c6a214
--- /dev/null
+++ b/clang/test/Sema/warn-unreachable_crash.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -verify -Wunreachable-code %s
+
+static void test(__fp16& x) {
+  if (x != 0 || x != 1.0) { // expected-note{{}}
+      x = 0.9;
+    } else
+      x = 0.8; // expected-warning{{code will never be executed}}
+}

ziqingluo-90 · 2025-06-05T04:48:03Z

Godbolt example: https://godbolt.org/z/6dv3Kearx

CC @YutongZhuu as this issue was first introduced in YutongZhuu@9ae3368

yronglin · 2025-06-05T16:12:04Z

clang/lib/Analysis/CFG.cpp

+      // `1.0` are float16 and double respectively.  In this case, we should do
+      // a conversion before comparing L1 and L2.  Their types must be
+      // compatible since they are comparing with the same DRE.
+      int8_t Order = Context->getFloatingTypeOrder(NumExpr1->getType(),


Suggested change

int8_t Order = Context->getFloatingTypeOrder(NumExpr1->getType(),

int Order = Context->getFloatingTypeSemanticOrder(NumExpr1->getType(),

Why we use int8_t here? the return type of getFloatingTypeOrder is int.

good point, I initially thought it just returns -1,0, or 1 but it's not the case.

yronglin · 2025-06-05T16:14:52Z

clang/lib/Analysis/CFG.cpp

+        if (L1.convert(L2.getSemantics(), llvm::APFloat::rmNearestTiesToEven,
+                       &convertLoseInfo))
+          return {};
+      if (convertLoseInfo)


Seems we check APFloat::opStatus is enough. We already ignore loseInfo in constant evaluator

llvm-project/clang/lib/AST/ExprConstant.cpp

Line 2871 in f871466

St = Result.convert(Info.Ctx.getFloatTypeSemantics(DestType), RM, &ignored);

yronglin · 2025-06-05T16:17:25Z

clang/lib/Analysis/CFG.cpp

+
+      if (Order > 0) {
+        // type rank L1 > L2:
+        if (L2.convert(L1.getSemantics(), llvm::APFloat::rmNearestTiesToEven,


I think useif (L2.convert(L1.getSemantics(), llvm::APFloat::rmNearestTiesToEven, &convertLoseInfo) != APFloat::opOK) more clear, WDYT?

yronglin · 2025-06-05T16:20:28Z

Thank you working on this! Overall LGTM! But I don't think I'm qualified to make the final sign. CC @steakhal.
Could you also add an item in ReleaseNotes?

steakhal

I don't think I have much to say. I never really touched floating-point stuff.
I left the comments I had while reading though.

clang/test/Sema/warn-unreachable_crash.cpp

clang/lib/Analysis/CFG.cpp

yronglin

Thanks, LGTM!

ziqingluo-90 requested review from Fznamznon and Sirraide June 5, 2025 04:45

llvmbot added clang Clang issues not falling into any other category clang:analysis labels Jun 5, 2025

ziqingluo-90 requested review from haoNoQ, tbaederr and yronglin June 5, 2025 04:46

ziqingluo-90 requested a review from ahatanak June 5, 2025 05:17

add test

1729bb5

yronglin reviewed Jun 5, 2025

View reviewed changes

yronglin requested a review from steakhal June 5, 2025 16:20

steakhal reviewed Jun 5, 2025

View reviewed changes

address comments

43cc8cd

yronglin approved these changes Jun 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][CFG] Fix assertion failure in checkIncorrectLogicOperator #142897

[clang][CFG] Fix assertion failure in checkIncorrectLogicOperator #142897

ziqingluo-90 commented Jun 5, 2025

Uh oh!

llvmbot commented Jun 5, 2025

Uh oh!

llvmbot commented Jun 5, 2025

Uh oh!

ziqingluo-90 commented Jun 5, 2025

Uh oh!

yronglin Jun 5, 2025

Uh oh!

ziqingluo-90 Jun 6, 2025

Uh oh!

yronglin Jun 5, 2025

Uh oh!

yronglin Jun 5, 2025

Uh oh!

yronglin commented Jun 5, 2025

Uh oh!

steakhal left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yronglin left a comment

Uh oh!

Uh oh!

	int8_t Order = Context->getFloatingTypeOrder(NumExpr1->getType(),
	int Order = Context->getFloatingTypeSemanticOrder(NumExpr1->getType(),

[clang][CFG] Fix assertion failure in checkIncorrectLogicOperator #142897

Are you sure you want to change the base?

[clang][CFG] Fix assertion failure in checkIncorrectLogicOperator #142897

Conversation

ziqingluo-90 commented Jun 5, 2025

Uh oh!

llvmbot commented Jun 5, 2025

Uh oh!

llvmbot commented Jun 5, 2025

Uh oh!

ziqingluo-90 commented Jun 5, 2025

Uh oh!

yronglin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

ziqingluo-90 Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

yronglin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

yronglin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

yronglin commented Jun 5, 2025

Uh oh!

steakhal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yronglin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!