Page MenuHomePhabricator

Add missing nullptr checks.
ClosedPublic

Authored by aprantl on Jan 8 2020, 1:43 PM.

Details

Summary

GetPersistentExpressionStateForLanguage() can return a nullptr if it cannot construct a typesystem. This patch adds missing nullptr checks at all uses.

Inspired by rdar://problem/58317195

Diff Detail

Event Timeline

aprantl created this revision.Jan 8 2020, 1:43 PM
davide added a subscriber: davide.

Raphael and Jim should look at the expression evaluator bits.

If we can't make a persistent expression state, are we going to be able to do anything useful with expressions? I don't see anything wrong here, but it seems like we should really be putting up a crunchy frog warning and erroring out of "expr"directly if we really can't make a type system.

If we can't make a persistent expression state, are we going to be able to do anything useful with expressions? I don't see anything wrong here, but it seems like we should really be putting up a crunchy frog warning and erroring out of "expr"directly if we really can't make a type system.

I looked at that for a bit, but there is no obvious place where to early exit without refactoring much more of ClangExpressionDeclMap.

shafik added a subscriber: shafik.

I am curious what prompted this set of changes, they look sensible but I don't see how we can do anything useful if we end up in this state.

Do we currently have a way to end up in this state?

Do we currently have a way to end up in this state?

Not in llvm.org. This was prompted by a swift-lldb-crash where we can end up with no Swift context after a catastrophic error. I believe that in practice the clang runtimes are always available, but the API returns a nullable pointer so the call sites should take this into account:

PersistentExpressionState *
Target::GetPersistentExpressionStateForLanguage(lldb::LanguageType language) {
  auto type_system_or_err = GetScratchTypeSystemForLanguage(language, true);

  if (auto err = type_system_or_err.takeError()) {
    LLDB_LOG_ERROR(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_TARGET),
                   std::move(err),
                   "Unable to get persistent expression state for language {}",
                   Language::GetNameForLanguageType(language));
    return nullptr;
  }

  return type_system_or_err->GetPersistentExpressionState();
}

Should we merge this like that, or is there a better way of doing this?

davide added a comment.Jan 9 2020, 2:34 PM

Should we merge this like that, or is there a better way of doing this?

We should merge it like this, IMHO.

davide added a comment.Jan 9 2020, 2:46 PM

@teemperor what do you think?

The C++ expression parser will probably behave incredibly incorrectly without a persistent state but before this patch it just crashed, so I think this is good to go.

teemperor accepted this revision.Jan 10 2020, 12:02 AM

If we can't make a persistent expression state, are we going to be able to do anything useful with expressions? I don't see anything wrong here, but it seems like we should really be putting up a crunchy frog warning and erroring out of "expr"directly if we really can't make a type system.

I looked at that for a bit, but there is no obvious place where to early exit without refactoring much more of ClangExpressionDeclMap.

FWIW, you could also just add the check to ClangUserExpression::ClangUserExpressionHelper::ResetDeclMap and return a nullptr there. The API is designed to return a nullptr for utility expressions on that don't use the DeclMap. That's the only place in upstream LLDB where we construct a DeclMap with a target, so if there is a check there then the DeclMap doesn't need any further changes and we can just assume there is a persistent state in the rest of the code. But it doesn't really make the code much better, just this patch smaller.

This revision is now accepted and ready to land.Jan 10 2020, 12:02 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 8:53 AM

The C++ expression parser will probably behave incredibly incorrectly without a persistent state but before this patch it just crashed, so I think this is good to go.

Should we consider a refactor to ClangExpressionDeclMap that makes this more obvious? [and maybe allows us to not sprinkling these checks everywhere] -- as a followup?