[JIT] Fix crash in unit tests

[redstar]

The unit tests ReOptimizeLayerTest.BasicReOptimization and JITLinkRedirectionManagerTest.BasicRedirectionOperation are failing for me with the error:

Program aborted due to an unhandled Error:
Error value was Success. (Note: Success values must still be checked prior to being destroyed).

The error is raised when a value is assigned to Err, due to the the missing ErrorAsOutParameter.

Fixing this problem uncovered another problem: if AnonymousPtrCreator or PtrJumpStubCreator is an error, then the object is not registered at the resource manager. However, the destructor tries to de-register the object. This fails then with

Assertion `I != ResourceManagers.end() && "RM not registered"'

Fix is to always register the object at the resource manager.

[lhames]

Is it necessary to move the

ObjLinkingLayer.getExecutionSession().registerResourceManager(*this);

line up here, or can that stay where it was?

[redstar]

Without moving the line up, no resource manager is registered, and I get an assertion in the destructor:

  ~JITLinkRedirectableSymbolManager() {
    ObjLinkingLayer.getExecutionSession().deregisterResourceManager(*this);
  }

because of the check in the else branch:

void ExecutionSession::deregisterResourceManager(ResourceManager &RM) {
  runSessionLocked([&] {
    assert(!ResourceManagers.empty() && "No managers registered");
    if (ResourceManagers.back() == &RM)
      ResourceManagers.pop_back();
    else {
      auto I = llvm::find(ResourceManagers, &RM);
      assert(I != ResourceManagers.end() && "RM not registered");
      ResourceManagers.erase(I);
    }
  });
}

So yes, moving it up is necessary. And the if (Err) return; is then meaningless.
I'm running with assertions enabled, btw.

[lhames]

Thanks for answering those questions.

I think there's an even easier solution here: We can hoist the creator functions up into the named constructor and then pass them into the regular constructor, which would no longer be able to error out:

static Expected<std::unique_ptr<RedirectableSymbolManager>>
Create(ObjectLinkingLayer &ObjLinkingLayer, JITDylib &JD) {
  auto AnonymousPtrCreator(jitlink::getAnonymousPointerCreator(
    ObjLinkingLayer.getExecutionSession().getTargetTriple()));
  auto PtrJumpStubCreator(jitlink::getPointerJumpStubCreator(
    ObjLinkingLayer.getExecutionSession().getTargetTriple()));
  if (!AnonymousPtrCreator || !PtrJumpStubCreator)
    return make_error<StringError>("Architecture not supported",
                                    inconvertibleErrorCode());
  return std::unique_ptr<RedirectableSymbolManager>(
      new JITLinkRedirectableSymbolManager(ObjLinkingLayer, JD,
      AnonymousPtrCreator, PtrJumpStubCreator));
}

[redstar]

Changed the code according to your proposal. Looks much better now, thanks!

[lhames]

It would be good to just return the object here, rather than naming it then returning it, but otherwise LGTM. Thanks @redstar!

[redstar]

Done. yw.

[redstar]

Hm. Why does the PR not close after manually merging?

[lhames]

Weird -- I thought it had been merged too, but it doesn't look like it. Should be fixed now.