Page MenuHomePhabricator

[lldb] Fix dynamic_cast by no longer failing on variable without metadata
ClosedPublic

Authored by teemperor on Aug 8 2019, 2:31 AM.

Details

Summary

Our IR rewriting infrastructure currently fails when it encounters a variable which has no metadata associated.
This causes dynamic_cast to fail as in this case IRForTarget considers the type info pointers ('@_ZTI...') to be
variables without associated metadata. As there are no variables for these internal variables, this is actually
not an error and dynamic_cast would work fine if we didn't throw this error.

This patch fixes this by removing this diagnostics code. In case we would actually hit a variable that has no
metadata (but is supposed to have), we still have the error in the expression log so this shouldn't make it
harder to diagnose any missing metadata errors.

This patch should fix dynamic_cast and also adds a bunch of test coverage to that language feature.

Fixes rdar://10813639

Diff Detail

Event Timeline

teemperor created this revision.Aug 8 2019, 2:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 2:31 AM
labath added a subscriber: labath.Aug 8 2019, 3:03 AM

This looks like a very hacky way to achieve something. Isn't there some better way to detect this situation? Maybe something to do with the linkage type of these variables?

lldb/packages/Python/lldbsuite/test/lang/cpp/dynamic_cast/main.cpp
19

put the #include at the top?

labath added a comment.Aug 8 2019, 3:11 AM

(and it won't work on windows, because the msvc demangler will demangle this as "struct Base `RTTI Type Descriptor'")

The whole information we have "@_ZTI4Base = external constant i8*", that there is no metadata and that we call dynamic_cast on it. I don't see a better way for detecting this than actually checking if we are trying to retrieve RTTI info as a global variable. But I agree that the check itself could be more robust. Looking at the code for the demangler I don't see any good way that fits our use case though (e.g. no isTypeInfo or something like that). The only alternative I see for that would be to implement our own mangling detection and then just have a simple check if the mangled name is type information related (e.g. startswith("_ZTI") and whatever the MS equivalent its). But that wouldn't be better than what we have now.

teemperor updated this revision to Diff 214102.Aug 8 2019, 3:55 AM
  • Fixed MS mangling detection.
  • Moved include to top.
  • Extracted check into own function.

Maybe to clarify the situation a bit: LLDB thinks the type info variable is a global variable and throws an error because it has no metadata for that variable. This makes sense in most cases, but type info pointers are not global variables that are declared anywhere in our code. So I think filtering out these artificial global variables in this branch seems like the intended way this should work.

labath added a comment.Aug 8 2019, 5:06 AM

I'm not very familiar with this code, so I don't really understand what kind of rewriting it does, or what is this metadata thing, but...... what if we turn this around somehow? What are the circumstances under which we don't find metadata for a global variable and we definitely *do* want to report this as an error?

Are there any, or is this just some kind of a sanity check on the inner workings of the expression parser machinery? I tried replacing the bit of code which currently special cases objc selectors and variables without external linkage (and now RTTI info) with just an unconditional "true", and nothing spectacular seems to happen -- all of the tests pass (including the one you've just added). If we can fix tests by deleting code, then I would prefer we do that instead of introducing more dubious code...

What are the circumstances under which we don't find metadata for a global variable and we definitely *do* want to report this as an error?

The metadata just maps the variables back to the original Clang decls. So if LLDB or Clang fail to register the metadata for any reason, then we currently report this back to the user and point out the specific variable that failed. Without this check we will just assume everything is fine but then probably fail later with another error that is less specific ("failed to run expression" or so). I'm fine with deleting this as we can still see the warning in the log file and it's obvious to the user that something went wrong if they see any error (even if it's less specific).

all of the tests pass

That piece of code has 0 test coverage and was checked in without any tests, so that's expected :(. But I tried for an hour now to trigger a reasonable error after removing this code, so I think this is as safe as it gets.

teemperor updated this revision to Diff 214149.Aug 8 2019, 7:29 AM
teemperor added a reviewer: labath.
  • Delete the checks instead of adding a type-info exception.
friss added a subscriber: friss.Aug 8 2019, 9:29 AM

I only have distant notions of what this code does, so this question might be completely off: Is this the code that decides whether we need to rewrite the accesses to a variable to go through the __lldb_args structure? If yes, I was surprised to learn recently that we rewrite accesses to global variables at all. I understand why we do it for locals, but why not access globals through standard codegen? If the issue your addressing is the result of this transformation, do you have any idea why we do the transformation in the first place?

friss added inline comments.Aug 8 2019, 9:31 AM
lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
1307–1308

Unrelated to your patch, but this if block is pretty interesting.

labath accepted this revision.Aug 9 2019, 1:27 AM

Thanks. I still don't know much about this, but I am very happy that we're able to fix stuff by deleting code. We can always revisit this decision if we run into problems later on..

This revision is now accepted and ready to land.Aug 9 2019, 1:27 AM
teemperor marked 2 inline comments as done.Aug 9 2019, 3:37 AM

I only have distant notions of what this code does, so this question might be completely off: Is this the code that decides whether we need to rewrite the accesses to a variable to go through the __lldb_args structure? If yes, I was surprised to learn recently that we rewrite accesses to global variables at all. I understand why we do it for locals, but why not access globals through standard codegen? If the issue your addressing is the result of this transformation, do you have any idea why we do the transformation in the first place?

That seems to be the case, but I am not aware why (as I never really touched the __lldb_args struct logic).

lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp
1307–1308

Yeah, just removed that :)

teemperor updated this revision to Diff 214517.Aug 10 2019, 3:20 AM
teemperor marked an inline comment as done.
teemperor retitled this revision from [lldb] Fix dynamic_cast by not treating type info pointers as variables. to [lldb] Fix dynamic_cast by no longer failing on variable without metadata.
teemperor edited the summary of this revision. (Show Details)
  • Rebased patch.
  • Updated title/description.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2019, 3:57 AM

This change caused failures on the Windows bot: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/7681 compounding on the existing failures from [lldb] Refactor guard variable checks in IRForTarget