[Lldb-commits] [PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

[Lang Hames via Phabricator via lldb-commits]

lhames accepted this revision.
lhames added a comment.

Committed in r323163.



================
Comment at: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2106
+// 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.
----------------
clayborg wrote:
> 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 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D41997