[cfe-dev] [ASTImporter] Import order problems

[Aleksei Sidorin via cfe-dev]

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 пишет:
> 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


-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170907/24dee4de/attachment.html>