Fix VarDecl Import #482

martong · 2018-08-23T14:08:00Z

Fixes #477

martong · 2018-08-23T14:12:42Z

lib/AST/ASTImporter.cpp

+    auto *Recent = const_cast<VarDecl *>(MergeWithVar->getMostRecentDecl());
+    updateAttrAndFlags(D, MergeWithVar);
+    ToVar->setPreviousDecl(Recent);
+  }

  // Merge the initializer.


This is not a "merge" rather an import.

balazske · 2018-08-23T16:22:13Z

unittests/AST/ASTImporterTest.cpp

+  ASSERT_TRUE(ToD0);
+  ASSERT_TRUE(ToD1);
+  EXPECT_NE(ToD0, ToD1);
+  EXPECT_EQ(ToD1->getPreviousDecl() ,ToD0);


Better variable names can be used: FromDWithInit, FromDWithDef and similar.

Yes, good point.

balazske · 2018-08-24T07:51:32Z

lib/AST/ASTImporter.cpp

@@ -1421,13 +1423,22 @@ bool ASTNodeImporter::ImportDefinition(VarDecl *From, VarDecl *To,
  if (To->getAnyInitializer())
    return false;



Really this function does not ensure the definition, only the initializer? (If "definition" is not the same as initializer.)

Yes, the definition may be independent from the initializer.
Perhaps a better name would be just ImportInitializer.

Renamed to ImportInitializer.

balazske · 2018-08-24T08:42:03Z

lib/AST/ASTImporter.cpp

-    LexicalDC->addDeclInternal(ToVar);
+  if (MergeWithVar) {
+    auto *Recent = const_cast<VarDecl *>(MergeWithVar->getMostRecentDecl());
+    updateAttrAndFlags(D, MergeWithVar);


The updateAttrAndFlags is used to update data (from D) in the found Decl. I think this should be called at the MapImported cases (line 3223 and 3229).

This will not work that way. Just tried and moved updateAttrAndFlags before the MapImported calls, and the unit test which test the used flag fails.
If we think about it this is logical, since we should update the used flag when we do create a new VarDecl, and that is happening once we reach to this line.
In those lines you mentioned (line 3223 and 3229) we do not create any new declaration, thus there is nothing to update.

Probably include it in both places? At create the used flag should be copied anyway. If an imported Decl is mapped to an existing, the imported may have the used flag set but the existing not, in this case the update is needed. Probably this case does not occur any more in the unit test because way of importing has changed (but the unit test originally was made to test the case when there is an existing VarDecl and an imported VarDecl is mapped to that and the used flag should be updated).

Okay, makes sense, I am going to check that and also the test.

So, I've written two new test cases which exercises those cases. And indeed it is needed to add updateAttrAndFlags there.
Then I realized that we have to do this always after we found an existing decl or even if we created a new one.
This seems like a general rule, so I added the call of updateAttrAndFlags into Import::(Decl*).

balazske · 2018-08-24T08:47:25Z

lib/AST/ASTImporter.cpp

@@ -3178,8 +3199,8 @@ Decl *ASTNodeImporter::VisitVarDecl(VarDecl *D) {

  // Try to find a variable in our own ("to") context with the same name and
  // in the same context as the variable we're importing.
+  VarDecl *MergeWithVar = nullptr;


This should be renamed to ExistingVar or similar.

Renamed to FoundByLookup, since that is how we call it in VisitFunctionDecl too.

martong · 2018-08-24T09:55:27Z

Added a new commit, so you can easily track what changed, before merge I'll squash.

balazske · 2018-08-24T10:27:23Z

lib/AST/ASTImporter.cpp

+            VarDecl *FoundDef = FoundVar->getDefinition();
+            if (D->isThisDeclarationADefinition() && FoundDef) {
+              return Importer.MapImported(D, FoundDef);
+            }


Is there no problem with update of initializer? If the D is definition and has initializer, and the found has no initializer, it is not updated. (But theoretically the variable can have only one definition, so the definition of D should be the same as the found one, with initializer.)

Hmm, good point, but I don't think that could be a problem exactly because what you have written in the parentheses.
I would not merge the initializer. Intstead, we could diagnose an ODR error here if we see that the two definitions have different initializers, I added a FIXME for that.

martong · 2018-08-30T15:09:18Z

run tests

martong · 2018-08-30T15:15:57Z

run tests

balazske · 2018-08-30T14:43:16Z

lib/AST/ASTImporter.cpp

@@ -3211,11 +3246,11 @@ Decl *ASTNodeImporter::VisitVarDecl(VarDecl *D) {
                return nullptr;


I am not sure if this array type import is needed with the new code. The FoundVar is updated (setType) but later we create a new VarDecl instead (and if a new VarDecl is created it should contain all new information, the existing should not be changed, no "merge" is done).
In line 3239 the getToContext is used with D that is in the FromContext, is this correct?

Yes, I agree, this comes from the "From" context.
Also, I created a new issue to revisit this code: #485

balazske · 2018-08-31T06:40:41Z

unittests/AST/ASTImporterTest.cpp

+    Decl *FromTU = getTuDecl(
+        "int x = 1; int f() { return x; }", Lang_CXX, "input1.cc");
+    auto *FromD =
+        FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());


This is good if we are sure that there is not any other hidden generated function in FromTU. Better is to match always with name.

Good catch, thank you.

martong · 2018-08-31T11:59:21Z

Update: squashed the commits into one.

martong · 2018-09-03T14:57:19Z

https://reviews.llvm.org/D51597

martong requested a review from balazske August 23, 2018 14:08

martong commented Aug 23, 2018

View reviewed changes

balazske reviewed Aug 23, 2018

View reviewed changes

balazske reviewed Aug 24, 2018

View reviewed changes

balazske reviewed Aug 30, 2018

View reviewed changes

balazske reviewed Aug 31, 2018

View reviewed changes

balazske approved these changes Aug 31, 2018

View reviewed changes

Fix import of VarDecl

5d2169a

martong force-pushed the fix_VarDecl branch from b354783 to 5d2169a Compare August 31, 2018 11:59

martong merged commit 4cf7f40 into Ericsson:ctu-clang6 Aug 31, 2018

martong added the ASTImporter label Sep 3, 2018

martong deleted the fix_VarDecl branch September 3, 2018 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VarDecl Import #482

Fix VarDecl Import #482

martong commented Aug 23, 2018

martong Aug 23, 2018

balazske Aug 23, 2018

martong Aug 24, 2018

balazske Aug 24, 2018

martong Aug 24, 2018

martong Aug 24, 2018

balazske Aug 24, 2018

martong Aug 24, 2018

balazske Aug 24, 2018

martong Aug 24, 2018

martong Aug 24, 2018

balazske Aug 24, 2018

martong Aug 24, 2018

martong commented Aug 24, 2018

balazske Aug 24, 2018

martong Aug 24, 2018 •

edited

Loading

martong commented Aug 30, 2018

martong commented Aug 30, 2018

balazske Aug 30, 2018

martong Aug 30, 2018

balazske Aug 31, 2018

martong Aug 31, 2018

martong commented Aug 31, 2018

martong commented Sep 3, 2018

		@@ -1421,13 +1423,22 @@ bool ASTNodeImporter::ImportDefinition(VarDecl From, VarDecl To,
		if (To->getAnyInitializer())
		return false;

		@@ -3211,11 +3246,11 @@ Decl ASTNodeImporter::VisitVarDecl(VarDecl D) {
		return nullptr;

Fix VarDecl Import #482

Fix VarDecl Import #482

Conversation

martong commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong commented Aug 24, 2018

Choose a reason for hiding this comment

martong Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

martong commented Aug 30, 2018

martong commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong commented Aug 31, 2018

martong commented Sep 3, 2018

martong Aug 24, 2018 •

edited

Loading