In D72775#1822384, @MaskRay wrote:

It'd be great if you can provide the exact numbers.

Where * = with this patch
Run with a release build with assertions enabled on Windows 10 with AMD Ryzen 3900X (12C/24T).

Updated to address review comments.

In D70447#1792191, @mehdi_amini wrote:

I'm not sure I totally agree with this: a global thread pool without preemption and priority inversion seems to be easily leading to deadlock through starvation. It is also breaking composability of library components since you may end-up enqueuing to the same pool from a thread in the pool.

In D70447#1787132, @MaskRay wrote:

lld/Common/Threads.h is the only user of llvm/Support/Parallel.h. parallelForEach and parallelSort in lld do not take a context parameter, and hence the implementation of llvm/Support/Parallel.h uses a function-local static variable. If we can add a context parameter (llvm::ThreadPool), we may be able to make llvm/Support/Parallel.h more general and probably re-implement it on top of lib/Support/ThreadPool.cpp . If we can achieve that goal, do we still need to move the parallel utilities to lld?

Ping.

Updated the comment about llvm_shutdown() in LLD.

• Stop the threads on llvm_shutdown if and when that is called, but do not deallocate any memory.

I have also tested this patch with LLD built with libc++ on Ubuntu 18.04.3 LTS.

This patch basically reduces the amount of dependence that ThreadPoolExecutor has on what is effectively undefined behaviour. The changes have been based on issues that have been observed when running LLD on Windows built with the MSVC (2015 & 2017) static run-times and MinGW LLD on Windows.

Latest version of this patch is now in D70447.

Apart from the fix for the test, LGTM.

This change has caused some regressions in some of our local testing. I've filed the bug PR43461 (https://bugs.llvm.org/show_bug.cgi?id=43461) for the issue.

This change breaks if you add FILL(0x10101010) before *(.aaa) in fill.test:

# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/fill.s -o %t.o
# RUN: ld.lld -o %t --script %s %t.o
# RUN: llvm-objdump -s %t | FileCheck %s

Abandoning in favour of D63925.

Ping.

In D63657#1553962, @lebedev.ri wrote:

(that being said this entire script is kinda bogus, in this sense of integration into other build, the version tag makes no sense since it will be a llvm-one)

Simplified as suggested by reviewer.

In D63657#1553961, @lebedev.ri wrote:

1. Can you just use something along the lines of git describe --always --dirty to avoid the update-index/diff-index?

If I read correctly, this is to assign /DISCARD to the memory region rodata and expect .rodata to inherit the memory region?

Sorry I didn't follow the discussion in D61186 and didn't notice D61251. It seems @grimar and @peter.smith were fine with D61251 (this patch is essentially D61251). @andrewng if you are ok with letting this somewhat contrived example fail, we can probably still mark this as resolved (wontfix).

George's comment reminds me why we didn't take this approach in the first place, as it doesn't work correctly for the following test case:

In D61599#1497031, @mvhooren wrote:

Applied Clang Format to the changed files. Renamed bufferSize -> BufSize to match naming convention..

Some of the changes in "lib/Support/Unix/Memory.inc" don't appear to have been clang formatted.

In D61599#1493926, @mvhooren wrote:

In D61599#1493889, @lhames wrote:

I don't think this is the right solution. MemoryBlock's purpose is to track the allocated memory. What the client does with that memory (including subdividing it) is their business. The client already knows the number of bytes they're requesting, so the fix for fragmentation in SectionMemoryManager should be limited to fixes in SectionMemoryManager.

Yes I agree it would be better if the Size member of MemoryBlock just held the allocated size. This would mean that r357058 and r351916 (and perhaps others) should be reverted.

Submitted bug https://bugs.llvm.org/show_bug.cgi?id=41673 regarding the issue that the /DISCARD/ section is in the output.

Can I land this patch?

In D60131#1483142, @phosek wrote:

D61197 seems to have addressed the issue, D61186 made no difference.

In D60131#1482865, @phosek wrote:

This change broke our kernel build, I filed a bug PR41653 which has more details, but it seems like that with this change, linker produces wrong layout. Can we please revert this change until we figure out how to address this issue?

In D61186#1482564, @peter.smith wrote:

