[analyzer] Fix crash on compound literals with bitfields in unions #146418
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Baranov Victor (vbvictor) ChangesFix a crash in Fixes #146050. Full diff: https://github.com/llvm/llvm-project/pull/146418.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bee71cf296ac3..31cc160b8ae85 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1100,6 +1100,9 @@ Crash and bug fixes
- Fixed a crash in ``UnixAPIMisuseChecker`` and ``MallocChecker`` when analyzing
code with non-standard ``getline`` or ``getdelim`` function signatures. (#GH144884)
+- Fixed a crash when analyzing default bindings as compound literals in
+ designated initializers for bitfields in unions. (#GH146050)
+
Improvements
^^^^^^^^^^^^
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 388034b087789..288c53b375f28 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2187,7 +2187,10 @@ std::optional<SVal> RegionStoreManager::getBindingForDerivedDefaultValue(
// Lazy bindings are usually handled through getExistingLazyBinding().
// We should unify these two code paths at some point.
- if (isa<nonloc::LazyCompoundVal, nonloc::CompoundVal>(val))
+ // 'nonloc::ConcreteInt' values can arise from compound literals in
+ // designated initializers for bitfields in unions.
+ if (isa<nonloc::LazyCompoundVal, nonloc::CompoundVal, nonloc::ConcreteInt>(
+ val))
return val;
llvm_unreachable("Unknown default value");
diff --git a/clang/test/Analysis/fields.c b/clang/test/Analysis/fields.c
index 203c30c5960a1..66c882cdba1f3 100644
--- a/clang/test/Analysis/fields.c
+++ b/clang/test/Analysis/fields.c
@@ -113,6 +113,102 @@ void testBitfields(void) {
}
+struct BitfieldUnion {
+ union {
+ struct {
+ unsigned int addr : 22;
+ unsigned int vf : 1;
+ };
+ unsigned int raw;
+ };
+};
+
+struct BitfieldUnion processBitfieldUnion(struct BitfieldUnion r) {
+ struct BitfieldUnion result = r;
+ result.addr += 1;
+ return result;
+}
+
+void testBitfieldUnionCompoundLiteral(void) {
+ struct BitfieldUnion r1 = processBitfieldUnion((struct BitfieldUnion){.addr = 100, .vf = 1});
+ clang_analyzer_eval(r1.addr == 101); // expected-warning{{TRUE}}
+ clang_analyzer_eval(r1.vf == 1); // expected-warning{{UNKNOWN}}
+
+ struct BitfieldUnion r2 = processBitfieldUnion((struct BitfieldUnion){.addr = 1});
+ clang_analyzer_eval(r2.addr == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(r2.vf); // expected-warning{{UNKNOWN}}
+}
+
+struct NestedBitfields {
+ struct {
+ unsigned x : 16;
+ unsigned y : 16;
+ } inner;
+};
+
+struct NestedBitfields processNestedBitfields(struct NestedBitfields n) {
+ struct NestedBitfields tmp = n;
+ tmp.inner.x += 1;
+ return tmp;
+}
+
+void testNestedBitfields(void) {
+ struct NestedBitfields n1 = processNestedBitfields((struct NestedBitfields){.inner.x = 1});
+ clang_analyzer_eval(n1.inner.x == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(n1.inner.y == 0); // expected-warning{{TRUE}}
+
+ struct NestedBitfields n2 = processNestedBitfields((struct NestedBitfields){{200, 300}});
+ clang_analyzer_eval(n2.inner.x == 201); // expected-warning{{TRUE}}
+ clang_analyzer_eval(n2.inner.y == 300); // expected-warning{{TRUE}}
+}
+
+struct UnionContainerBitfields {
+ union {
+ unsigned val;
+ struct {
+ unsigned x : 16;
+ unsigned y : 16;
+ };
+ } u;
+};
+
+struct UnionContainerBitfields processUnionContainer(struct UnionContainerBitfields c) {
+ struct UnionContainerBitfields tmp = c;
+ tmp.u.x += 1;
+ return tmp;
+}
+
+void testUnionContainerBitfields(void) {
+ struct UnionContainerBitfields c1 = processUnionContainer((struct UnionContainerBitfields){.u.val = 100});
+ clang_analyzer_eval(c1.u.x == 101); // expected-warning{{FALSE}} // expected-warning{{TRUE}}
+
+ struct UnionContainerBitfields c2 = processUnionContainer((struct UnionContainerBitfields){.u.x = 100});
+ clang_analyzer_eval(c2.u.x == 101); // expected-warning{{TRUE}}
+}
+
+struct MixedBitfields {
+ unsigned char x;
+ unsigned y : 12;
+ unsigned z : 20;
+};
+
+struct MixedBitfields processMixedBitfields(struct MixedBitfields m) {
+ struct MixedBitfields tmp = m;
+ tmp.y += 1;
+ return tmp;
+}
+
+void testMixedBitfields(void) {
+ struct MixedBitfields m1 = processMixedBitfields((struct MixedBitfields){.x = 100, .y = 100});
+ clang_analyzer_eval(m1.x == 100); // expected-warning{{TRUE}}
+ clang_analyzer_eval(m1.y == 101); // expected-warning{{TRUE}}
+
+ struct MixedBitfields m2 = processMixedBitfields((struct MixedBitfields){.z = 100});
+ clang_analyzer_eval(m2.y == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(m2.z == 100); // expected-warning{{TRUE}}
+}
+
+
//-----------------------------------------------------------------------------
// Incorrect behavior
//-----------------------------------------------------------------------------
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Baranov Victor (vbvictor) ChangesFix a crash in Fixes #146050. Full diff: https://github.com/llvm/llvm-project/pull/146418.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bee71cf296ac3..31cc160b8ae85 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1100,6 +1100,9 @@ Crash and bug fixes
- Fixed a crash in ``UnixAPIMisuseChecker`` and ``MallocChecker`` when analyzing
code with non-standard ``getline`` or ``getdelim`` function signatures. (#GH144884)
+- Fixed a crash when analyzing default bindings as compound literals in
+ designated initializers for bitfields in unions. (#GH146050)
+
Improvements
^^^^^^^^^^^^
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 388034b087789..288c53b375f28 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2187,7 +2187,10 @@ std::optional<SVal> RegionStoreManager::getBindingForDerivedDefaultValue(
// Lazy bindings are usually handled through getExistingLazyBinding().
// We should unify these two code paths at some point.
- if (isa<nonloc::LazyCompoundVal, nonloc::CompoundVal>(val))
+ // 'nonloc::ConcreteInt' values can arise from compound literals in
+ // designated initializers for bitfields in unions.
+ if (isa<nonloc::LazyCompoundVal, nonloc::CompoundVal, nonloc::ConcreteInt>(
+ val))
return val;
llvm_unreachable("Unknown default value");
diff --git a/clang/test/Analysis/fields.c b/clang/test/Analysis/fields.c
index 203c30c5960a1..66c882cdba1f3 100644
--- a/clang/test/Analysis/fields.c
+++ b/clang/test/Analysis/fields.c
@@ -113,6 +113,102 @@ void testBitfields(void) {
}
+struct BitfieldUnion {
+ union {
+ struct {
+ unsigned int addr : 22;
+ unsigned int vf : 1;
+ };
+ unsigned int raw;
+ };
+};
+
+struct BitfieldUnion processBitfieldUnion(struct BitfieldUnion r) {
+ struct BitfieldUnion result = r;
+ result.addr += 1;
+ return result;
+}
+
+void testBitfieldUnionCompoundLiteral(void) {
+ struct BitfieldUnion r1 = processBitfieldUnion((struct BitfieldUnion){.addr = 100, .vf = 1});
+ clang_analyzer_eval(r1.addr == 101); // expected-warning{{TRUE}}
+ clang_analyzer_eval(r1.vf == 1); // expected-warning{{UNKNOWN}}
+
+ struct BitfieldUnion r2 = processBitfieldUnion((struct BitfieldUnion){.addr = 1});
+ clang_analyzer_eval(r2.addr == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(r2.vf); // expected-warning{{UNKNOWN}}
+}
+
+struct NestedBitfields {
+ struct {
+ unsigned x : 16;
+ unsigned y : 16;
+ } inner;
+};
+
+struct NestedBitfields processNestedBitfields(struct NestedBitfields n) {
+ struct NestedBitfields tmp = n;
+ tmp.inner.x += 1;
+ return tmp;
+}
+
+void testNestedBitfields(void) {
+ struct NestedBitfields n1 = processNestedBitfields((struct NestedBitfields){.inner.x = 1});
+ clang_analyzer_eval(n1.inner.x == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(n1.inner.y == 0); // expected-warning{{TRUE}}
+
+ struct NestedBitfields n2 = processNestedBitfields((struct NestedBitfields){{200, 300}});
+ clang_analyzer_eval(n2.inner.x == 201); // expected-warning{{TRUE}}
+ clang_analyzer_eval(n2.inner.y == 300); // expected-warning{{TRUE}}
+}
+
+struct UnionContainerBitfields {
+ union {
+ unsigned val;
+ struct {
+ unsigned x : 16;
+ unsigned y : 16;
+ };
+ } u;
+};
+
+struct UnionContainerBitfields processUnionContainer(struct UnionContainerBitfields c) {
+ struct UnionContainerBitfields tmp = c;
+ tmp.u.x += 1;
+ return tmp;
+}
+
+void testUnionContainerBitfields(void) {
+ struct UnionContainerBitfields c1 = processUnionContainer((struct UnionContainerBitfields){.u.val = 100});
+ clang_analyzer_eval(c1.u.x == 101); // expected-warning{{FALSE}} // expected-warning{{TRUE}}
+
+ struct UnionContainerBitfields c2 = processUnionContainer((struct UnionContainerBitfields){.u.x = 100});
+ clang_analyzer_eval(c2.u.x == 101); // expected-warning{{TRUE}}
+}
+
+struct MixedBitfields {
+ unsigned char x;
+ unsigned y : 12;
+ unsigned z : 20;
+};
+
+struct MixedBitfields processMixedBitfields(struct MixedBitfields m) {
+ struct MixedBitfields tmp = m;
+ tmp.y += 1;
+ return tmp;
+}
+
+void testMixedBitfields(void) {
+ struct MixedBitfields m1 = processMixedBitfields((struct MixedBitfields){.x = 100, .y = 100});
+ clang_analyzer_eval(m1.x == 100); // expected-warning{{TRUE}}
+ clang_analyzer_eval(m1.y == 101); // expected-warning{{TRUE}}
+
+ struct MixedBitfields m2 = processMixedBitfields((struct MixedBitfields){.z = 100});
+ clang_analyzer_eval(m2.y == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(m2.z == 100); // expected-warning{{TRUE}}
+}
+
+
//-----------------------------------------------------------------------------
// Incorrect behavior
//-----------------------------------------------------------------------------
|
There was a problem hiding this comment.
Could you help me with a Store for which the crash happened without the fix?
It would help me understand why do we have there a ConcreteInt, because I fear, if a ConcreteInt can get here, it's likely that other NonLocs can also reach this.
Anyways, LCVs and CompoundVals form one set of exception, and the ConcreteInt case is definitely something else thus shouldn't be handled by the same if but rather split into a separate if IMO.
There was a problem hiding this comment.
Could you help me with a Store for which the crash happened without the fix?
I'm not sure what do you mean by "a Store", minimal repro goes like this: https://godbolt.org/z/8z1vqf6Wv.
We get the crush as soon as try to get default binding when encountered a copied struct.
The fix may be a little "light-minded", I'll try to investigate more on the issue with other NonLocs that could get beside ConcreteInt. Also, can some deeper logic be broken, and we should never expect ConcreteInt right here?
There was a problem hiding this comment.
Sorry for my very late reply.
Could you help me with a Store for which the crash happened without the fix?
I'm not sure what do you mean by "a Store", minimal repro goes like this: https://godbolt.org/z/8z1vqf6Wv.
We get the crush as soon as try to get default binding when encountered a copied struct.
When you have the crash, walk up a couple stack frames until you see the ProgramState, frequently denoted by a variable named State. If you call State->dump() on it, you will have a couple things printed, including the Store and Environment (in other words, the memory and the registers of the abstract machine, respectively) along with some other stuff like checker state and the constraint system, but those are not relevant in this case.
I wanted to have a look at it when the crash happened.
The fix may be a little "light-minded", I'll try to investigate more on the issue with other NonLocs that could get beside ConcreteInt. Also, can some deeper logic be broken, and we should never expect ConcreteInt right here?
By the looks of the code from where this unreachable crash refers to in the godbolt link suggests to me that the code originally had no intention of handling other ConcreteInts than zero.
I think I managed to dump it, like this.
"store": { "pointer": "0x11a3f940", "items": [
{ "cluster": "SymRegion{conj_$0{int &, LC1, S945, #0}}", "pointer": "0x11930820", "items": [
{ "kind": "Direct", "offset": 0, "value": "0 S32b" }
]},
{ "cluster": "tmp", "pointer": "0x11a3fa50", "items": [
{ "kind": "Default", "offset": 0, "value": "1 S32b" }
]}
]}After thinking about this for a while, we should not have here a ConcreteInt of 1.
I bisected this crash to 9b2ec87 (#116840), thanks to manyclangs. And it starts to make sense now. Basically, in this particular case we want to copy a CompoundVal{CompoundVal{1}}, and I should have only elided one level of CompoundVal I think. And now, it collapses down to 1 thus we bind that as a default binding instead.
What I should have done in that patch was to check if the value is either of Symbol, Zero, Unknown, LazyCompoundVal, CompoundVal, to preserve the invariant that a default binding should only ever refer to one of these.
vbvictor
force-pushed
the
analyzer-fix-crush-146050
branch
from
239706f to
66593be
Compare
3 participants
Fix a crash in
RegionStore::getBindingForDerivedDefaultValuewhen analyzing compound literals with designated initializers for bitfields in unions. The crash occurred becausenonloc::ConcreteIntvalueswere not handled as possible default bindings.
Fixes #146050.