Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

[lhames]

Summary

Failure to build the method override tables results in expression failures on the following trivial test case:

class Base {
public:
  virtual ~Base() {}
  virtual void foo() {}
};

class Derived : public Base {
public:
  virtual void foo() {}
};

int main() {
  Derived d;
  Base *b = &d;
  return 0; // "expr b->foo()" ok. "expr d.foo()" crashes. 
}

The reason is that without an overrides table, the definition of foo in derived is treated as a new method definition (rather than an override) and allocated its own vtable entry which does not exist in the vtable of Derived in the compiled program.

[aprantl]

Awesome!

Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2103

Could you add some doxygen comments explaining what the new function do and why doing this is necessary?

Python/lldbsuite/test/expression_command/call-overridden-method/TestCallOverriddenMethod.py

14

where is this used?

Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp

15

the expressions are missing :-)
Perhaps convert this into an inline testcase?

[jingham]

Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp

15

The expressions are in the test .py file. It isn't necessary to put them here, I'd just fix the comment.

Please don't convert regular test cases into inline ones, however. The benefit of inline test cases is mostly that they are easier to write, but they are harder to debug when they go wrong. So if the are already in regular form I'd rather not convert them.

[clayborg]

Very cool and close. It would be nice to function correctly without asserts, see inlined comment.

Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2105–2106

Use lldb_assert and possibly return false afterwards in case the asserts are compiled out

[davide]

In D41997#974957, @clayborg wrote:

Very cool and close. It would be nice to function correctly without asserts, see inlined comment.

Out of curiosity, why does lldb roll its own assertion() mechanism instead of using the standard one?

[lhames]

Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2103

Will do.

2105–2106

We can switch to lldb_assert to give us more control (at compile time) about when we turn it on or off, but I don't think we should bail out: this is an invariant, rather than a potential error case.

Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp

15

The text of the comment was just cribbed from one of the other expression tests. Is there a preferred phraseology?

return 0; // run expressions here

?

[labath]

Python/lldbsuite/test/expression_command/call-overridden-method/Makefile

10

no-limit-debug-info handling has recently been centralized into Makefile.rules. This block here should no longer be necessary.

[lhames]

Updated to address review comments:

• assert changed to lldbassert
• comments added
• test case breakpoint comment simplified
• unused import removed from testcase
• testcase Makefile cleaned up

[clayborg]

Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2106

I don't like things that can crash when asserts are off. I don't see why we wouldn't just check this, If someone does pass in a method from on AST and another from another AST, what will happen? Crash somewhere else? Why would we risk crashing or misbehaving here when it is so easy to check and avoid. I'll leave it at your discretion to do what you think is right though since I know clang does this all over.

[lhames]

Committed in r323163.

Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

2106

This can not crash unless someone has called it out-of-contract (like passing a nullptr into a routine that requires not-null). If user-input can cause a value that would have been used as a non-null argument to be null it should be guarded outside the method, not inside. I.e.:

static void foo(Foo *f) {
  assert(f && "f must not be null");
  //...
}

auto *f = lookup(readline());
if (!f)
  return; // Can't call foo without a valid f so bail out
foo(f);

If you're passing something that can't be null, there's no need for the check:

Foo* bar(); // Never returns null
foo(bar()); // this is fine.

The case in this patch is like the latter: isOverload is called with two CXXMethodDecls, where the second was found via a lookup on a CXXRecordDecl that is on the same context as the first CXXMethodDecl. There's no intervening user input that could mess with that, so we're safe.

[clayborg]

Sounds good then.

[davide]

Lang committed this a while ago (r323163)

Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 

   return template_param_infos.args.size() == template_param_infos.names.size();
 }
 
