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

15

where is this used?

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

16

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

[jingham]

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

16

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

16

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

11

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();
 }
 
+// Checks whether m1 is an overload of m2 (as opposed to an override).
+// This is called by addOverridesForMethod to distinguish overrides (which share
+// a vtable entry) from overloads (which require distinct entries).
+static bool isOverload(clang::CXXMethodDecl *m1, clang::CXXMethodDecl *m2) {
+  // FIXME: This should detect covariant return types, but currently doesn't.
+  lldbassert(&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);
+}
+
+// If decl is a virtual method, walk the base classes looking for methods that
+// decl overrides. This table of overridden methods is used by IRGen to determine
+// the vtable layout for decl's parent class.
+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));
+  }
+}
+
+// If clang_type is a CXXRecordDecl, builds the method override list for each
+// of its virtual methods.
+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
+
+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>
+
+"""
+
+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', '// Set breakpoint here')
+
+    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; // Set breakpoint here
+}