Page MenuHomePhabricator

[ASTImporter] Use llvm::Expected and Error in the importer API
ClosedPublic

Authored by martong on May 2 2019, 5:39 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.May 2 2019, 5:39 AM
Herald added projects: Restricted Project, Restricted Project. ยท View Herald Transcript

I think the best way to handle these errors in LLDB is to just log and then return some default value. That should make the current command print an error, which is better than terminating LLDB.

Otherwise the LLDB part of this patch LGTM.

martong updated this revision to Diff 197948.May 3 2019, 3:46 AM
  • Log and do not assert anywhere

IIUC, when Expected returns fails, it returns an Error object that might have information about what went wrong. Would it be possible to include the contents of that error n the log message? We often get "I can't run an expression in a really complex proprietary code base" and need to try to sort out what went wrong from these logs. So every crumb of information we can preserve is useful...

a_sidorin accepted this revision.May 4 2019, 4:12 AM

๐Ÿ‘

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
277 โ†—(On Diff #197948)

'true' body needs to be surrounded by braces as well. Same below.

This revision is now accepted and ready to land.May 4 2019, 4:12 AM
sgraenitz added inline comments.
lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
1980 โ†—(On Diff #197948)

[...] include the contents of that error n the log message?

e.g:

if (auto type_owner = merger.ImporterForOrigin(from_context).Import(type)) {
  return *type_owner;
} else {
  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS);
  LLDB_LOG_ERROR(log, type_owner.takeError(), "Couldn't import type: {0}")
  return QualType();
}
martong updated this revision to Diff 198618.May 8 2019, 3:41 AM
  • Add braces to 'true' cases when 'false' case has braces
  • Simplify logging and error handling with LLDB_LOG_ERROR
martong updated this revision to Diff 198623.May 8 2019, 4:31 AM
  • Use LLDB_LOG_ERROR in ImportDefinitionTo
martong marked 2 inline comments as done.May 8 2019, 4:39 AM
aprantl added inline comments.May 8 2019, 9:26 AM
clang/lib/AST/ASTImporter.cpp
5039 โ†—(On Diff #198623)

We don't typically commit FIXME's into LLVM code. Why not just deal with the error properly from the start?

lldb/source/Symbol/ClangASTImporter.cpp
65 โ†—(On Diff #198623)
if (!delegate_sp)
 return {};
68 โ†—(On Diff #198623)

The else is redundant.

139 โ†—(On Diff #198623)

Can you convert this to an early return instead?

sgraenitz added inline comments.May 8 2019, 10:59 AM
lldb/source/Symbol/ClangASTImporter.cpp
68 โ†—(On Diff #198623)

Here it's necessary for the scope of ret_or_error. That's a bit unfortunate. Maybe invert the condition?

martong marked 8 inline comments as done.May 9 2019, 7:25 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
5039 โ†—(On Diff #198623)

Ok, changed that to return with the error.

lldb/source/Symbol/ClangASTImporter.cpp
68 โ†—(On Diff #198623)

Ok, I have inverted the condition and this way the else became redundant, so I removed it.

139 โ†—(On Diff #198623)

Okay, I have added

if (!delegate_sp)
  return nullptr;

But we can't get rid of consumeError because otherwise the error object remains unchecked and will cause run-time assert.
LLDB_LOG_ERROR calls consumeError too plus logs the error msg, so I changed to use that.

martong updated this revision to Diff 198822.May 9 2019, 7:25 AM
martong marked 3 inline comments as done.
  • Remove FIXME and return the error
  • Use early return where possible and remove redundant else
martong updated this revision to Diff 198824.May 9 2019, 7:41 AM
  • Remove remaining FIXMEs
aprantl added inline comments.May 9 2019, 9:28 AM
clang/include/clang/AST/ASTImporter.h
203 โ†—(On Diff #198824)

Wouldn't it make more sense to return Expected<TypeSourceInfo &>?

martong marked an inline comment as done.May 10 2019, 5:18 AM
martong added inline comments.
clang/include/clang/AST/ASTImporter.h
203 โ†—(On Diff #198824)

That would not be consistent with the other changes. In this patch we change the signature of each functions similarly:
From T foo(...) to Expected<T> foo(...).
Also, TypeSourceInfo factory functions in ASTContext do return with a pointer. For example:

TypeSourceInfo *CreateTypeSourceInfo(QualType T, unsigned Size = 0) const;

/// Allocate a TypeSourceInfo where all locations have been
/// initialized to a given location, which defaults to the empty
/// location.
TypeSourceInfo *
getTrivialTypeSourceInfo(QualType T,
                         SourceLocation Loc = SourceLocation()) const;
aprantl added inline comments.May 13 2019, 11:40 AM
clang/include/clang/AST/ASTImporter.h
203 โ†—(On Diff #198824)

I believe that LLVM's practice of passing non-null pointers around is bad API design because it's never quite clear from looking at an API which pointer parameters are nullable, so I was hoping that we could correct some of that at least in the cases where we introduce API that return Expected<> objects to make it obvious that this is either an error or a valid object. If you that the perceived inconsistency weighs stronger than the readability improvements let me know and I can reconsider.

martong marked an inline comment as done.May 14 2019, 4:25 AM
martong added inline comments.
clang/include/clang/AST/ASTImporter.h
203 โ†—(On Diff #198824)

I couldn't agree more that having non-null-able pointers is a bad API, and we'd be better to have references in these cases. However, I think the situation is more complex than that.

If we changed TypeSourceInfo * to TypeSourceInfo & in this patch, that would involve to make similar changes with other importer functions (NestedNameSpecifier *, Decl *, Expr *, etc.). That would result in a huge patch and I am afraid we could not follow the incremental development suggestion in the LLVM dev policy. Thus, even if we decide to change to references I propose to do that in a separate patch.

Also, some AST nodes are indeed null-able. E.g. the body of a FunctionDecl might be null. Or the described class template of a CXXRecordDecl may be null (and we rely on this, because first we create the CXXRecordDecl then we import the ClassTemplateDecl and only then we set the relation).
The point is that we should identify those AST nodes which are really non-nullable in the AST. This does not seem trivial.
Besides, since the ASTImporter class is part of the AST lib, it would look strange if we changed to use references only in the ASTImporter. Perhaps, we should change in the whole AST API, but that might broke too many dependencies I guess. Still, this may worth an RFC on cfe-dev.

aprantl accepted this revision.May 14 2019, 8:34 AM

Alright, thanks for taking the time to walk me through the thought process!

Thank you guys for the review!

Alright, thanks for taking the time to walk me through the thought process!

martong updated this revision to Diff 199569.May 15 2019, 2:50 AM
  • Rebase to master
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. ยท View Herald TranscriptMay 15 2019, 3:27 AM
Herald added a subscriber: llvm-commits. ยท View Herald Transcript