[Lldb-commits] [lldb] [LLDB] Fix crash in TypeSystemClang::GetIndexofChildMemberWithName. (PR #117808)

[Michael Buch via lldb-commits]

================
@@ -6754,12 +6754,12 @@ size_t TypeSystemClang::GetIndexOfChildMemberWithName(
           llvm::StringRef field_name = field->getName();
           if (field_name.empty()) {
             CompilerType field_type = GetType(field->getType());
+            std::vector<uint32_t> save_indices = child_indexes;
----------------
Michael137 wrote:

> So there could be several successful iterations, lengthening the vector, but final failure. At the point of failure there could be some number of no-longer-valid indexes that have been added to the vector, so clearing it is the only option (other than saving it before the loop and restoring it when the error condition is encountered...).
>
> And (to clarify a bit more), the reason that that is a problem is because at line 6759 below, we make a recursive call, but if it fails we don't return an error; instead we continue executing in this function (but now with a child_indexes vector that would be incorrect if the recursive call hadn't cleared it or restored it to its original state).

Right, but if we changed `GetIndexOfChildMemberWithName` to return an `Expected`, then we'd be able to handle the failure of the recursive call too. My thinking was that the case where Clang finds a path to a base-class but we then fail to compute the index to that element feels like a situation where we should bail out of this function. Although the counterpoint would be, what if Clang found paths to multiple base-classes, and only one of them failed. Then yes, we need the backtracking that you implement here. But looking current implementation, wouldn't we return a bogus result if Clang found multiple `CXXBasePaths` anyway? It would just add all the indices into `child_indexes` (it doesn't just pick "the first result" like the FIXME that Pavel points to suggests).

Correct me if I'm misunderstanding your points though!

I'm happy to go with your simple approach for now and clean up the function as a follow-up. It looks like there's a few things wrong with it. And the test-case you added here will be useful to drive that refactor.

I  think I prefer @labath's suggestion over mine too. That feels like the most robust option.

https://github.com/llvm/llvm-project/pull/117808