[clang] Fix of a crash 'Cannot get layout of forward declarations!' during CTU static analysis #155375
Conversation
…uring CTU static analysis When a type is imported with `ASTImporter`, the "original declaration" of the type is imported. In some cases this is not the definition (of the class). Before the fix the definition was only imported if there was an other reference to it in the AST to import. This is not always the case (like in the added test case), if not the definition was missing in the "To" AST which can cause the assertion later.
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesWhen a type is imported with Full diff: https://github.com/llvm/llvm-project/pull/155375.diff 3 Files Affected:
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 6299efaf6bbfc..b4c9cf699fd9c 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -1740,10 +1740,21 @@ ExpectedType ASTNodeImporter::VisitDeducedTemplateSpecializationType(
}
ExpectedType ASTNodeImporter::VisitTagType(const TagType *T) {
- Expected<TagDecl *> ToDeclOrErr = import(T->getOriginalDecl());
+ TagDecl *DeclForType = T->getOriginalDecl();
+ Expected<TagDecl *> ToDeclOrErr = import(DeclForType);
if (!ToDeclOrErr)
return ToDeclOrErr.takeError();
+ // If there is a definition of the 'OriginalDecl', it should be imported to
+ // have all information for the type in the "To" AST. (In rare cases no other
+ // reference may exist to the definition and it would not be imported
+ // otherwise.)
+ if (TagDecl *DefDecl = DeclForType->getDefinition()) {
+ Expected<TagDecl *> ToDefDeclOrErr = import(DefDecl);
+ if (!ToDefDeclOrErr)
+ return ToDefDeclOrErr.takeError();
+ }
+
if (T->isCanonicalUnqualified())
return Importer.getToContext().getCanonicalTagType(*ToDeclOrErr);
diff --git a/clang/test/Analysis/ctu-import-type-decl-definition.c b/clang/test/Analysis/ctu-import-type-decl-definition.c
new file mode 100644
index 0000000000000..4610ff8268555
--- /dev/null
+++ b/clang/test/Analysis/ctu-import-type-decl-definition.c
@@ -0,0 +1,43 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -emit-pch -o %t/import.ast %t/import.c
+
+// RUN: %clang_cc1 -analyze \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-config experimental-enable-naive-ctu-analysis=true \
+// RUN: -analyzer-config display-ctu-progress=true \
+// RUN: -analyzer-config ctu-dir=%t \
+// RUN: -verify %t/main.c
+
+//--- main.c
+
+// expected-no-diagnostics
+
+typedef struct X_s X_t;
+unsigned long f_import(struct X_s *xPtr);
+
+static void freeWriteFileResources(struct X_s *xPtr) {
+ f_import(xPtr);
+}
+
+//--- import.c
+
+typedef struct Y_s Y_t;
+
+struct Y_s {
+};
+
+struct X_s {
+ Y_t y;
+};
+
+unsigned long f_import(struct X_s *xPtr) {
+ if (xPtr != 0) {
+ }
+ return 0;
+}
+
+//--- externalDefMap.txt
+13:c:@F@f_import import.ast
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 15df9af889386..49bdc591dda92 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -10027,7 +10027,8 @@ struct ImportTemplateParmDeclDefaultValue
EXPECT_EQ(ToD->getPreviousDecl(), ToDInherited);
} else {
EXPECT_EQ(FromD, FromDInherited->getPreviousDecl());
- EXPECT_EQ(ToD, ToDInherited->getPreviousDecl());
+ // The order becomes reversed by the import process.
+ EXPECT_EQ(ToD->getPreviousDecl(), ToDInherited);
}
}
|
| } | ||
|
|
||
| //--- externalDefMap.txt | ||
| 13:c:@F@f_import import.ast |
There was a problem hiding this comment.
Could you please generate this in the RUN lines?
If we decide to update the USR generation, this would break.
As this test does not care about USR generation and its correctness, that would be just noise.
clang/lib/AST/ASTImporter.cpp
Outdated
| Expected<TagDecl *> ToDefDeclOrErr = import(DefDecl); | ||
| if (!ToDefDeclOrErr) |
There was a problem hiding this comment.
| Expected<TagDecl *> ToDefDeclOrErr = import(DefDecl); | |
| if (!ToDefDeclOrErr) | |
| if (Expected<TagDecl *> ToDefDeclOrErr = import(DefDecl); !ToDefDeclOrErr) |
|
BTW this made me think of lazy loaded decls. (see |
|
It is possible to set the |
|
Probably |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/26374 Here is the relevant piece of the build log for the reference |
@balazske, the test you added seems to not handle paths on MacOS with forward slashes in them correctly. It seems like sed might be trying to interpret them mistakenly instead of treating it as a path. Can you take a look and fix it so we can get the bot back to green? |
|
I can not test on MacOS so it takes longer to have a fix for it. |
…rd declarations!' during CTU static analysis" (#156010) Reverts llvm/llvm-project#155375 Test does not work correctly on MacOS.
|
I have figured out how (hopefully) fix the |
|
#156056 is the new PR (but I can not verify if this really works). |
When a type is imported with
ASTImporter, the "original declaration" of the type is imported. In some cases this is not the definition (of the class). Before the fix the definition was only imported if there was an other reference to it in the AST to import. This is not always the case (like in the added test case), if not the definition was missing in the "To" AST which can cause the assertion later.