Ericsson / clang

Cross Translation Unit analysis capability for Clang Static Analyzer. (Fork of official clang at http://llvm.org/git/clang)
http://clang.llvm.org/
Other
15 stars 10 forks source link

Reduce a minimal unit test for MemberExpr where we cannot import the FoundDecl #370

Closed martong closed 5 years ago

martong commented 6 years ago

There are cases in protobuf where we can't import a found decl of a MemberExpr. Interestingly, PR #369 fixes the error but does not results more "Functions already imported should have body." assert as expected.

This task is about to add the below assert and try to reduce to the minimal code where the assert still happens.

   auto *ToDecl =
       dyn_cast_or_null<NamedDecl>(Importer.Import(E->getFoundDecl().getDecl()));
+  assert(!E->getFoundDecl().getDecl() || ToDecl);
   if (!ToDecl && E->getFoundDecl().getDecl())
     return nullptr;
martong commented 6 years ago

Already tried the following tests, but none of them could fire the assert:

+TEST_P(ASTImporterTestBase, Maci) {
+  Decl *FromTU = getTuDecl(
+      R"(
+namespace NS0 { struct string{}; }
+namespace NS1 { struct string{}; }
+
+using namespace NS0;
+struct S {
+union {
+    int i;
+    string s;
+};
+};
+void f() { S So; So.s; }
+      )",
+      Lang_CXX, "input0.cc");
+  auto *FromD =
+      FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl(hasName("f")));
+
+  auto *ToD = Import(FromD, Lang_CXX);
+}
+
+TEST_P(ASTImporterTestBase, Maci2) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct A { void foo(){} };
+      struct B : A { void foo(){} };
+      void bar(B a) {
+        a.A::foo();
+      }
+      )",
+      Lang_CXX, "input0.cc");
+  auto *FromD =
+      FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl(hasName("bar")));
+
+  Import(FromD, Lang_CXX);
+}
martong commented 6 years ago

We don't get the "Functions already imported should have body." assertion because the function which is being imported by CTU does have a body.

Let's assume CTU imports foo:

void foo() {
  bar();
}
void bar() {
   X x;
   x.asdf; // Buggy MemberExpr;
}

Then bar will not have a body, but foo will have.

balazske commented 6 years ago

CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) contains an assert(FD->hasBody() && "Functions already imported should have body.");. I could not figure out why this assert is there at all. The passed function FD should be in the "From" context and must have a body by precondition. The findFunctionInDeclContext (its result is passed to importDefinition) can not return a function that has no body part.

martong commented 6 years ago

Agree, this would make more sense:

assert(ToDecl->hasBody() && "Functions already imported should have body.");
Xazax-hun commented 6 years ago

Hmm, I agree to change the assert.

balazske commented 6 years ago

The assert belongs into getCrossTUDefinition before return (non-error case). The same assert was originally there in clang5 branch version but was moved into a new function importDefinition. The FD in importDefinition is not the same as the FD in getCrossTUDefinition.

martong commented 5 years ago

Outdated.