LGTM, thanks for all the changes @llunak!

@int3 I don't want to block this, I understand your needs, and I value the intention of this patch. I'd be happy to reintroduce llvm::sys::ThreadLocal later. However I wasn't able to reproduce the 1% regression (see D123879 for numbers), although I don't deny it could exist in some configurations (depending on the machine, OS, LLVM build options, allocator). I'm just wondering if 1% regression isn't acceptable for now in the trunk, so that we can commit a more frictionless version of this patch.

I see no difference on the ELF side, but @MaskRay maybe you're running a more through test? This is a two-stage LLD, second stage uses -march=native -Xclang -O3 -fstrict-aliasing -fwhole-program-vtables -flto=thin, running on a c5a.24xlarge EC2 instance.

ubuntu@XXX:~/chromium/src/out/Default$ uname -a
Linux XXX 5.13.0-1021-aws #23~20.04.2-Ubuntu SMP Thu Mar 31 11:36:15 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
ubuntu@XXX:~/chromium/src/out/Default$ lscpu
Architecture:                    x86_64
CPU op-mode(s):                  32-bit, 64-bit
Byte Order:                      Little Endian
Address sizes:                   48 bits physical, 48 bits virtual
CPU(s):                          96
On-line CPU(s) list:             0-95
Thread(s) per core:              2
Core(s) per socket:              48
Socket(s):                       1
NUMA node(s):                    1
Vendor ID:                       AuthenticAMD
CPU family:                      23
Model:                           49
Model name:                      AMD EPYC 7R32
Stepping:                        0
CPU MHz:                         2799.992
BogoMIPS:                        5599.98
Hypervisor vendor:               KVM
Virtualization type:             full
L1d cache:                       1.5 MiB
L1i cache:                       1.5 MiB
L2 cache:                        24 MiB
L3 cache:                        192 MiB
NUMA node0 CPU(s):               0-95
Vulnerability Itlb multihit:     Not affected
Vulnerability L1tf:              Not affected
Vulnerability Mds:               Not affected
Vulnerability Meltdown:          Not affected
Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl and seccomp
Vulnerability Spectre v1:        Mitigation; usercopy/swapgs barriers and __user pointer sanitization
Vulnerability Spectre v2:        Mitigation; LFENCE, IBPB conditional, IBRS_FW, STIBP conditional, RSB filling
Vulnerability Srbds:             Not affected
Vulnerability Tsx async abort:   Not affected
Flags:                           fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext
                                 fxsr_opt pdpe1gb rdtscp lm constant_tsc rep_good nopl nonstop_tsc cpuid extd_apicid aperfmperf tsc_known_freq pni pclmu
                                 lqdq monitor ssse3 fma cx16 sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand hypervisor lahf_lm cmp_legacy cr8_lega
                                 cy abm sse4a misalignsse 3dnowprefetch topoext perfctr_core ssbd ibrs ibpb stibp vmmcall fsgsbase bmi1 avx2 smep bmi2 r
                                 dseed adx smap clflushopt clwb sha_ni xsaveopt xsavec xgetbv1 clzero xsaveerptr rdpru wbnoinvd arat npt nrip_save rdpid

(bin_tls_bumpptr is with this patch, bin_no_tls is without, using Chromium checkout 8660f8deda4982de98baddafaffb651898144bac and LLVM checkout b859c39c40a79ff74033f67d807a18130b9afe30)

ubuntu@XXX:~/chromium/src/out/Default$ hyperfine '~/llvm-project/stage2/bin_tls_bumpptr/ld.lld @__link_unit_tests.rsp' '~/llvm-project/stage2/bin_no_tls/ld.lld @__link_unit_tests.rsp'
Benchmark 1: ~/llvm-project/stage2/bin_tls_bumpptr/ld.lld @__link_unit_tests.rsp
  Time (mean ± σ):      9.743 s ±  0.057 s    [User: 21.273 s, System: 78.149 s]
  Range (min … max):    9.661 s …  9.845 s    10 runs

