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

[teemperor]

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

[labath]

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

18 ↗

(On Diff #214089)

put the #include at the top?

[labath]

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

[teemperor]

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]

• Fixed MS mangling detection.
• Moved include to top.
• Extracted check into own function.

[teemperor]

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]

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...

[teemperor]

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]

• Delete the checks instead of adding a type-info exception.

[friss]

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]

lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp

1388–1391 ↗

(On Diff #214149)

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

[labath]

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..

[teemperor]

In D65932#1621309, @friss wrote:

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

1388–1391 ↗

(On Diff #214149)

Yeah, just removed that :)

[teemperor]

• Rebased patch.
• Updated title/description.

[stella.stamenova]

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

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic_cast/ExtBase.h

+class ExtBase {
+  virtual char bar();
+};

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic_cast/ExtBase.cpp

+#include "ExtBase.h"
+
+char ExtBase::bar() {
+  return 'x';
+}

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic_cast/Makefile

+LEVEL = ../../../make
+CXX_SOURCES := main.cpp ExtBase.cpp
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/dynamic_cast/TestDynamicCast.py

+from lldbsuite.test import lldbinline
+
+lldbinline.MakeInlineTest(__file__, globals(), [])

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

+#include "ExtBase.h"
+
+class Base {
+public:
+  virtual char foo() {
+    return 'b';
+  }
+};
+
+class Derived : public Base {
+public:
+  char foo() override {
+    return 'd';
+  }
+};
+
+class NonOverrideDerived : public Base {
+};
+
+class ExtDerived : public ExtBase {
+public:
+  char bar() override {
+    return 'y';
+  }
+};
+
+int main() {
+  Derived d;
+  NonOverrideDerived d2;
+  Base *b = &d;
+  Base *real_base = new Base();
+  char c = dynamic_cast<Derived *>(b)->foo();
+
+  ExtDerived ext_d;
+  ExtBase *ext_b = &ext_d;
+  ExtBase *ext_real_base = new ExtBase();
+  c = dynamic_cast<ExtDerived *>(ext_b)->bar();
+
+
+  return 0; //% self.expect("expression dynamic_cast<class Derived *>(b) == (Derived*)b", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class Base *>(b) == (Base*)b", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class Derived *>(real_base) == nullptr", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class NonOverrideDerived *>(&d) == nullptr", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class ExtDerived *>(real_base) == nullptr", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class Derived *>(&d2) == nullptr", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class NonOverrideDerived *>(&d2) == (NonOverrideDerived *)&d2", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class Derived *>(&ext_d) == nullptr", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class ExtDerived *>(ext_b) == (class ExtDerived*)ext_b", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class ExtBase *>(ext_real_base) == (class ExtBase*)ext_real_base", substrs = ["bool", " = true"])
+            //% self.expect("expression dynamic_cast<class ExtDerived *>(ext_real_base) == nullptr", substrs = ["bool", " = true"])
+}

lldb/trunk/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp

bool IRForTarget::MaybeHandleVariable(Value *llvm_value_ptr) {
   } else if (GlobalVariable *global_variable =
                  dyn_cast<GlobalVariable>(llvm_value_ptr)) {
     if (!GlobalValue::isExternalLinkage(global_variable->getLinkage()))
       return true;
 
     clang::NamedDecl *named_decl = DeclForGlobal(global_variable);
 
     if (!named_decl) {
-      if (IsObjCSelectorRef(llvm_value_ptr))
-        return true;
-
-      if (!global_variable->hasExternalLinkage())
-        return true;
-
       LLDB_LOG(log, "Found global variable \"{0}\" without metadata",
                global_variable->getName());
 
-      return false;
+      return true;
     }
 
     llvm::StringRef name(named_decl->getName());
 
     clang::ValueDecl *value_decl = dyn_cast<clang::ValueDecl>(named_decl);
     if (value_decl == nullptr)
       return false;