[PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF
Lang Hames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 22 16:16:32 PST 2018
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
More information about the llvm-commits
mailing list