This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Added error handling for AST import.
ClosedPublic

Authored by balazske on Sep 4 2018, 7:51 AM.

Details

Summary

The goal of this change is to make the ASTImporter::Import functions return
llvm::Expected instead of the imported type.
As first part the ASTNodeImporter visit functions are updated to return with
llvm::Expected. Various import functions are added to ASTNodeImporter to
simplify the code and have a common place for interface towards ASTImporter
(from ASTNodeImporter). There is some temporary code that is needed before
ASTImporter is updated.

Diff Detail

Repository
rL LLVM

Event Timeline

balazske created this revision.Sep 4 2018, 7:51 AM

This patch is huge, but we change here almost all implementation functions of ASTNodeImporter to return with either Error or with Expected<T>.
We could not really come up with a cohesive but smaller patch because of the recursive nature of the importer. (We are open to ideas about how to cut this patch into smaller parts.)
Most of the changes are trivial: we always have to check the return value of any import function and pass on the error to the caller if there was any.
In my opinion one great simplification is the introduction of the auxiliary importSeq function. It imports each entity one-by-one after each other and in case of any error it returns with that error and does not continue with the subsequent import operations.

Hi Gabor,

To make the review proces faster, you can split the review on separate parts for Stmts, Decls, Types, etc. Otherwise, the review can last for too long.

