Page MenuHomePhabricator

[lldb] Add basic -flimit-debug-info support to expression evaluator
ClosedPublic

Authored by labath on Jun 10 2020, 5:52 AM.

Details

Summary

This patch adds support for evaluation of expressions referring to types
which were compiled in -flimit-debug-info (a.k.a -fno-standalone-debug)
in clang. In this mode it's possible that the debug information needed
to fully describe a c++ type is not present in a single shared library

  • for example debug info for a base class or a member of a type can

only be found in another shared library. This situation is not
currently handled well within lldb as we are limited to searching within
a single shared library (lldb_private::Module) when searching for the
definition of these types.

The way that this patch gets around this limitation is by doing the
search at a later stage -- during the construction of the expression ast
context. This works by having the parser (currently SymbolFileDWARF, but
a similar approach is probably needed for PDBs too) mark a type as
"forcefully completed". What this means is that the parser has marked
the type as "complete" in the module ast context (as this is necessary
to e.g. derive classes from it), but its definition is not really there.
This is done via a new field on the ClangASTMetadata struct.

Later, when we are importing such a type into the expression ast, we
check this flag. If the flag is set, we try to find a better definition
for the type in other shared libraries. This part reuses the same code
path (ClangASTSource::CompleteType) that we are using to find
definitions of types that are incomplete in some module (the difference
between this situation and -flimit-debug-info is that in the former
case, the type is only ever used opaquely in the module -- e.g. FILE *

  • whereas in the latter the type was fully defined in the source code,

but the compiler chose not to emit its definition in the debug info).

This required a small refactor of the ClangASTSource::CompleteType
function -- I split off the part which searches for the equivalent
complete type into a separate function. This was needed because the
import process operates slightly differently here. Previously, we were
being asked to complete an already imported forward declaration. Now we
are searching for an equivalent declaration before importing anything --
that's because importing an "forcefully completed" type would mark the
type as complete in the destination AST and we would not be asked to
complete it anymore.

This patch only implements this handling for base classes, but other
cases (members, array element types, etc.). The changes for that should
be fairly simple and mostly revolve around marking these types as
"forcefully completed" at an approriate time -- the importing logic is
generic already.

Another aspect, which is also not handled by this patch is viewing these
types via the "frame variable" command. This does not use the AST
importer and so it will need to handle these types on its own -- that
will be the subject of another patch.

Diff Detail

Event Timeline

labath created this revision.Jun 10 2020, 5:52 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

The overall approach is what we had talked about previously and looks good as far as I can tell. I don't do enough expression parser work to give the accept on this, but I have no problems with what I see. If any expression parser experts are not on the reviewers, please add them!

This will be great to have, thanks for starting this off!!!

If we run into a class in the AST importer that was forcefully completed and we can't find a better definition, what do we do? Error out? Do what we did. I would like there to be a nice log line in the "log enable lldb expr" to tell us when this happens so we can know why an expression is failing if we don't emit an error and stop the expression.

Also, for "frame variable" and the any ValueObject objects, we can easily see what a BaseClass is marked as forcefully completed and just do a simple type search for the type given a full decl context as context and substitute the type. That won't use any of this code, it will just need to know to grab the the full type from the target by doing a search. We could make a function similar to CompilerType::GetCompleteType() that takes a target so that it can search all of the modules in a target for a type.

It's great to see this being addressed! I have a high-level question: When completing types across lldb::Modules — in which ASTContext is the complete type created? Since a per-module TypeSystem can be shared by many debuggers, I want to make sure that types from another module don't pollute another module's ASTContext, and that they are created in the/a scratch context instead.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
882

Can you document what case exactly is being handled here?

lldb/source/Plugins/ExpressionParser/Clang/ClangASTMetadata.h
87

It would be important to document somewhere what the semantics of this attribute are exactly, since the name is not self-explanatory.

labath updated this revision to Diff 270097.Jun 11 2020, 3:38 AM

add more comments and logging

If we run into a class in the AST importer that was forcefully completed and we can't find a better definition, what do we do? Error out? Do what we did. I would like there to be a nice log line in the "log enable lldb expr" to tell us when this happens so we can know why an expression is failing if we don't emit an error and stop the expression.

We do what we did previously -- import the empty class. The included test case exercises this scenario as well.

I'll print a log message when that happens.

Also, for "frame variable" and the any ValueObject objects, we can easily see what a BaseClass is marked as forcefully completed and just do a simple type search for the type given a full decl context as context and substitute the type. That won't use any of this code, it will just need to know to grab the the full type from the target by doing a search. We could make a function similar to CompilerType::GetCompleteType() that takes a target so that it can search all of the modules in a target for a type.

Yes, that's the rough idea, but I haven't tried implementing it yet.

It's great to see this being addressed! I have a high-level question: When completing types across lldb::Modules — in which ASTContext is the complete type created? Since a per-module TypeSystem can be shared by many debuggers, I want to make sure that types from another module don't pollute another module's ASTContext, and that they are created in the/a scratch context instead.

