Thanks again, Peter!

Thank you for the explanation, Peter!

With overloaded operators we can write something like

error("relocation " + toString(Type) +
        " cannot refer to absolute symbol: " + toString(Sym) +
        definedAt(toObjLoc(Sym.File)) +
        referencedBy(S.getSymLoc(Sym, RelOff)));

How would you prefer it to look like?
I don't insist on the current style, but I believe it is important to be discussed prior to rewriting the code because the decision will influence the design directly.

@chrisjackson @arichardson @ruiu, thank you for the feedback! I am going to continue working on the patch somewhere in October. I would really appreciate any additional comments meanwhile.

If I calculate it right, your code has the same cyclomatic complexity as the original one. At the same time, it is one level deeper, taking into account the number of conditions for the particular lines.

Is it possible to add a test case?

Please see D50560 for my variant.

In D47540#1176742, @chrisjackson wrote:

While more structured, I think this approach could be even more invasive than that suggested. Do you have an example implementation?

Unfortunately, we don't run the profile test suite on Windows.

Did you consider dividing the patch into two separate changes to solve the two described issues independently?

You may want to add a test for placement new, no?

Can we try the approach where the final style of diagnostic messages is determined in the reporting module and all other code just don't care about it?
Right now, we have a bunch of functions here and there which prepare some parts of the message in a specific style. Maybe it will be more semantically clear if there is a distinct place where the resulting message is prepared while all other parts of the linker provide only basic parts of the message (like message text, source location(s), code location(s), etc.) in a structured form. In this case, we can adjust only that distinct place to add any required message styles.

Ping.

• Shortened relocateNonAllocForRelocatable();
• Made it static;
• Removed an unused argument;
• Changed an expression for Bits.

I think we need to see the other places converted, too. Reports for duplicate symbols and invalid relocations will probably require additional changes, which may lead to better design when we see the whole picture.

Ping.

Thanks!

Ping. Is there anything I can improve here?

• Add a test.

Not all toolchains call ToolChain::GetLinkerPath. For example, MSVC toolchain uses its own code:

void visualstudio::Linker::ConstructJob(...) {
...
  StringRef Linker = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "link");
  if (Linker.equals_lower("lld"))
    Linker = "lld-link";
...
}

In my case, I am trying to cross-compile:

> ...\clang.exe a.cpp -fuse-ld=lld -target i686-pc-linux-gnu
clang.exe: error: invalid linker name in argument '-fuse-ld=lld'

• Use DenseSet<>::count(), as George suggested.

Ping. @espindola, @ruiu, is it OK to land?

I don't see anything that violates the specs. With this patch, the offset of a segment (p_offset) is still congruent to its virtual address (p_vaddr), modulo page size. This rule is enforced in lines 1850-1852 of ELF/Writer.cpp:

if (Cmd == First)
  return alignTo(Off, std::max<uint64_t>(Cmd->Alignment, Config->MaxPageSize),
                 Cmd->Addr);

and I didn't touch them.

It's not for an application. We use it for a kernel image.

• Remove "tee" in the test.

• Change type of ArchiveName to std::string.
• Add a test.

I asked @evgeny777 and he let me take this revision.

• Adjust to the tip.

Accepted by @rafael in the mailing list.

Update according to Rafael's comments:

• Replace !FirstLoadSection with Load->LastSec != Out::ProgramHeaders.
• Add a corresponding comment.

• Remove "tee". Thanks, George!

In D37626#908500, @ruiu wrote:

I started a discussion in the llvm-dev mailing list to eliminate the need to scan .eh_frame sections in a special way. Please send a comment there if you have one.

• Move Undefs and getUndefinedSymbols() into InputFile
• Change type of Undefs to DenseSet<StringRef> so that it can be used to search for the symbol without creating another filter container.

Shorten the script in the test.

Address review comments.

Thank you for the patch. I agree with Peter, the fix should be accomplished with a test.

In D39196#904887, @rengolin wrote:

I fear someone will eventually cry foul that we "broke their code".

In D41993#975015, @ruiu wrote:

I also wonder how these initial values (6 and 5) were decided.

Thanks for the review, @rafael!

Thanks for the comments!