[LLD] Remove global state in lld/COFF

[akhuang]

Summary

Remove globals from the lldCOFF library, by moving globals into a context class.
This patch mostly moves the config object into COFFLinkerContext.

See https://lists.llvm.org/pipermail/llvm-dev/2021-June/151184.html for
context about removing globals from LLD.

[aganea]

lld/COFF/DLL.cpp

107

I'm seeing that a lot of the *Chunk classes have this extra context now. Have you tried to measure peak RAM usage when linking large executables? Also in scenarios when /Gy is used, for example MinGW seems to use that.

My point being, if we can pass the context by argument that would be better maybe instead of storing it into these tiny objects. But I realize that isn't always practical, thus the global access proposed in D108850 - COFFLinkerContext would simply inherit from CommonLinkingContext and then could be accessed from anywhere by a call to context().

[akhuang]

lld/COFF/DLL.cpp

107

Yeah, it feels non-ideal to store the reference in a bunch of Chunks, I'll try to see whether it's feasible to pass it by argument.

Memory usage doesn't seem to increase too much, though -- peak memory usage for linking chrome (measured with https://github.com/sgraham/tim/blob/master/tim.exe) goes up by 4mb (3972mb -> 3976mb). I think chrome builds with /Gy.

[MaskRay]

(If ELF ever needs a context, I may just use Context instead of ELFLinkerContext)

[aganea]

Thanks for doing this @akhuang! Looks good generally, a few comments:

lld/COFF/COFFLinkerContext.h

61

I think this field isn't needed anymore, since after your patch, DefinedAbsolute and applySecIdx() have direct access to ctx.outputSections.size(), which this field duplicates. The only thing that is needed is probably an assert() (see other comment in Chunks.cpp)

lld/COFF/CallGraphSort.cpp

76–77

Could be ctx see other comment.

lld/COFF/Chunks.cpp

97–103

I would pass a unsigned numOutputSections here and assert within the function that the value fits on 16-bits.

lld/COFF/DLL.cpp

107

Sounds good.

lld/COFF/InputFiles.cpp

1023

Shouldn't we keep this in ctx? Otherwise we'll be creating over and over these fake sections for each bitcode file?

lld/COFF/PDB.cpp

1721

Could also be const uint32_t secrelReloc...

lld/COFF/SymbolTable.h

50

Leaving ctx as a constructor parameter is also a perfectly valid thing to do! Please see https://stackoverflow.com/questions/6185020/initializing-member-variables-using-the-same-name-for-constructor-arguments-as-f/6185043#6185043

lld/COFF/Symbols.cpp

108

Just a tiny detail, it is a bit confusing that the diff show a totally unrelated addition here, ie.before we had numOutputSection and now we have ::getRVA(). Just my preference, but I usually insert new additions in places where nothing was changed, as compared with the previous revision.

lld/COFF/Symbols.h

24–25

Please remove commented out code.

lld/COFF/Writer.cpp

721

Please capture only &ctx

1946–1947

Please remove, see the other comment.

[aganea]

Hello @akhuang! Are you able to work on this before the 14.0 branch out? If no, can I take it over and land it? Barring any comments - @MaskRay would you have a bit of time to take a look please ?

[akhuang]

@aganea Ah, right, I can take a look at the comments today

[akhuang]

Address comments

[aganea]

Thanks again Amy! LGTM with a note:

lld/COFF/Driver.cpp

1465

As an option, in this kind of long function, one could probably also create an alias at the top, if you want to avoid those many changes:

Config &config = ctx.config;

[akhuang]

cleanup and fix stuff that got messed during rebase

[akhuang]

further cleanup

[aganea]

lld/COFF/Driver.cpp

1406

Sorry I actually meant Configuration *config = &ctx.config; just so that you don’t have all these changes below when you do a git blame.

[akhuang]

lld/COFF/Driver.cpp

1406

haha right, makes sense

[akhuang]

config. to config->

[aganea]

lld/COFF/Driver.cpp

1406

Up to your discretion, I think you can apply the same "recipe" in a few other places (see other comments).

lld/COFF/DriverUtils.cpp

99

And here?

lld/COFF/Writer.cpp

1022–1023

And here?

1433

And here.

1777

Maybe here too.

1962

You could probably do the same thing here: Configuration *config = ctx.config;

[akhuang]

some more ctx.config -> config->

[aganea]

Still looks good. Perhaps if you can, before commit to ensure there are no regressions, check that the contents of Chromium's out/ dir is the same before and after this patch? (or any other real-world example)

