[analyzer] Canonicalize the Decls of FieldRegions #156668
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When calculating the offset of a FieldRegion, we need to find out which field index the given field refers to. Previously, if for some reason the field was not found, then the `Idx` passed to `Layout.getFieldOffset` was out of bounds and caused undefined behavior when dereferenced an out of bounds element in `ASTVector::FieldOffsets::operator[]`, which asserts this in debug builds, but exposes the undefined behavior in release builds. In this patch, I refactored how we enumerate the fields, and gracefully handle the scenario where the field is not found. That case is still bad, but at least it should not expose the undefined behavior in release builds, and should assert earlier in debug builds than before. The motivational case was transformed into a regression test, that would fail if no canonicalization would happen when creating a FieldRegion. This was reduced from a production crash. In the test case, due to how modules work, there would be multiple copies of the same template specialization in the AST. This could lead into inconsistent state when the FieldRegion's Decl was different to the RecordDecl's field - because one referred to the first and the other to the second. This made `calculateOffset` fail to compute the field index, triggering the undefined behavior in production. While this inconsistency gets fixed now, I think the assertion is still warranted to avoid potential undefined behavior in release builds. CPP-6691,CPP-6849 Co-authored-by: Marco Borgeaud <marco.borgeaud@sonarsource.com>
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Benics (balazs-benics-sonarsource) ChangesWhen calculating the offset of a FieldRegion, we need to find out which field index the given field refers to. In this patch, I refactored how we enumerate the fields, and gracefully handle the scenario where the field is not found. The motivational case was transformed into a regression test, that would fail if no canonicalization would happen when creating a FieldRegion. This was reduced from a production crash. While this inconsistency gets fixed now, I think the assertion is still warranted to avoid potential undefined behavior in release builds. CPP-6691,CPP-6849 Full diff: https://github.com/llvm/llvm-project/pull/156668.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
index 5f271963e3d09..f68fb1e6df759 100644
--- a/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ b/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1271,7 +1271,7 @@ const SymbolicRegion *MemRegionManager::getSymbolicHeapRegion(SymbolRef Sym) {
const FieldRegion*
MemRegionManager::getFieldRegion(const FieldDecl *d,
const SubRegion* superRegion){
- return getSubRegion<FieldRegion>(d, superRegion);
+ return getSubRegion<FieldRegion>(d->getCanonicalDecl(), superRegion);
}
const ObjCIvarRegion*
@@ -1704,16 +1704,23 @@ static RegionOffset calculateOffset(const MemRegion *R) {
if (SymbolicOffsetBase)
continue;
- // Get the field number.
- unsigned idx = 0;
- for (RecordDecl::field_iterator FI = RD->field_begin(),
- FE = RD->field_end(); FI != FE; ++FI, ++idx) {
- if (FR->getDecl() == *FI)
- break;
+ auto MaybeFieldIdx = [FR, RD]() -> std::optional<unsigned> {
+ assert(FR->getDecl()->getCanonicalDecl() == FR->getDecl());
+ for (auto [Idx, Field] : llvm::enumerate(RD->fields())) {
+ if (FR->getDecl() == Field->getCanonicalDecl())
+ return Idx;
+ }
+ return std::nullopt;
+ }();
+
+ if (!MaybeFieldIdx.has_value()) {
+ assert(false && "Field not found");
+ goto Finish; // Invalid offset.
}
+
const ASTRecordLayout &Layout = R->getContext().getASTRecordLayout(RD);
// This is offset in bits.
- Offset += Layout.getFieldOffset(idx);
+ Offset += Layout.getFieldOffset(MaybeFieldIdx.value());
break;
}
}
diff --git a/clang/test/Analysis/modules/explicit-templ-inst-crash-in-modules.cppm b/clang/test/Analysis/modules/explicit-templ-inst-crash-in-modules.cppm
new file mode 100644
index 0000000000000..6eec29c7187ba
--- /dev/null
+++ b/clang/test/Analysis/modules/explicit-templ-inst-crash-in-modules.cppm
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// DEFINE: %{common-flags}= -std=c++20 -I %t -fprebuilt-module-path=%t
+//
+// RUN: %clang_cc1 %{common-flags} %t/other.cppm -emit-module-interface -o %t/other.pcm
+// RUN: %clang_analyze_cc1 -analyzer-checker=core %{common-flags} %t/entry.cppm -verify
+
+
+//--- MyStruct.h
+template <typename T> struct MyStruct {
+ T data = 0;
+};
+template struct MyStruct<int>; // Explicit template instantiation.
+
+//--- other.cppm
+module;
+#include "MyStruct.h"
+export module other;
+static void implicit_instantiate_MyStruct() {
+ MyStruct<int> var;
+ (void)var;
+}
+
+//--- entry.cppm
+// expected-no-diagnostics
+module;
+#include "MyStruct.h"
+module other;
+
+void entry_point() {
+ MyStruct<int> var; // no-crash
+ (void)var;
+}
|
NagyDonat
approved these changes
Sep 3, 2025
NagyDonat
reviewed
Sep 3, 2025
NagyDonat
approved these changes
Sep 3, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/26685 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When calculating the offset of a FieldRegion, we need to find out which field index the given field refers to.
Previously, if for some reason the field was not found, then the
Idxpassed toLayout.getFieldOffsetwas out of bounds and caused undefined behavior when dereferenced an out of bounds element inASTVector::FieldOffsets::operator[], which asserts this in debug builds, but exposes the undefined behavior in release builds.In this patch, I refactored how we enumerate the fields, and gracefully handle the scenario where the field is not found.
That case is still bad, but at least it should not expose the undefined behavior in release builds, and should assert earlier in debug builds than before.
The motivational case was transformed into a regression test, that would fail if no canonicalization would happen when creating a FieldRegion. This was reduced from a production crash.
In the test case, due to how modules work, there would be multiple copies of the same template specialization in the AST. This could lead into inconsistent state when the FieldRegion's Decl was different to the RecordDecl's field - because one referred to the first and the other to the second. This made
calculateOffsetfail to compute the field index, triggering the undefined behavior in production.While this inconsistency gets fixed now, I think the assertion is still warranted to avoid potential undefined behavior in release builds.
CPP-6691,CPP-6849