Page MenuHomePhabricator

Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF
ClosedPublic

Authored by lhames on Jan 12 2018, 9:40 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

lhames created this revision.Jan 12 2018, 9:40 AM

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 added inline comments.
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 requested changes to this revision.Jan 12 2018, 11:08 AM
clayborg added a subscriber: 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

This revision now requires changes to proceed.Jan 12 2018, 11:08 AM

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 added inline comments.Jan 12 2018, 2:48 PM
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 added a subscriber: labath.Jan 15 2018, 2:38 AM
labath added inline comments.
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 this revision to Diff 130284.Jan 17 2018, 3:04 PM

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 accepted this revision.Jan 17 2018, 3:10 PM
clayborg added inline comments.
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.

This revision is now accepted and ready to land.Jan 17 2018, 3:10 PM
lhames accepted this revision.Jan 22 2018, 4:16 PM

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.

Sounds good then.

davide closed this revision.Apr 2 2018, 9:38 AM

Lang committed this a while ago (r323163)