[MaskRay]

(If ELF is ever able to do this, we may use a shorter name than const COFFLinkerContext &ctx ...)

[MaskRay]

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.

[aganea]

In 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.

[MaskRay]

In D110450#3273343, @aganea wrote:

In 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 (hidden visibility doesn't help) 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)).

[aganea]

In D110450#3273360, @MaskRay wrote:

thread_local performance is fine on aarch64 but has a higher cost (hidden visibility doesn't help) 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:(

It isn't particularly cheap on the Windows ABI either: https://godbolt.org/z/frEe87zcq - however I/O dominates in LLD right now, so using thread_local for ctx doesn't really change the runtime performance of LLD, at least on Windows/x64: https://reviews.llvm.org/D108850?id=370678#2975472

[aganea]

Hello @akhuang! Do you think I can land this on your behalf? I've already rebased & tested locally.

[akhuang]

In D110450#3653404, @aganea wrote:

Hello @akhuang! Do you think I can land this on your behalf? I've already rebased & tested locally.

Yep, go ahead! Sorry I've been fairly unresponsive on this patch.

[mstorsjo]

This patch broke one case for me - it's reproducible by improving one of the existing testcases like this:

diff --git a/lld/test/COFF/export-all-lto.ll b/lld/test/COFF/export-all-lto.ll
index 13620e11d9b9..8d54b981a104 100644
--- a/lld/test/COFF/export-all-lto.ll
+++ b/lld/test/COFF/export-all-lto.ll
@@ -2,9 +2,10 @@
 
 ; RUN: llvm-as %s -o %t.bc
 
-; RUN: lld-link -lldmingw -dll -out:%t.dll %t.bc -noentry -output-def:%t.def
+; RUN: lld-link -lldmingw -dll -out:%t.dll %t.bc -noentry -output-def:%t.def -implib:%t.lib
 ; RUN: llvm-readobj --coff-exports %t.dll | grep Name: | FileCheck %s
 ; RUN: cat %t.def | FileCheck --check-prefix=IMPLIB %s
+; RUN: llvm-nm %t.lib | FileCheck --check-prefix=IMPLIB-SYMS %s
 
 ; CHECK: Name: MyComdatFunc
 ; CHECK: Name: MyExtData
@@ -14,6 +15,12 @@
 ; IMPLIB: MyExtData @2 DATA
 ; IMPLIB: MyLibFunc @3{{$}}
 
+; IMPLIB-SYMS: 00000000 T MyComdatFunc
+; IMPLIB-SYMS: 00000000 T __imp_MyComdatFunc
+; IMPLIB-SYMS: 00000000 D __imp_MyExtData
+; IMPLIB-SYMS: 00000000 T MyLibFunc
+; IMPLIB-SYMS: 00000000 T __imp_MyLibFunc
+
 target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-w64-windows-gnu"

@aganea How do you want to go about fixing this; should I revert the patch and land this test improvement separately, so that this can be relanded with the test in place?

[aganea]

@mstorsjo Yes please revert. Thanks in advance!

[mstorsjo]

In D110450#4036025, @aganea wrote:

@mstorsjo Yes please revert. Thanks in advance!

Ok, done now!

[aganea]

lld/COFF/COFFLinkerContext.cpp

41

@mstorsjo These two structs were keeping a dangling pointer, as the FakeSections above went out of scope.

[thakis]

This broke tests on Windows: http://45.33.8.238/win/73025/step_10.txt

Please take a look and revert for now if it takes a while to fix.

[andrewng]

In D110450#4039625, @thakis wrote:

This broke tests on Windows: http://45.33.8.238/win/73025/step_10.txt

Please take a look and revert for now if it takes a while to fix.

I've already committed a fix: https://reviews.llvm.org/rG85a2f29fd485d29e6a93de6f0c2bfdc009d06a56.

[aganea]

In D110450#4039625, @thakis wrote:

This broke tests on Windows: http://45.33.8.238/win/73025/step_10.txt

Please take a look and revert for now if it takes a while to fix.

Looking now!

[aganea]

In D110450#4039659, @andrewng wrote:

In D110450#4039625, @thakis wrote:

This broke tests on Windows: http://45.33.8.238/win/73025/step_10.txt

Please take a look and revert for now if it takes a while to fix.

I've already committed a fix: https://reviews.llvm.org/rG85a2f29fd485d29e6a93de6f0c2bfdc009d06a56.

Thank you!