+static bool isOverload(clang::CXXMethodDecl *m1, clang::CXXMethodDecl *m2) {
+  // FIXME: This should detect covariant return types, but currently doesn't.
+  assert(&m1->getASTContext() == &m2->getASTContext() &&
+         "Methods should have the same AST context");
+  clang::ASTContext &context = m1->getASTContext();
+
+  const auto *m1Type =
+    llvm::cast<clang::FunctionProtoType>(
+      context.getCanonicalType(m1->getType()));
+
+  const auto *m2Type =
+    llvm::cast<clang::FunctionProtoType>(
+      context.getCanonicalType(m2->getType()));
+
+  auto compareArgTypes =
+    [&context](const clang::QualType &m1p, const clang::QualType &m2p) {
+      return context.hasSameType(m1p.getUnqualifiedType(),
+                                 m2p.getUnqualifiedType());
+    };
+
+  return !std::equal(m1Type->param_type_begin(), m1Type->param_type_end(),
+                     m2Type->param_type_begin(), compareArgTypes);
+}
+
+static void addOverridesForMethod(clang::CXXMethodDecl *decl) {
+  if (!decl->isVirtual())
+    return;
+
+  clang::CXXBasePaths paths;
+
+  auto find_overridden_methods =
+    [decl](const clang::CXXBaseSpecifier *specifier, clang::CXXBasePath &path) {
+      if (auto *base_record =
+          llvm::dyn_cast<clang::CXXRecordDecl>(
+            specifier->getType()->getAs<clang::RecordType>()->getDecl())) {
+
+        clang::DeclarationName name = decl->getDeclName();
+
+        // If this is a destructor, check whether the base class destructor is
+        // virtual.
+        if (name.getNameKind() == clang::DeclarationName::CXXDestructorName)
+          if (auto *baseDtorDecl = base_record->getDestructor()) {
+            if (baseDtorDecl->isVirtual()) {
+              path.Decls = baseDtorDecl;
+              return true;
+            } else
+              return false;
+          }
+
+        // Otherwise, search for name in the base class.
+        for (path.Decls = base_record->lookup(name); !path.Decls.empty();
+             path.Decls = path.Decls.slice(1)) {
+          if (auto *method_decl =
+                llvm::dyn_cast<clang::CXXMethodDecl>(path.Decls.front()))
+            if (method_decl->isVirtual() && !isOverload(decl, method_decl)) {
+              path.Decls = method_decl;
+              return true;
+            }
+        }
+      }
+
+      return false;
+    };
+
+  if (decl->getParent()->lookupInBases(find_overridden_methods, paths)) {
+    for (auto *overridden_decl : paths.found_decls())
+      decl->addOverriddenMethod(
+        llvm::cast<clang::CXXMethodDecl>(overridden_decl));
+  }
+}
+
+static void addMethodOverrides(ClangASTContext &ast, CompilerType &clang_type) {
+  if (auto *record =
+      ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType()))
+    for (auto *method : record->methods())
+      addOverridesForMethod(method);
+}
+
 bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die,
                                                 lldb_private::Type *type,
                                                 CompilerType &clang_type) {

       }
     }
 
+    addMethodOverrides(m_ast, clang_type);
     ClangASTContext::BuildIndirectFields(clang_type);
     ClangASTContext::CompleteTagDeclarationDefinition(clang_type);
 

Python/lldbsuite/test/expression_command/call-overridden-method/Makefile

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+# clang-3.5+ outputs FullDebugInfo by default for Darwin/FreeBSD
+# targets.  Other targets do not, which causes this test to fail.
+# This flag enables FullDebugInfo for all targets.
+ifneq (,$(findstring clang,$(CC)))
+  CFLAGS_EXTRAS += -fno-limit-debug-info
+endif
+
+include $(LEVEL)/Makefile.rules
+
+clean::
+  rm -rf $(wildcard *.o *.d *.dSYM)

Python/lldbsuite/test/expression_command/call-overridden-method/TestCallOverriddenMethod.py

+"""
+Test calling an overriden method.
+
+Note:
+  This verifies that LLDB is correctly building the method overrides table.
+  If this table is not built correctly then calls to overridden methods in
+  derived classes may generate references to non-existant vtable entries,
+  as the compiler treats the overridden method as a totally new virtual
+  method definition.
+  <rdar://problem/14205774>
+
+"""
+
+from __future__ import print_function
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ExprCommandCallOverriddenMethod(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+        # Find the line number to break for main.c.
+        self.line = line_number(
+            'main.cpp',
+            '// Please test these expressions while stopped at this line:')
+
+    def test(self):
+        """Test calls to overridden methods in derived classes."""
+        self.build()
+
+        # Set breakpoint in main and run exe
+        self.runCmd("file a.out", CURRENT_EXECUTABLE_SET)
+        lldbutil.run_break_set_by_file_and_line(
+            self, "main.cpp", self.line, num_expected_locations=-1, loc_exact=True)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # Test call to method in base class (this should always work as the base
+        # class method is never an override).
+        self.expect("expr b->foo()")
+
+        # Test call to overridden method in derived class (this will fail if the
+        # overrides table is not correctly set up, as Derived::foo will be assigned
+        # a vtable entry that does not exist in the compiled program).
+        self.expect("expr d.foo()")

Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp

+class Base {
+public:
+  virtual ~Base() {}
+  virtual void foo() {}
+};
+
+class Derived : public Base {
+public:
+  virtual void foo() {}
+};
+
+int main() {
+  Derived d;
+  Base *b = &d;
+  return 0; // Please test these expressions while stopped at this line:
+}