Apart from the adding the "forcefully completed" flag, this solution does not change the contents of module ASTs in any way. If we take the example in the test case, the individual modules ASTs would look something like:
libone:

struct One {
  int one;
};

libtwo:

struct [[forcefully_completed]] One { };
struct Two : One {
  int two;
};

a.out:

struct [[forcefully_completed]] One { };
struct [[forcefully_completed]] Two { };
struct InheritsFromOne: One {
  int member;
};
...

When importing into the expression AST, we pick the best representation for each of the types and create a merged class. I'm not entirely sure what happens when importing this into the AST for persistent expression results (what's the name for that?). Ideally the merged type would be imported there too, but I am not sure if that actually happens right now -- expr inherits_from_one.one works, but a plain expr inherits_from_one just displays the member member. I'm not yet sure if this is simply due to lack of support in the "frame variable" machinery which is supposed to display the result, or if there is some more importing that needs to happen.

teemperor requested changes to this revision.Jun 11 2020, 7:32 AM

I think the approach here makes sense. The only issue I see is how we get the complete replacement type (see the inline comment).

There is the issue if having an artificial empty class in an expression is an error, but we anyway already do this so I don't think this discussion should block this patch.

Also the FindCompleteType refactoring should IMHO land as it's own NFC commit just because it's such a huge part of the patch (I don't think that needs a separate review, it LGTM).

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
891

This check can really easily break. We should always be able to replace the ExternalASTSource with some kind of multiplexer (like MultiplexExternalSemaSource) without breaking functionality, so doing an RTTI check for a specific class here will break that concept. For example activating the import-std-module setting will make Clang add its own ASTReader source to the AST, so the ClangASTSourceProxy is hidden in some Multiplex* class (that contains ClangASTSourceProxy and the ASTReader) and this check will fail.

I think can simplified to this logic which doesn't require any RTTI support or ClangASTSource refactoring (even though the FindCompleteType refactor is IMHO something we should do even if we don't need it for this patch). I also removed that we forcibly complete the class which I don't think is necessary:

if (td && md && md->IsForcefullyCompleted()) {
  Expected<DeclContext *> dc_or_err = ImportContext(td->getDeclContext());
  if (!dc_or_err)
    return dc_or_err.takeError();
  Expected<DeclarationName> dn_or_err = Import(td->getDeclName());
  if (!dn_or_err)
    return dn_or_err.takeError();
  DeclContext *dc = *dc_or_err;
  DeclarationName dn = *dn_or_err;
  DeclContext::lookup_result lr = dc->lookup(dn);
  if (lr.size()) {
    clang::Decl *lookup_found = lr.front();
    RegisterImportedDecl(From, lookup_found);
    m_decls_to_ignore.insert(lookup_found);
    return lookup_found;
  }
}

The only required change for getting this to work is to get rid of the ImportInProgress flag that completely disables all lookups when we copy a type. I don't think there is a good reason for even having this flag as it's anyway usually not correctly set when we do some kind of import, so I think deleting it is fine.

lldb/source/Plugins/ExpressionParser/Clang/CxxModuleHandler.cpp
11 ↗(On Diff #269811)

I guess that's WIP code

lldb/test/API/functionalities/limit-debug-info/Makefile
6

The in-tree limit-debug-info test already uses the LIMIT_DEBUG_INFO_FLAGS variable instead of hardcoding the flag, so I think we might as well do it here too.

This revision now requires changes to proceed.Jun 11 2020, 7:32 AM
labath updated this revision to Diff 272650.Jun 23 2020, 2:53 AM
labath marked 4 inline comments as done.
  • address review comments
labath marked 4 inline comments as done.Jun 23 2020, 2:58 AM
labath added inline comments.
lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
891

I wasn't really sure what that flag is guarding against (i.e. how reliable recursive lookups are), so I was trying to go beneath it. But if you think deleting it is ok, then the new version is definitely better. (There are ways we could mitigate the RTTI problem, but this sidesteps it completely.)

lldb/source/Plugins/ExpressionParser/Clang/CxxModuleHandler.cpp
11 ↗(On Diff #269811)

Yep. removed.

lldb/test/API/functionalities/limit-debug-info/Makefile
6

sounds good.

labath marked an inline comment as done.Jul 1 2020, 4:47 AM

So, what do you make of this patch now, Raphael?

teemperor accepted this revision.Jul 1 2020, 5:13 AM

LGTM beside the _check_type check always passing as discussed on IRC. Thanks!

This revision is now accepted and ready to land.Jul 1 2020, 5:13 AM
labath updated this revision to Diff 274768.Jul 1 2020, 5:15 AM
  • redo the sanity check in the test (get_field_array does not include inherited types)
  • skip gmodules test flavours due to pr46284
This revision was automatically updated to reflect the committed changes.