<div dir="ltr"><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Hi Aleksei,</span><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">I've created the patches for the two major issues mentioned in the previous mail. They are not small, but I think they are within a reasonable size (less than 1000 lines of change).</div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">Sorry about the storm of patches lately, but only now we had the time window to opensource the changes from our fork. </div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">We still have a few changes, but they are rather smaller ones, Balazs will probably create a few more patches next week (you can follow our plan for the upstreaming here: <a href="https://github.com/Ericsson/clang/projects/2">https://github.com/Ericsson/clang/projects/2</a> )</div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">I won't be available for the next two weeks but I'll try to check emails occasionally and will try to answer your questions and concerns.</div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">> <span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">- Do the same things after and before a new AST node is created (::Create)</span></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span style="text-align:start;text-indent:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><a href="https://reviews.llvm.org/D47632">https://reviews.llvm.org/D47632</a><br></span></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">> - <span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Decl chains are not imported</span></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span style="text-align:start;text-indent:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><a href="https://reviews.llvm.org/D47532">https://reviews.llvm.org/D47532</a><br></span></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Cheers,</span></div><div style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Gabor</span></div><br></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Apr 26, 2018 at 11:39 AM Gábor Márton <<a href="mailto:martongabesz@gmail.com">martongabesz@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Aleksei,<br>
<br>
It is a great and welcoming initiative to identify the weaknesses in<br>
the ASTImporter.<br>
<br>
About, handling the error case after a call:<br>
I couldn't agree more that this is a burden to always have a check at<br>
the call site, but this is the price we pay for not having exceptions.<br>
By using macros I have a feeling that the code would be even less<br>
readable because we would hide control flow modifications inside the<br>
macro, also because finally we'd need to create so many different<br>
kinds of macros.<br>
As far as I know, there are cases where an `Import` can return a<br>
`nullptr` or a default even in case of non-error situations, please<br>
correct me if I am wrong. Thus, introducing an optional like return<br>
type would require to discover all these cases. Still, I think this<br>
would worth it, because the enforcement of error checking (as you<br>
mentioned), plus it would make it more explicit which situation is<br>
really an error.<br>
Actually, I think this should be done with a few large commits, e.g.<br>
once we change the return type of `ASTImporter::Import(Decl*)` to<br>
`Optional<Decl*>` then we have to change all call sites.<br>
By introducing an `ImportOrError` and then doing the changes gradually<br>
would result the same final state as if we had done it in one larger<br>
step where we changed all call sites, but until all call sites are<br>
changed we have to maintain both `Import` and `ImportOrError`.<br>
<br>
<br>
Beside the problem of handling the error case after an `Import` call<br>
we have observed several other weaknesses, and would like to hear your<br>
opinion about them as well.<br>
So, the list of problems so far:<br>
<br>
- Do the same things after and before a new AST node is created (::Create)<br>
The original import logic should be really simple, we create the node,<br>
then we mark that as imported, then we recursively import the parts<br>
for that node and then set them on that node.<br>
However, the AST is actually a graph, so we have to handle circles.<br>
If we mark something as imported (Imported()) then we return with the<br>
corresponding To whenever we want to import that node again, this way<br>
circles are handled.<br>
In order to make this algorithm work we must ensure things, which are<br>
not enforced in the current ASTImporter:<br>
* There are no Import() calls in between any node creation (::Create)<br>
and the Imported() call.<br>
* There are no VisitXXX() calls in any other VisitYYY() function, only<br>
call of Import() allowed.<br>
* Before actually creating an AST node (::Create), we must check if<br>
the Node had been imported already, if yes then return with that one.<br>
One very important case for this is connected to templates: we may<br>
start an import both from the templated decl of a template and from<br>
the template itself.<br>
There are good news, the first and third points can be enforced by<br>
providing a variadic forwarding template for the creation, we are<br>
currently preparing to upstream this change from our fork<br>
(<a href="https://github.com/Ericsson/clang/pull/354/files" rel="noreferrer" target="_blank">https://github.com/Ericsson/clang/pull/354/files</a>) but this is going<br>
to be a huge change.<br>
Still, I have no idea how to enforce the second point. (Maybe a checker?)<br>
<br>
- Decl chains are not imported<br>
Currently we import declarations only if there is no available definition.<br>
If in the "From" TU there are both a declaration and a definition we<br>
import only the definition.<br>
Thus we do not import all information from the original AST.<br>
One example:<br>
struct B { virtual void f(); };<br>
void B::f() {}<br>
Here, when we import the definition, then the virtual flag is not<br>
getting imported, so finally we import a function as a non-virtual<br>
function.<br>
An other problem is with recursive functions:<br>
void f(); void f() { f(); }<br>
When we import the prototype then we end up having two identical<br>
definitions for f(), and no prototype.<br>
We again preparing a patch for this, but this solves the problem only<br>
in case of functions.<br>
By not importing the whole decl chain we loose all attributes which<br>
may be present only in declarations.<br>
<br>
- Structural Equivalency<br>
There are several holes here. E.g. we do not check the member<br>
functions in a class for equivalency, only members.<br>
Thus, we may end up stating that a forward decl is equivalent with a<br>
class definition if the class does not have data members.<br>
There are no tests at all for structural equivalency.<br>
In our fork we have some tests, and planning to upstream them too.<br>
<br>
- Minimal import<br>
This feature is used by LLDB, but not at all in CTU. This feature<br>
makes the code bigger and more complex.<br>
There are 0 unit tests for minimal import, whatever information we<br>
have about this we can get only from the lldb repo.<br>
<br>
- Lookup<br>
The way we lookup names during an import of a decl is getting really<br>
complex, especially with respect to friends.<br>
We find an already imported function from an unnamed namespace, thus<br>
we simply skip the import of a function in an other TU if that has the<br>
same name and that is also in an unnamed namespace<br>
(<a href="https://github.com/Ericsson/clang/issues/335" rel="noreferrer" target="_blank">https://github.com/Ericsson/clang/issues/335</a>).<br>
We still use `localUncachedLookup` which can be very inefficient, we<br>
should be using `noload_lookup`.<br>
<br>
<br>
Thanks,<br>
Gabor<br>
<br>
On Wed, Apr 25, 2018 at 5:52 PM, Aleksei Sidorin via cfe-dev<br>
<<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br>
> Hello,<br>
><br>
> I'd like to discuss the idea of changing the way how we check for import<br>
> issues in ASTImporter and ASTNodeImporter.<br>
><br>
> 1. Introduction<br>
><br>
> ASTImporter is now extensively used in Cross-TU analysis and is in active<br>
> development. However, we find (and introduce) many errors related to how<br>
> import failures are handled now.<br>
><br>
> Currently, ASTImporter::Import() and ASTNodeImporter::Visit*() methods<br>
> return nullptr for pointers and default value in case of conflict. And this<br>
> approach brings many issues due to the following reasons.<br>
><br>
> 1. We have to check the return result and take actions explicitly.<br>
><br>
> The common strategy of failure handling is very simple - just return. While<br>
> it is very simple, almost all the code of ASTImporter consists of such<br>
> explicit checks making it much harder to read.<br>
><br>
> 2. Both nullptr and default value are valid by itself so we have to place<br>
> explicit checks to check if a failure happened. Example:<br>
><br>
> Decl *To = Importer.Import(From);<br>
> if (From && !To)<br>
> return nullptr;<br>
><br>
> This doesn't look well and it's easy to forget where From can be nullptr and<br>
> where it is impossible.<br>
><br>
> 3. Error checking is not enforced.<br>
><br>
> We have to check for import errors every time Import() is called. And it is<br>
> extremely easy to forget placing a check after a call. This error pattern is<br>
> so common in ASTImporter that I was going to write a CSA check for this.<br>
><br>
> 2. Possible solutions.<br>
><br>
> The problem with enforcing checks is the most annoying one but also looks to<br>
> be the simplest - we can just replace the return value with Optional or<br>
> ErrorOr so clients will be enforced to perform a failure checking. This<br>
> replacement can include two changes.<br>
><br>
> 1. A number of large commits that change the return type of<br>
> ASTNodeImporter::Visit*() methods. And I'd like to hear the comments from<br>
> existing users that have their own improvements of ASTImporter because they<br>
> have to change their code not touched by the commit as well - there be some<br>
> pain.<br>
><br>
> 2. Introducing an alternative method in ASTImporter that returns ErrorOr.<br>
> So, we can migrate to the new, say, ImportOrError(), by small patches.<br>
><br>
> This replacement also solves the problem (2) because value and error are now<br>
> separated.<br>
><br>
> The problem of improving readability of these checks is a bit harder. The<br>
> most straightforward approach is to create a macro like:<br>
><br>
> #define IMPORT_PTR_INTO_VAR(VarName, Type, From) \<br>
> Type *VarName = nullptr; \<br>
> { \<br>
> auto MacroImportRes = Importer.Import(From); \<br>
> if (!MacroImportRes) \<br>
> return MacroImportRes.getError(); \<br>
> VarName = *MacroImportRes; \<br>
> }<br>
><br>
> Same for non-pointer type. So, the final code will look like this:<br>
><br>
> ErrorOr<Expr *> ASTNodeImporter::VisitCXXMemberCallExpr(CXXMemberCallExpr<br>
> *E) {<br>
> IMPORT_QUAL_TYPE_INTO_VAR(T, E->getType())<br>
> IMPORT_PTR_INTO_VAR(ToFn, Expr, E->getCallee())<br>
><br>
> SmallVector<Expr *, 4> ToArgs(E->getNumArgs());<br>
> IMPORT_CONTAINER(E->arguments(), ToArgs);<br>
><br>
> return new (Importer.getToContext()) CXXMemberCallExpr(<br>
> Importer.getToContext(), ToFn, ToArgs, T, E->getValueKind(),<br>
> Importer.Import(E->getRParenLoc()));<br>
> }<br>
><br>
> But macros don't look clean. Unfortunately, I still haven't found a better<br>
> option so any suggestions are very welcome.<br>
><br>
> PS. The error handling pattern we wish to get is very close to exception<br>
> handling in C++ because usually we don't want to perform local error<br>
> handling but centralized instead. With a possibility to just throw an<br>
> exception and catch it in the ASTImporter entry point (returning an<br>
> ErrorOr), error handling in ASTImporter becomes almost trivial and pretty<br>
> clean. But we don't use exceptions in LLVM and clang and I don't know a<br>
> similar functionality in LLVM utilities.<br>
><br>
> --<br>
> Best regards,<br>
> Aleksei Sidorin,<br>
> SRR, Samsung Electronics<br>
><br>
> _______________________________________________<br>
> cfe-dev mailing list<br>
> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div>