[PATCH] D110450: [LLD] Remove global state in lld/COFF

Wed Jan 26 11:23:13 PST 2022

MaskRay added a comment.

In D110450#3273343 <https://reviews.llvm.org/D110450#3273343>, @aganea wrote:

> In D110450#3273293 <https://reviews.llvm.org/D110450#3273293>, @MaskRay wrote:
>
>> If other ports want to follow suit, I think storing `const COFFLinkerContext &ctx;` in classes like `*File` are fine since the instances are not too many.
>>
>> Placing the member in `*Section` is definitely too wasteful. For classes like `ImportThunkChunk`, I think it'd be better to avoid storing the member as well.
>
> @akhuang mentioned that this patch only increases by 4 MB the peak mem when linking a Chromium .dll with /Gy, see above https://reviews.llvm.org/D110450#3028764
> Since the COFF driver would still not be thread-safe after this patch, if you prefer we can also use `lld::context<COFFLinkerContext>()` (instead of storing ctx in *ImportThunkChunk). And then as a second step, introduce `thread_local` contexts, for cases like this where passing `ctx` through the callstack is infeasible/requires too many changes. See https://reviews.llvm.org/D108850?id=370678, lld/Common/Globals.cpp, L44.

If it is not trivial to avoid the context variable in classes like `COFFLinkerContext`, I think it is fine.
4MB increase seems acceptable.

`thread_local` performance is fine on aarch64 but has a higher cost on other architectures (especially x86-64 which many people are concerned) because llvm-project compiles lld files with `-fPIC` which uses traditional GD/LD TLS models which are slow:(

(This remind me that at some point I should implement TLSDESC for x86-64 (now that ld.lld support is fully correct after D116900 <https://reviews.llvm.org/D116900>)).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110450