This is just for demonstrative purposes, @oontvoo feel free to cherry-pick anything of interest into the other patch.

I've posted an alternate implementation, please see D123879:

In D122922#3446555, @mstorsjo wrote:

In D122922#3446489, @oontvoo wrote:

not super familiar with this effort. What is the scope of this "per-session" ? Is is per-process? If so, it doesn't solve the problem (here we have multiple threads using the same bptrAlloc, hence the race condition)

No, it's meant so that you can use lld as a library, so that one process can have multiple threads running in parallel, where more than one such thread can call e.g. lld::coff::link() at the same time. I haven't followed the state of the work on that in very close detail, but as far as I understand, the implementation strategy has been to gather everything that previously was global, into a per-invocation context, that is passed around (and/or stored as a thread local variable somewhere I think). So within each linker invocation, you'd only ever have allocations happening on one thread, the one where the user called lld::*::link() - each such linker job then would run multiple threads for occasional parallelism in the linking process though.
[...]
I think it'd be valuable to align these two thread safety efforts so we don't end up with two mechanisms for doing the same. I think @aganea has been involved in that effort though. @aganea - can you chime in here?

+ @gribozavr2

Hello! It seems the loop_dispatch.c test is occasionally causing errors on one of the builders: https://lab.llvm.org/buildbot/#/builders/84/builds/22238/steps/6/logs/FAIL__libomp__loop_dispatch_c (also see the builder's history)
Anyone would have a chance to look at it please? Thanks in advance!

In D122887#3438999, @CarlosAlbertoEnciso wrote:

Thanks for your review @aganea

Thank you for your work and the patience! :-)

LGTM, thanks again @CarlosAlbertoEnciso !

Hello! Thanks for adding me :-) Interesting challenge!
It's a bit sad that we have to do all this high-level/application-level memory management. Most modern allocators already support (lock-free) per-thread memory pools out-of-the-box, along with migration between threads and to the global pool/arena. This patch seems to be needed solely because we use BumpPtrAllocator/SpecificBumpPtrAllocator. I wonder how things would perform with just using malloc() instead + a modern underlying allocator (rpmalloc, mimalloc, ...). Memory locality brought by the bumpalloc is important, but it'd be interesting to compare benchmarks. FWIW there were discussions with @lattner at some point about integrating rpmalloc into the LLVM codebase, but I never got to post a RFC.

Do you think it would be possible to omit Optional and just pass PrintScope &HeaderScope everywhere? It feels like without the printer, these functions don't have much purpose?

In D123225#3435142, @mehdi_amini wrote:

In essence the call stack would be: the thread loop in ThreadPool::grow() calls Task() which would call the TaskGroup logic lambda, which would call the user lambda. Regular non-TaskGroup would not go through that logic.

I don't quite get what you mean? Can you elaborate a bit?
I agree that the complexity increase is worrisome, so any idea to manage it is welcome :)

Not sure @chandlerc will be able to review your patch, adding some people that have been working in this space recently.

LGTM.

In D123185#3433103, @thieta wrote:

@aganea Added more testing - adding a third collision and also probe all the types and in different orders, since the order fooled the testing the first time since the order they where processed in LLD made a difference for the output.

I had to drop the log message since when we know set the entry to nullptr there is nothing informative to log, but I am not convinced the log message was that useful and I really don't think it's worth adding some more complexity to store the paths of all collisions.

WDYT?

This looks good, but please see my other comment first! https://reviews.llvm.org/D122372#3432711

There's a typo in the title, s/descirbes/describes/

Thanks for following up on this @CarlosAlbertoEnciso!

LGTM, thanks @thieta !

In D122372#3413297, @thieta wrote:

Consolidated the test-inputs and used SED instead. I tried using available input but couldn't get it to collide the same way this one did.

Oh yeah to fully repro the bug, you'd probably need to insert and extra record in the second PDB to shift the offsets to the LF_FUNC_ID. Seems good otherwise, just two (and a half) comments.

Reduced further:

> yaml2obj.exe -o=a.obj lld\test\COFF\Inputs\pdb-type-server-simple-a.yaml
> sed -e s/ts.pdb/bs.pdb/ lld\test\COFF\Inputs\pdb-type-server-simple-b.yaml | yaml2obj.exe > b.obj
> llvm-pdbutil.exe yaml2pdb lld\test\COFF\Inputs\pdb-type-server-simple-ts.yaml -pdb ts.pdb
> llvm-pdbutil.exe yaml2pdb lld\test\COFF\Inputs\pdb-type-server-simple-ts.yaml -pdb bs.pdb
> lld-link.exe a.obj b.obj /debug
(crash)

In D122372#3411508, @thieta wrote:

The inputs are a bit big - but I am unsure on how to trim them down efficiently.

LGTM with a comment:

Can you please craft a small test that would showcase the problem? The test shall fail before your change, and should succeed after.

There's a typo in the title:

[llmv-pdbutil] Move InputFile/FormatUtil/LinePrinter to PDB library.

llvm-pdbutil

In D121801#3386781, @rnk wrote:

Essentially, this looks good to me. I have some minor concerns that as we migrate dumping functionality into library code, it will tend to increase the code size of production tools such as the linker. Maybe archive semantics and section GC paper over those problems. However, in the end I don't think it makes sense to worry about unmeasured, hypothetical code size problems. If there are problems, I guess it would make sense to factor out "dumping" functionanlity from the PDB and CodeView libraries into their own CVDump helper library or something. This is similar to how we pulled YAML writing out of lib/Object.

Seems reasonable having an integration with LLD, but maybe others would like to chime in?

In D121724#3384642, @MaskRay wrote:

Personally I am not feeling very comfortable adding such an environment variable just so that it can be tested.
I think it may make sense to add the clang driver change without a test.

Thanks for making the changes!

In D120175#3338696, @abrachet wrote:

In D120175#3335453, @aganea wrote:

There is currently no testing because, although there is testing for generateCompilationDiagnostics, it is harder to force lld to crash, even if we added a way to crash lld on purpose, the test would need to depend on lld. Looking for suggestions if anyone has any.

I think you add test coverage in LLD, through a environment variable that triggers a crash in any of the drivers & --reproduce to ensure the archive was created.

lld has tests for --reproduce already. Even if I added an environment variable to crash lld, the test would depend on lld being built, adding a big dependency to clang's tests. Do we think that's worth it?

It would be a interesting addition to bind LLVM_LINKER_IS_LLD to a LIT test "feature" -- in a follow-up patch if you're willing to? (see config.available_features.add('z3') and LLVM_WITH_Z3 in clang/test/lit.cfg.py for an example)

There is currently no testing because, although there is testing for generateCompilationDiagnostics, it is harder to force lld to crash, even if we added a way to crash lld on purpose, the test would need to depend on lld. Looking for suggestions if anyone has any.

I think you add test coverage in LLD, through a environment variable that triggers a crash in any of the drivers & --reproduce to ensure the archive was created.

LGTM, thanks!

Ideally RegisterCodeGenFlags should be a function call not a class, since it only registers static cl::opts. I think you can simply remove cgf from CommonLinkerContext.h and create a short-lived llvm::codegen::RegisterCodeGenFlags object on the stack inside the CommonLinkerContext constructor definition.

Use more generic lld::cleanup() instead of lld::elf::cleanup()

@mehdi_amini Just to unblock users, I've posted https://reviews.llvm.org/D119277 which fixes the issue originally raised by @krzysz00
I'm also working on a more long term solution in https://reviews.llvm.org/D119049

namespace fix.

All things considered I think in the short term it is better to aim for small patches/fixes that can be transplanted to release/14.x if need be. I've posted D119277 for the issue you were seeing @krzysz00, but I am still planning on proper fix over the next week or so.

@krzysz00 I will revert and re-land with both fixes.

@yaxunl I have a fix, I will send a patch later today.

LGTM with a comment:

