LGTM

We are using FileOutputBuffer. That class creates a temporary file then atomically rename it a desitnation filename when commit() is called. So I'd think we should add a checkpoint just before commit().

IIRC lld's --build-id={md5,sha1} was slow at first but after we made them a tree-hash to utilize mutli-cores, its cost became negligible. Have you considered taking that approach? This watermark hash is probably not a thing that people attack by the collision attack, so it might not have to be a cryptographically-safe hash, but still cryptographically-safe hash function has nice properties compared to non-crypto ones.

LGTM

LGTM

LGTM

LGTM

In D64930#1636625, @MaskRay wrote:

In D64930#1626816, @MaskRay wrote:

In D64930#1626651, @peter.smith wrote:

I'm personally in favour of accepting D64930 and D64906 (I think that there is also D65865 on X86) as I think that this is an important size optimization already performed by Gold and BFD. Thinking about how to progress this further, I think it would be good to write a summary of where each of these reviews is at and what the plan is for this feature. For example:

Thanks for pushing forward this patch series!

• Have all the comments been addressed, I think the main concern was about TLS?

All addressed. There was concern about TLS but I aligned p_vaddr/p_offset to p_align of PT_TLS.

• Which targets do we want this to apply to, all of them, or just those with a 64k page size?

All, probably except x86_64 (with max-page-size=4096).

bool enable = config->emachine != EM_X86_64 || config->maxPageSize != 4096;

• What is the dependency tree of the reviews, I think D64906 is a pre-requisite for all of them?

A tree with a single internal node:) Yes, D64906 is a pre-requisite for all of them.

D64906 base, PPC64
  D64930 AArch64 47 tests
  D65865 i386 33 tests
  Dxxxxx ARM (30~50 tests)
  Dxxxxx MIPS
  rLLDxxxxx RISC-V <10 tests
  rLLD PPC32 <10 tests
  Dxxxxx x86_64 with maxpagesize != 4096

Each new target can be enabled independently. The code change is trivial:

-      bool enable = config->emachine == EM_PPC64;
+      bool enable =
+          config->emachine == EM_AARCH64 || config->emachine == EM_PPC64;

The only problem is the number of the affected tests. Due to the layout change nature of the patch series, lots of addresses have to be updated. x86_64 with maxpagesize=4096 is estimated to affect 300+ tests.

• Does anyone have any objection, or are they happy for individual people active in a backend to approve that part?

We haven't heard from MIPS yet. @atanasyan

@ruiu Let's move D64906, D64930 and D65865 forward?

LGTM

LGTM

It might make sense to zero-initialize a new instance with memset(sym, 0, sizeof(Symbol)) before setting any (non-zero/false) values to sym members, as it is less error-prone. What do you think?

LGTM but please wait for Sid.

This has been merged on July 2 as http://reviews.llvm.org/rL364813

In D66274#1634895, @arichardson wrote:

In D66274#1634810, @ruiu wrote:

In D66274#1634792, @MaskRay wrote:

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

I had to do this for the CHERI fork of LLD since we added a few new entries. We currently always use a 128-bit mask (https://github.com/CTSRD-CHERI/llvm-project/blob/master/lld/ELF/Relocations.cpp#L139) but I guess we could reorder the enum to have a fast path for commonly checked values and do the second compare only for less common entries. I can submit a patch if you would like to use the 128-bit variant.

LGTM

LGTM

In D66274#1634792, @MaskRay wrote:

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

Nice! Thank you for doing this.

LGTM

LGTM

LGTM

LGTM

I confirmed that these tests are not affected by the actual path on a machine running the tests. I missed the part that you added .line directive in this patch. Thank you for doing this.

Currently the following 3 relocation types do not trigger the creation of a canonical PLT:

• GOT-generating
• PLT-generating
• symbolic relocation with 0 addend in a writable location

LGTM

Generally looking good, but do these tests pass on Windows on which a path separator is \? It seems like you hardcoded / as a path separator in the tests.

I agree with you that this implementation is straightforward and simple, and I'm fine to add this if changing wine is not easy.

Is this due to the historical reason that wine is creating an .edata section instead of -export directive? I feel like this feature is obscure, and if there's a way to avoid adding this, I'd like to try that first. If GNU ld is fine with -export directive now, and wine's source code can be updated to use the directive (which is I believe a good refactoring as it would simplify the code), can wine's code be updated?

I feel that the help message and the commit message are a bit too terse. Could you add a bit more about what is the "table" here? If what that"table" means is obvious for most wasm linker users, we probably don't need to explain it, but I don't believe that's the case.

LGTM

LGTM

LGTM

I think this makes sense. Let me submit this tomorrow (I'm leaving now, and I don't want to take a risk to break buildbots.)

• fix another typo

• fix typo

LGTM

Honestly I don't have a strong preference on the behavior of weak symbols in this case. I have a slight concern that if there is a program that depends on the current behavior, they may break by this change, but this case is too subtle and linker-dependent, so I'm perhaps worried too much.

This function accumulated code and become a bit too long. We probably have to split the function into small pieces as a follow-up patch.

Sorry, I forgot one thing: can you add a test?

As to post-process -verbose output, I think that that is too fragile to depend on, as -verbose is mainly for inspection and debugging. Any arbitrary output may be added to the -verbose output that can confuse post-process scripts. I also don't want to recommend users post-process -verbose output because of the same reason -- if people rely on the current -verbose output too much, any change to the file may break scripts in the wild which could prevent us from adding new messages there.

LGTM

LGTM

In D65430#1614557, @MaskRay wrote:

In D65430#1606209, @emaste wrote:

In D65430#1605980, @peter.smith wrote:

My understanding is that many developers use makefile/ninja generation systems such as cmake rather than hand-write the file themselves. As such would this get much use unless it was integrated into these generators? May be worth approaching them to see if they have any requirements/observations about the option?

Having it integrated into e.g. cmake would be a great project for someone to take on, but for us (FreeBSD) it would be useful by itself. Our build is about 175K lines of hand-written Makefiles and we could plug it in with a small change in a couple of places. In any case we shouldn't hold up lld support waiting on prospective cmake changes IMO.

Ed, does ld.lld a.o -o /dev/null | sed 's/(.*//' | sort -u | sed '$!s/$/ \\/';} generate a usable Makefile fragment? I think the scenarios where people write Makefile are not very common now (think cmake/bazel/meson/scons/buck/...). Makefile and build.ninja are mostly used as an "assembly language". I think a dependency graph feature will be useful, but a Makefile fragment is probably not the best format. (I actually like Makefile more than the alternatives but I know the trend is that people are moving away from hand-written Makefile) At least it looks like the output can be easily generated from ld.lld -t output.

LGTM

LGTM

LGTM

LGTM

• rebased

LGTM

LGTM

LGTM

LGTM

• address review comments

In D65430#1605980, @peter.smith wrote:

No objections to the idea in principle. I think it may need a bit of wider discussion to reach its full potential though. My understanding is that many developers use makefile/ninja generation systems such as cmake rather than hand-write the file themselves. As such would this get much use unless it was integrated into these generators? May be worth approaching them to see if they have any requirements/observations about the option?

• quote a filename if it contains a space character
• add a test