I do not actually understand the algorithm yet, but I'm sending out other review comments so that you can fix them early.

What do you think of this patch? If you think this is okay to commit and should be backported to 5.0, you might want to do that, since 5.0 release is happening really soon.

Thank you for adding the comment! I now understand what this function is doing. LGTM

Can you upload a patch between SVN head and your patch instead of one between your last patch and your local head? On reviews.llvm.org, I'm seeing the diffs between your patches instead of the main repository and your patch, which is making code review hard.

LGTM

LGTM. Thank you for doing this!

LGTM

Since Rafael is on vacation, I'll submit this patch with the following fixes.

LGTM

I'm sorry but I realized that I don't actually understand what LinkerScript::allocateHeaders exactly does, so I don't understand the new code as well. I believe you know better than me, so can you add a function comment to describe what that function is supposed to do?

LGTM

I also wonder if you really want to mention -z notext. We want to recommend or endorse the option because we want to avoid text relocations in general. In most cases, you just need to compile object files with -fPIC, so I believe you want to mention only that.

LGTM

LGTM

LGTM

I *believe* busy-waiting for a very short period of time before putting itself into the blocked state is what the condition variable or the mutex implemented in the standard library already do. If you want to tune parameters for multi-thread programming primitives, you can try, but I personally wouldn't do that because I know it is hard to improve because it's already improved by experts in that field.

LGTM

LGTM

LGTM

First of all, busy-waiting is a no-no, so we can't eliminate the condition variable from the thread pool. When a thread has to wait for some event to happen, it has to use a condition variable to put itself into the "blocked" state instead of busy-looping until the condition is met. That is in general true even if using busy loops makes your program faster, as your OS supports multiple processes and you don't want one process eat up all CPU resources.

Until this patch, lld behavior was deterministic. It generated the same output as long as your input files and command line options are the same.

This patch allows us to embed a piece of C++ code to each command line option to construct a list of argument candidates at runtime. With this patch, .inc files generated by OptParserEmitter contain C macros that in turn include other .inc files. That is a flexible mechanism but can be too flexible and fragile, as you can write any code in .td file, and pieces of C++ code you write in .td files are not verified even for syntax validation until that nested text inclusions are processed by a C++ compiler. It does two-stages of code generation, one is done by OptTable and the other is done by C macros. I can imagine that a single typo in a .td file can cause a mysterious error that is hard to debug. I feel like, even though command line processing is not that easy, I wonder if it is that complicated to require two-stage code generation.

LGTM

LGTM

Publishing comments about coding style first. I'll do real code review later.

I think it is more straightforward if you change Writer::writeBuildId to compute a hash using not only an executable but also a pdb file. If you do that, every time debug info is different, you'll get a different build id, which I think solves the issue.

LGTM

This is interesting. If we could make the output of this feature usable, and if the feature is useful for other applications than chromium, it might be useful to add this feature to the linker. I'd create a new subdirectory analysis under COFF and put this code there.

I still wonder if a communication between threads can make something that slow. Where is the exact location where you observe the slowdown? Until we understand the exact reason, we cannot exclude the possibility that this change is hiding a real issue.

LGTM

LGTM

Hans already merged it in r310723.

You seem to have committed a release note for 5.0 to SVN trunk. What you should've done is to do that to the 5.0 branch. I'll correct the error for you.

Well, since it is supposed to be a thread-pool, it shouldn't spawn a thread for a new task. It should keep threads running until a process is terminated.

If I understand https://bugs.llvm.org/show_bug.cgi?id=31586 correctly, this is adding more code to a hack that will be removed in near future. I don't think we should do that unless it is absolutely necessary, especially because this patch touches not a single place but many places in the source code.

It is broken. The taskgroup should not spawn more threads than the number of physical cores however it is used. It is intended to be a thread-pool, and spawning threads per task is nonsense.

Then why does it take so much time in some test cases if both are the same in the big-O? It should never create 65k+ threads. If it does, something's broken. This patch seems to be trying to fix at a wrong place.

Because the last one is not parallelized by the taskgroup, no?

This doesn't seem correct. Imagine that you have infinite number of cores. Then, the new code would take 2x time than the old code. I guess you are "fixing" the problem at a wrong place.

LGTM