Just to clarify my understanding, are you referring to (https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/Using_ld_the_GNU_Linker/sections.html#OUTPUT-SECTION-PHDR)

If a section is assigned to one or more segments, then all subsequent allocated sections will be assigned to those segments as well, unless they use an explicitly :phdr modifier. You can use :NONE to tell the linker to not put the section in any segment at all.

Something like:

/DISCARD/ : { *(.comment) } :my_phdr
 .name : { *(.name) }
...

Where we'd want .name to to be assigned to my_phdr and any following OutputSections? I can see that this is how ld.bfd and ld.gold behave, although as you say this is somewhat of a contrived example.

This is how this function used to behave (see r316307), when findOrphanPos ignored non-live sections.

In D61186#1482350, @grimar wrote:

I am still not sure we should not just remove the /DISCARD/ like D61251 did (your explanation makes sense to me, though).

Simplified test as suggested in review.

In D61186#1482074, @grimar wrote:

I think it is 2 lines of code change actually :) I believe the much more correct way to fix it is this: https://reviews.llvm.org/D61251
With this approach there is no /DISCARD/ in the output.
(note, I did not change the original test case there).

In D61186#1480437, @grimar wrote:

From what I see in the output for this case LLD creates an /DISCARD/ output section.
This does not look correct to me, because /DISCARD/ is kind of key word, but not a section name I believe.
Also, bfd does not do that.

So I think issue first of all is that we are trying to create output section with this name, it should never be in the output.

In D60131#1471202, @ruiu wrote:

IIUC, you are fixing an issue that a segment whose size is 0 doesn't have a file offset that is congruent modulo page size. Is this correct?

The file offset for an empty segment is not actually significant; we can just set a dummy value. Have you considered that?

Is this patch acceptable or do we need to start looking for an alternative (and most probably more complex solution) to the issue described above?

This would be the follow up patch to D60131 as a fix for the orphan PHDR test failure.

The follow up patch to fix the orphan PHDR test is D60273.

In D60131#1453128, @jakehehrlich wrote:

This seems like the wrong fix to me. Shouldn't we be fixing the alignment bug with respect to segments not changing things like this?

If I recall correctly I made a change to make NOBITS the default because some sections nthat could have been NOBITS we're winding up as PROGBITS. I don't remember the exact details. I'm fairly sure this change regresses that point. You've changedna bunch of test from expecting NOBITS to expecting PROGBITS but that seems unjustified.

I am not sure we have the practice to disable the test cases. What do you think about uploading the fix for orphan behavior
in a separate change rebased on this patch too, so that these 2 patches can be reviewed together?
(in that case, having XFAIL here is fine I think).

Updated to address review comments.

The default was SHT_PROGBITS in the past, and was changed to SHT_NOBITS to "reduce file size". However, it's not clear that it actually reduces output file size, but only reduces the value of file size in the output. In any case, it does cause the alignment problem described and shown in the symbol-only-align.test test.

In D59824#1443159, @tlively wrote:

I think there are more tests that should use -o - in wasm/target-feature-none.yaml, but otherwise the wasm changes lgtm.

I've added D59824 to restore testing and usage of output to "-".

The stdout.s test you added in fact failed on Windows because this change was missing, and @thakis disabled it in rL351949. So, I think we can just re-enable stdout.s to test this.

Ping...

Updated the patch based on review suggestion.

As suggested, I have switched back to what was my original change to fix this issue.

In D57960#1390773, @ruiu wrote:

This class should do a minimum number of attempt to remove a file, instead of trying to remove a file at various points to increase the chance to successfully removing the file. Why don't you close the file before removing the file?

Update to the eh-frame-negative-pcrel-sdata8.s test.

Updated testing as per review comments.

In D49557#1168549, @ruiu wrote:

Thanks! How did yoy find this?

Abandoned in favour of D44923.

Updated to address review comments.

In D44898#1048789, @ruiu wrote:

2.8%. Somehow clang-fsds got 2.39% faster.

That speedup is interesting. I don't have a good theory to explain it though.

Even if this patch is somewhat performance-neutral, we still should avoid extra memory allocation if we can. Let's try ignoring synthetic sections in ICF. That's I think better.