[PATCH] D142949: [lld] Destroy CommonLinkerContext inside lld::*::link after D108850

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

In D142949#4098579 <https://reviews.llvm.org/D142949#4098579>, @MaskRay wrote:

> How is `auto *ctx = new CommonLinkerContext` (with a later cleanup) different from an on-stack `COFFLinkerContext ctx;` whose destructor is not called?

How is the destructor not called in this patch? ctx on the stack in this patch will call `~CommonLinkerContext()` and the would destroy all the instances, the string saver, the bump allocator, etc. We want to avoid that in the "normal" scenario, for a quick exit.

You're absolutely right in the sense that as code stands right now <https://github.com/llvm/llvm-project/blob/main/lld/tools/lld/lld.cpp#L191>, ctx //could// very well live on the stack. But since your fix rGd9dbf9e30a58 <https://reviews.llvm.org/rGd9dbf9e30a581fcadd667b6d8e5827a4003b85a2>, we could revert rG45b8a741fbbf <https://reviews.llvm.org/rG45b8a741fbbf271e0fb71294cb7cdce3ad4b9bf3> and catch fatal() and exceptions in LLD-as-lib usages. Do we want that? Or do we keep assuming that on the first fatal error, we return `SafeReturn { ... canRunAgain = false }` to force the enclosing app. to exit? I'd prefer something more resilient if possible (that is, revert rG45b8a741fbbf <https://reviews.llvm.org/rG45b8a741fbbf271e0fb71294cb7cdce3ad4b9bf3> and not commit this patch).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142949/new/

https://reviews.llvm.org/D142949