Maybe take a look at llvm/unittests/Support/ProgramTest.cpp (for example TestExecuteAndWaitTimeout). You should be able to start the executable again with an extra env. variable which prevents the call to PrintStackTraceOnErrorSignal in llvm/utils/unittest/UnitTestMain/TestMain.cpp.

Do you think you can add a unit test (in SupportTests) that demonstrates the issue before your patch?

In that case @werat can you get the caller application to call llvm::InitLLVM? That plus the snippet in https://reviews.llvm.org/D119009#3297591 should shield from crashes if the handler isn't installed.

Could you please apply clang format just for the changed lines? You can do that usually through git clang-format.

I'm not sure it's a good idea to start loading DLLs when we get to this point.
I would rather suggest calling sys::PrintStackTraceOnErrorSignal({}); in SBDebugger::Initialize(). "argv0" will be auto-detected if necessary, otherwise if you can, change the SBDebugger::Initialize() API to pass it? It's only there so that we can find llvm-symbolizer next to the running executable.
You can also add something along those lines to avoid the crash:

static bool isDebugHelpInitialized(void) {
   return fStackWalk64 && fSymInitialize && fSymSetOptions && fMiniDumpWriteDump;
}
...
static void PrintStackTraceForThread(llvm::raw_ostream &OS, HANDLE hProcess,
                                     HANDLE hThread, STACKFRAME64 &StackFrame,
                                     CONTEXT *Context) {
  if (!isDebugHelpInitialized())
    return;

In D118070#3289227, @MaskRay wrote:

I know that you want a place to be accessed by both clang driver and lld-link but I am a bit nervous about the clang-driver style MSVC library sitting inside llvm/lib/Support/.
Is there a better place? @compnerd @aganea

Thanks @hans!

Cool! :) Seems good generally.
Sounds like an expensive flag for the runtime perf :-D But I guess it makes the debugging experience a bit nicer.

In D108850#3280778, @krzysz00 wrote:

`@aganea This change broke SerializeToHsaco in MLIR, can you help resolve this issue or revert? See https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/GPU/Transforms/SerializeToHsaco.cpp#L440

I can't quite tell what the new boolean arguments mean.

Fixing now.

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

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.

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)

In D118072#3267627, @mstorsjo wrote:

In D118072#3267462, @aganea wrote:

The feature can still be enabled with -mllvm -experimental-debug-variable-locations=true, so D116821 wasn't completely reverted.
(l
Why not adding your initial bug repro as a test, along with the flags above on the cmd line? It also looks like that prior mappings were added without test coverage.

Ok, I made such a testcase. I'm a bit afraid that it might be a little brittle in the long run though...

I was simply surprised that a such simple testcase wasn't catched elsewhere. Could you please explain 'brittle'? Do you think the outcome of this test isn't stable in the long run?
In any case LGTM.
I'll let the choice (to add a test or not) to your discretion ;-) I felt that it could make you loose time again later down the line...

The feature can still be enabled with -mllvm -experimental-debug-variable-locations=true, so D116821 wasn't completely reverted.

In D116821#3263405, @mstorsjo wrote:

Thanks @aganea! Would you mind posting such a patch for review, or should I? I’m not sure how to create a simple enough and robust testcase for it, but maybe it’s not strictly necessary when it’s just adding more enum entries in a table?

Feel free to go ahead with a patch!

In D116821#3261205, @mstorsjo wrote:

In D116821#3261109, @mstorsjo wrote:

I ran into a regression with this patch, which triggers the error "fatal error: error in backend: unknown codeview register ST0" on code that built fine before that.

Here's a reduced reproducer:

$ cat dither.c 
a() {
  long double b;
  asm volatile("" : "=t"(b));
}
$ clang -target x86_64-w64-mingw32 -c -O2 -g -gcodeview dither.c 
fatal error: error in backend: unknown codeview register ST0
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: bin/clang -target x86_64-w64-mingw32 -c -O2 -g -gcodeview dither.c
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module 'dither.c'.
4.	Running pass 'X86 Assembly Printer' on function '@a'

Do @rnk or @aganea have any clue about this?

Thanks again Amy! LGTM with a note:

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 ?