lib/AST/ASTImporter.cpp
162 ↗(On Diff #163819)

Can we get rid from namespace specifier usages on Error and None? As I see, even in this patch Error is used without it sometimes.

417 ↗(On Diff #163819)

The changes for argument indentation look redundant. Is it a result of clang-format?

840 ↗(On Diff #163819)

Could you please add some newlines to this function? Its control flow is not trivial so some blocks need to be separated.

Hi,
Did you mean to split the review only or the patches?
It is not trivial how to make incremental patches for this change, every part is connected to the other. The import functions for a type (Decl or Stmt) call imports of other types (Expr for example) too. If the import is changed for a single type (only Stmt) to the new way every other visit (for Decl and others) needs to be updated. And then again with the next type (Stmt for example). The importSeq function calls work with any type and the visit functions that use importSeq can not work if a part of the imports is the old way, another the new.
A solution can be to have the new and old import interface together for every type (with different names), then a part of the visit functions can be updated. The first change would be to introduce the new interface (that returns Expected) for every type, but do not change the visit functions (but a new version of every other "helper" function like importDefinition is needed). If a new version of such a function is introduced and the old remains, we do not have a diff view (with relative changes) for it.

lib/AST/ASTImporter.cpp
417 ↗(On Diff #163819)

clang-format aligns start of new argument lines to the ( character above. Indentations are not always consistent in this file, I can reformat the whole file with clang-format.
(But I do not like the formatting like

Error ImportDeclParts(NamedDecl *D, DeclContext *&DC, DeclContext *&LexicalDC,
                      DeclarationName &Name, NamedDecl *&ToD,
                      SourceLocation &Loc);

if there is a LongNamedClass::LongReturnType LongNamedClass::LongNamedFunction.)

Hi Balasz,
It's going to take some time to review the whole patch.

lib/AST/ASTImporter.cpp
194 ↗(On Diff #163819)

Do I understand correctly that we have to use the upper variant because ASTImporter API is unchanged?

417 ↗(On Diff #163819)

No need to reformat the whole file. As I see, the indentation changes are not breaking and too wide to roll them back so we can leave them as-is.

1087 ↗(On Diff #163819)

Nice catch!

1658 ↗(On Diff #163819)

Space before '*' is needed.

1663 ↗(On Diff #163819)

Silent fail doesn't look correct in case of structures. Should we add a FIXME?

1953 ↗(On Diff #163819)

Should we add a FIXME for removing this method?

2266 ↗(On Diff #163819)

Space after comma is lost.

2656 ↗(On Diff #163819)

This move looks like an additional functional change. Could you please explain the reason?

2683 ↗(On Diff #163819)

Same here.

2706 ↗(On Diff #163819)

We usually don't qualify casts.

4136 ↗(On Diff #163819)

// Stopped here.

balazske added inline comments.Oct 4 2018, 7:06 AM
lib/AST/ASTImporter.cpp
194 ↗(On Diff #163819)

The "upper variant" (importInto(ImportT &To, const ImportT &From) form) is the final one. It is only to make the code more simple, you can call importInto in ASTImporter instead of Importer.importInto. This second variant is needed because the ASTImporter API is not changed (yet), the Import functions still return pointer (later they will return Expected).
But this special version for pointer is still needed to have type cast for pointers. The commented-out code should be used later.

1663 ↗(On Diff #163819)

Maybe yes. But aborting the whole import of the DeclContext could cause lots of not imported things. A warning can be printed but maybe it is already printed at the called failed import. There was no FIXME in the old code either.

Hi Balázs,

Finally, I have completed the review. Despite the fact that this patch is huge, I found only a few issues.

lib/AST/ASTImporter.cpp
4604 ↗(On Diff #163819)

I think we can just return std::move(Err); as it was done below.

5657 ↗(On Diff #163819)

Shouldn't we return an error here?

7339 ↗(On Diff #163819)

That's true, this copying can be removed.

balazske marked 9 inline comments as done.Oct 15 2018, 4:01 AM
balazske added inline comments.
lib/AST/ASTImporter.cpp
2683 ↗(On Diff #163819)

I do not know exactly why this was made, it looks like a bugfix for a problem encountered during the tests. Maybe I can undo this change (there are other related problem fixes so this is probably not needed).

4604 ↗(On Diff #163819)

Changed to return the error.

5657 ↗(On Diff #163819)

Good catch, this looks like the change was missed.

balazske updated this revision to Diff 169677.Oct 15 2018, 4:05 AM
balazske marked 2 inline comments as done.
  • Fix in ASTNodeImporter::VisitCXXDependentScopeMemberExpr.
  • Removed trailing spaces in ASTImporter.cpp.
  • Corrected formatting and small problem fixes.
balazske updated this revision to Diff 169827.Oct 16 2018, 8:20 AM

Rebase to current master.

balazske updated this revision to Diff 169975.Oct 17 2018, 2:49 AM
  • ASTImporter::ImportDefinition compatibility rename.

In the current version all clang tests do pass and most of the LLDB tests. Some LLDB tests do not work on my machine, the problems exist on master too.
This code is still a temporary version (possible to commit now or wait until every change is made?). Later the Import functions should return Expected and the remaining functions in ASTImporter.cpp should be updated. For compatibility with old code in LLDB the Import functions can not be changed without LLDB code changes so a Import_New will be created instead. Code in clang will only use the Import_New. When LLDB was updated, Import_New should be renamed to Import.

To the reviewers: Please accept this patch formally if you do not find any problems. This is an intermediate state of the code and there is more work that is dependent on this change.

a_sidorin accepted this revision.Oct 18 2018, 2:49 PM

Hi Balasz,
I cannot find any problems with the latest changes. I suggest you to commit the patch to make further reviews easier.

lib/AST/ASTImporter.cpp
8197 ↗(On Diff #169975)

ImportDefinitionOrError?

This revision is now accepted and ready to land.Oct 18 2018, 2:49 PM
balazske added inline comments.Oct 19 2018, 2:16 AM
lib/AST/ASTImporter.cpp
8197 ↗(On Diff #169975)

The intention was to make the _New name only temporary until only this new function is used therefore it is not a nice name (requires changes in LLDB). Then it should be renamed back to ImportDefinition. It is probably not needed to indicate in the name that the function can fail, it can be seen from the return value. (This will be the case with other Import functions later.)

balazske updated this revision to Diff 170182.Oct 19 2018, 4:13 AM

Rebase again to last master.

  • Fix in ASTNodeImporter::VisitCXXDependentScopeMemberExpr.
  • Removed trailing spaces in ASTImporter.cpp.
  • Corrected formatting and small problem fixes.
  • Removed llvm namespace qualifier from Error and None.
  • ASTImporter::ImportDefinition compatibility rename.
This revision was automatically updated to reflect the committed changes.
cfe/trunk/include/clang/AST/ASTImporter.h