[ORC] fix unreachable code bug

[nickdesaulniers]

Summary

This was flagged in https://www.viva64.com/en/b/0629/ under "Snippet No.
31".

I assume that the CtorDtorsByPriority map is always meant to be cleared,
on success or failure.

[lhames]

We don't want to clear unconditionally: we should (at least optionally) preserve the state so that clients can attempt recovery from transient errors (e.g. network drop-outs).

That said, I think the behavior you have proposed is reasonable. We could make it the default by adding an argument to the run method:

Error run(bool ClearCtorDtorsOnFailure = true);

Or we should add a clear() method to the CtorDtorRunner interface and document that clients need to clear the list manually on failure. I think the first option is probably less error-prone.

[nickdesaulniers]

Wouldn't just deleting L142 from the existing sources be simpler? (or moving L146-148 into the for loop at the end (L142)?

[lhames]

In D62120#1511073, @nickdesaulniers wrote:

Wouldn't just deleting L142 from the existing sources be simpler? (or moving L146-148 into the for loop at the end (L142)?

Moving 146/148 into the loop gets you "map cleared on success but not on error" behavior, which is what I had intended. I have made that change in r361199, so I will close this out.

My thoughts above were on how to get "Optionally cleared on error" behavior, which I think is useful. Implementing that can wait for another day though.

[lhames]

Committed in r361199.

llvm/lib/ExecutionEngine/Orc/ExecutionUtils.cpp

for (auto &KV : CtorDtorsByPriority) {
     for (auto &Name : KV.second) {
       auto Added = Names.insert(Name).second;
       (void)Added;
       assert(Added && "Ctor/Dtor names clashed");
     }
   }
 
   auto &ES = JD.getExecutionSession();
+  Error E = Error::success();
   if (auto CtorDtorMap =
           ES.lookup(JITDylibSearchList({{&JD, true}}), std::move(Names),
                     NoDependenciesToRegister, true)) {
     for (auto &KV : CtorDtorsByPriority) {
       for (auto &Name : KV.second) {
         assert(CtorDtorMap->count(Name) && "No entry for Name");
         auto CtorDtor = reinterpret_cast<CtorDtorTy>(
             static_cast<uintptr_t>((*CtorDtorMap)[Name].getAddress()));
         CtorDtor();
       }
     }
-    return Error::success();
   } else
-    return CtorDtorMap.takeError();
+    E = CtorDtorMap.takeError();
 
   CtorDtorsByPriority.clear();
 
-  return Error::success();
+  return E;
 }
 
 void LocalCXXRuntimeOverridesBase::runDestructors() {
   auto& CXXDestructorDataPairs = DSOHandleOverride;
   for (auto &P : CXXDestructorDataPairs)
     P.first(P.second);
   CXXDestructorDataPairs.clear();
 }