[Clang][Analysis] Don't treat the default switch path as unreachable when all enumerators are covered #123166
Conversation
…erators are covered
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: Jeremy Rifkin (jeremy-rifkin) ChangesThis PR updates the CFG building logic and analysis based warning logic to not consider the default path for enum class E { a, b };
int foo(E e) {
switch(e) {
case E::a:
return 20;
case E::b:
return 30;
}
// reachable
}The motivation is here is to have A few test cases broke due to relying on missing returns in the default case. Two objective C test cases failed, which I would appreciate guidance on so that I can be sure to test the right things. On the llvm discord a point was raised about changes to this warning behavior maybe being disruptive for any codebases that rely heavily on this missing return analysis behavior. I think the fact so few tests are affected in llvm is a good sign, and similarly the fact other compilers warn about this makes any potential disruption limited. The two relevant commits introducing the enum covering logic: Closes #123153 Full diff: https://github.com/llvm/llvm-project/pull/123166.diff 6 Files Affected:
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 304bbb2b422c61..b819df35189bb8 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -4408,17 +4408,9 @@ CFGBlock *CFGBuilder::VisitSwitchStmt(SwitchStmt *Terminator) {
}
// If we have no "default:" case, the default transition is to the code
- // following the switch body. Moreover, take into account if all the
- // cases of a switch are covered (e.g., switching on an enum value).
- //
- // Note: We add a successor to a switch that is considered covered yet has no
- // case statements if the enumeration has no enumerators.
- bool SwitchAlwaysHasSuccessor = false;
- SwitchAlwaysHasSuccessor |= switchExclusivelyCovered;
- SwitchAlwaysHasSuccessor |= Terminator->isAllEnumCasesCovered() &&
- Terminator->getSwitchCaseList();
+ // following the switch body.
addSuccessor(SwitchTerminatedBlock, DefaultCaseBlock,
- !SwitchAlwaysHasSuccessor);
+ !switchExclusivelyCovered);
// Add the terminator and condition in the switch block.
SwitchTerminatedBlock->setTerminator(Terminator);
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 589869d0186575..2fa6f0a881e808 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -456,10 +456,7 @@ static ControlFlowKind CheckFallThrough(AnalysisDeclContext &AC) {
bool HasPlainEdge = false;
bool HasAbnormalEdge = false;
- // Ignore default cases that aren't likely to be reachable because all
- // enums in a switch(X) have explicit case statements.
CFGBlock::FilterOptions FO;
- FO.IgnoreDefaultsWithCoveredEnums = 1;
for (CFGBlock::filtered_pred_iterator I =
cfg->getExit().filtered_pred_start_end(FO);
diff --git a/clang/test/Analysis/cfg.cpp b/clang/test/Analysis/cfg.cpp
index 44a89df28e3b29..940994a7932192 100644
--- a/clang/test/Analysis/cfg.cpp
+++ b/clang/test/Analysis/cfg.cpp
@@ -178,7 +178,7 @@ namespace NoReturnSingleSuccessor {
// CHECK-NEXT: 1: x
// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
// CHECK-NEXT: 3: return [B1.2];
-// CHECK-NEXT: Preds (5): B3 B4 B5 B6 B2(Unreachable)
+// CHECK-NEXT: Preds (5): B3 B4 B5 B6 B2
// CHECK-NEXT: Succs (1): B0
// CHECK: [B2]
// CHECK-NEXT: 1: 0
@@ -188,7 +188,7 @@ namespace NoReturnSingleSuccessor {
// CHECK-NEXT: 5: [B2.4] (ImplicitCastExpr, IntegralCast, int)
// CHECK-NEXT: T: switch [B2.5]
// CHECK-NEXT: Preds (1): B7
-// CHECK-NEXT: Succs (5): B3 B4 B5 B6 B1(Unreachable)
+// CHECK-NEXT: Succs (5): B3 B4 B5 B6 B1
// CHECK: [B3]
// CHECK-NEXT: case D:
// CHECK-NEXT: 1: 4
@@ -254,14 +254,14 @@ int test_enum_with_extension(enum MyEnum value) {
// CHECK-NEXT: 5: [B2.4] (ImplicitCastExpr, IntegralCast, int)
// CHECK-NEXT: T: switch [B2.5]
// CHECK-NEXT: Preds (1): B7
-// CHECK-NEXT: Succs (4): B4 B5 B6 B3(Unreachable)
+// CHECK-NEXT: Succs (4): B4 B5 B6 B3
// CHECK: [B3]
// CHECK-NEXT: default:
// CHECK-NEXT: 1: 4
// CHECK-NEXT: 2: x
// CHECK-NEXT: 3: [B3.2] = [B3.1]
// CHECK-NEXT: T: break;
-// CHECK-NEXT: Preds (1): B2(Unreachable)
+// CHECK-NEXT: Preds (1): B2
// CHECK-NEXT: Succs (1): B1
// CHECK: [B4]
// CHECK-NEXT: case C:
diff --git a/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp b/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
index 611e1d8255976c..b791c527c2e33e 100644
--- a/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
@@ -383,6 +383,7 @@ class SwitchGuardedFieldsTest {
case Kind::V:
return -1;
}
+ halt();
}
int operator+() {
@@ -392,6 +393,7 @@ class SwitchGuardedFieldsTest {
case Kind::V:
return -1;
}
+ halt();
}
};
diff --git a/clang/test/Sema/return.c b/clang/test/Sema/return.c
index 14d5300e819492..b0814f152cbe95 100644
--- a/clang/test/Sema/return.c
+++ b/clang/test/Sema/return.c
@@ -265,7 +265,7 @@ int test_enum_cases(enum Cases C) {
case C4: return 3;
case C3: return 4;
}
-} // no-warning
+} // expected-warning {{non-void function does not return a value in all control paths}}
// PR12318 - Don't give a may reach end of non-void function warning.
int test34(int x) {
diff --git a/clang/test/SemaCXX/array-bounds.cpp b/clang/test/SemaCXX/array-bounds.cpp
index b584e1e7cd4537..3889a4bd0f4687 100644
--- a/clang/test/SemaCXX/array-bounds.cpp
+++ b/clang/test/SemaCXX/array-bounds.cpp
@@ -133,7 +133,7 @@ int test_pr9296() {
int test_sizeof_as_condition(int flag) {
int arr[2] = { 0, 0 }; // expected-note {{array 'arr' declared here}}
- if (flag)
+ if (flag)
return sizeof(char) != sizeof(char) ? arr[2] : arr[1];
return sizeof(char) == sizeof(char) ? arr[2] : arr[1]; // expected-warning {{array index 2 is past the end of the array (that has type 'int[2]')}}
}
@@ -170,16 +170,14 @@ void test_nested_switch() {
}
}
-// Test that if all the values of an enum covered, that the 'default' branch
-// is unreachable.
+// Test that a warning is not emitted if the code is unreachable.
enum Values { A, B, C, D };
void test_all_enums_covered(enum Values v) {
int x[2];
- switch (v) {
- case A: return;
- case B: return;
- case C: return;
- case D: return;
+ if(v == A) {
+ return;
+ } else {
+ return;
}
x[2] = 0; // no-warning
}
@@ -244,7 +242,7 @@ void test_pr10771() {
}
int test_pr11007_aux(const char * restrict, ...);
-
+
// Test checking with varargs.
void test_pr11007() {
double a[5]; // expected-note {{array 'a' declared here}}
|
|
Hmm... This isn't really my area of expertise, but I consider those types of warnings valuable. While it isn't technically 'unreachable', it IS by intent of the code. Is there perhaps some sort of annotation on the struct we have that we could skip this assumption on instead? Something that says "this enum might have values not in the enumerator list"? |
|
Hi Erich, thanks for taking a look. bool is_prime(int num) {
if(num < 0) {
std::unreachable();
}
}For such a function, the input On one hand I can see an argument for treating enums as special, however, from a safety standpoint I think it’s much better to not make any assumptions about valid enum values. Even setting flag enums and such aside, assuming enums can't take on abnormal values seems a very fickle assumption to bake into the compiler. Especially when C enums are involved, e.g. there is no warning on the following: enum E { a, b };
int foo(E e) {
switch(e) {
case E::a:
return 20;
case E::b:
return 30;
}
}I don't know of any existing annotation that could be used to indicate the intent of an enum to clang. While on one hand I can see value in annotating a flag enum is such, e.g., my preference would be to not annotate enums about intended use and rather annotate the uses about relevant assumptions (i.e. marking paths unreachable explicitly). Sometimes enums are used in ways they aren’t intended and it would be impossible to predict that at the point of declaration (whether this is good practice or not is a different question). I'd also prefer to follow existing good practice (warn about a potentially non-returning path) here rather than rely on adoption of a new practice (annotations about enum use). |
Hmm, I can think of two options here.
I do think it makes sense to require users to write int foo(Enum e) {
switch (e) {
case Enum::A: return 1;
// ...
}
std::unreachable();
}by default. At least that’s what I personally have been doing for a long time. Also, GCC, MSVC, and EDG all warn on this by default, which imo is an indication that this is considered a serious enough accidental error that we should do the same—and as someone who once spent far too much time debugging a program that was accidentally falling off the end of a function, I’d argue that it doesn’t happen often, but when it happens, it’s really hard to spot (and I at least would find myself thinking that ‘the compiler should really have warned about that’). |
jeremy-rifkin
changed the title
This PR updates the CFG building logic and analysis based warning logic to not consider the default path for
switchstatements to be unreachable when all enumerators are covered. I.e.:The motivation is here is to have
-Wreturn-typewarnings in cases such as above. Even though it's often sufficient to handle all enumerators, extraneous enum values are valid and common in the case of flag enums. More context at #123153. This is in-line with behavior in other compilers (gcc, msvc, and edg emit warnings for this).A few test cases broke due to relying on missing returns in the default case. One objective C test cases is failing, which I would appreciate guidance on so that I can be sure to test the right things.
On the llvm discord a point was raised about changes to this warning behavior maybe being disruptive for any codebases that rely heavily on this missing return analysis behavior. I think the fact so few tests are affected in llvm is a good sign, and similarly the fact other compilers warn about this makes any potential disruption limited.
The two relevant commits introducing the enum covering logic:
5020574
f69ce86
Closes #123153