[ASTImporter] Import order problems

Hi!

I stumbled upon a serious issue within the AST Importer. First I will walk you through a very synthetic example, then I will try to explain what is happening and finally, I will ask for help if you have a proper solution in mind.

The description of the problem

1. Let’s have a simple struct (please ignore the UB in the struct) in a.cpp:
struct A {
int b = a + 2;
int a = 5;
};

2. Let’s also have an empty b.cpp.
3. Let’s dump the AST of a.cpp:
clang -cc1 -emit-pch -o 1.ast a.cpp -std=c++11

4. Let’s merge the dumped AST with the empty file and print the AST:
clang -cc1 -ast-merge 1.ast -ast-print b.cpp -std=c++11

The output is:
struct A {
int a = 5;
int b = this->a + 2;
};

Note that the order of the fields is changed. This is a serious problem since the imported type will not be equivalent to the original one and this can cause various problems during importing non-trivial AST. The same issue can be triggered without having UB in the code, but I wanted to keep the example as small as possible.

This importing strategy also has further implications. Such as sometimes the ASTImporter ends up checking for equivalence of half done and full definitions. (And they are, of course, not equivalent.)

Possible explanation of the problem

During importing a DeclContext we will add each of the declarations to it. The order of the declarations will be the order in which we add the declarations. In case a declaration is also a definition, we might end up importing the definitions as well before finishing the import of the actual declaration. If a definition refers to a declaration that was not imported yet, we will start importing that declaration.

In the example above the importer starts with importing A::b. During importing A::b it will encounter a DeclRefExpr to A::a, which is not imported yet. The importer will continue with importing A::a. After the import of A::a is finished, it will be added to the DeclContext before the import of A::b is finished, thus A::a end up before A::b in the imported class.

Possible solutions to the problem

One solution would be to emulate what the parser does, i.e.: first importing only the declarations and after all the declarations are imported continue with the definitions. Unfortunately, I do not see an easy way to implement this logic with the current structure of the ASTImporter, but hopefully, it is only because I am not very familiar with that part of clang.

To circumvent this issue I ended up reordering the declarations within the DeclContext after finishing the import of the definition based on the original DeclContext. This feels like a hacky solution for this problem and also do not solve further implications described above.

Asking for help

What do you think? Did you observe similar anomalies? Do you have any comments or a solution in mind?

Thanks in advance,
Gábor Horváth

Hello Gabor.

I'm aware of this issue. As a workaround, I reorder the fields of imported structure after it is imported. There is no need to reorder Decls inside other DeclContexts (at least, for analyzer; but this would make sense if ASTImporter is used together with Parser).
Thank you also for this awesome repro. When I'll be at least a bit more free, I'll put the patch on review.

07.09.2017 17:25, Gábor Horváth пишет: