LGTM except for a few spelling/grammar nits.

Please make sure that your patch is formatted to match the LLVM style. clang-format can do that for you.

obj-path should produce the same order as save-temps because they use the same code path -- they both work by setting Filename to a non-empty string, which causes us to follow the code path that appends task identifiers and use non-temporary files. If you're seeing something different, maybe that should be fixed instead?

Doesn't the obj-path option already do what you want here?

In D44501#1200972, @smeenai wrote:

In D44501#1200324, @mstorsjo wrote:

This makes the design a little more similar to the ELF linker and should allow for features such as ARM range extension thunks to be implemented more easily.

@pcc - I found you mentioned this as part of this change; do you have any concrete plan on how to add ARM range extension thunks? I'm pondering giving that a try, but if you have ideas about how/where to start, that'd get me started quicker.

Have you looked at their design in the ELF port? CC @peter.smith, who added that support.

Please add a test with a 32-bit non-PIC executable.

LGTM

(@kubamracek FYI)

Do you have commit access?

LGTM

LGTM

In D50475#1193055, @ruiu wrote:

You don't need to call fetch again to add a file that you already have to the symbol table. You can directly add it by calling SymbolTable::addFile().

• Add a LazyObject test

Please upload patches with context. arc diff will do this for you.

The issue is that if two object files depend on one another, we can end up in a situation where SymbolTable::fetchLazy is called recursively multiple times on the same object file, so we need to make sure to do nothing on the second call. Reproducer:

$ cat a.s
.globl a, b
a:
$ cat b.s
.globl a, b
b:
$ as -o a.o a.s
$ as -o b.o b.s
$ ar cru foo.a a.o b.o
$ ld.lld -eb foo.a -m elf_x86_64

With your patch I get:

ld.lld: error: duplicate symbol: b
>>> defined at b.o:(.text+0x0) in archive foo.a
>>> defined at b.o:(.text+0x0) in archive foo.a

Ping

I don't think the .hidden directive is meaningful in COFF, is it?

LGTM

Looks great, thanks.

No, I have comments.

LGTM

But if we change the TempFile API so that it always creates the temporary in the same directory as the destination, that becomes a non-problem, no?

Can't we change dsymutil to create its temporary file in the same directory as the destination?

I thought about that but decided against it for a few reasons:

1. It seemed best to express what we want to happen directly. Putting the symbols in the object files seemed like a more round-about way of fixing the bug.
2. The set of libcall symbols is a property of the code generator linked into lld, not the one that was used to create the bitcode file. That means that if we see an old bitcode file it would not be correct to use its libcall symbols.
3. It would bloat every bitcode file with the same (potentially incorrect due to 2) content when that can be easily avoided.

I'd probably mention in the commit message that it's just the log output that's being made deterministic. As far as I know this shouldn't affect the contents of the output file.

In D50017#1181305, @ruiu wrote:

I'm wondering if we have to keep the list of symbols that could be added as undefined symbols by LTO. If LTO adds new undefined symbol, we always have to resolve them whatever they are, no?

In D50017#1181200, @ruiu wrote:

It might be a silly question, but are these functions (e.g. udivhi3 or mulxf3) are functions in static archives?

In D49383#1178807, @jgalenson wrote:

Thanks for the explanations! pcc, that example API was very helpful. I have the core part of it working, including integration with cfi-verify and objdump. I'll finish it up and put the patches up for review at some point.

Oh, it looks like @MaskRay fixed this in a different way in rL338128.

It is a char pointer.

LGTM

I suspect that we aren't gaining much from the flags/relocation count/section size because all of these properties typically depend on the section contents anyway, so I left them out.

• Switch to xxhash

In D49877#1177432, @ruiu wrote:

I think we should use a deterministic hash function such as xxHash64 instead of hash_combine to fix this issue, as I believe it's easier and more robust.

In D49383#1175928, @eugenis wrote:

By the state to be tracked, do you mean an index of .got.plt relocations by address?

I'm not sure that DataRefImpl would be a better API because you would need to keep track of a lot of state between getting the info for each PLT entry and that could get complicated. DataRefImpl is better suited for the thin wrappers around object file data structures where you're just enumerating the elements of an array so the between-element state is smaller.

And agreed with eugenis that if you can avoid using the disassembler in findPltEntries that might be simpler.

In D49383#1175843, @jgalenson wrote:

In D49383#1175738, @pcc wrote:

I agree with eugenis that the PLT symbolization should live in lib/Object. That would also help us implement PLT symbolization in llvm-objdump. I was imagining that there would be a function that would take an object file and return a list of (PLT entry address, symbol) pairs. That would seem to be sufficient for llvm-objdump as well as for this code.

I was discussing PLT symbolization offline with eugenis and did a little more investigation. It turns out that it shouldn't be that hard to symbolize the PLT, at least not on aarch64. All of bfd/gold/lld start their PLT entries with a straightforward ADRP/LDR sequence, which you can pretty straighforwardly map onto a GOT entry and then onto a symbol.

As I said initially, I did think putting this logic in core LLVM was the best way, but I still don't understand how to do it (other than the way I did). Do you have more details or a sample patch?

Looks like we already have https://bugs.llvm.org/show_bug.cgi?id=36272 about this.

In D49716#1173198, @peter.smith wrote:

I'm assuming that no merging of the code sections is done if the SHF_LINK_ORDER sections are different.

I agree with eugenis that the PLT symbolization should live in lib/Object. That would also help us implement PLT symbolization in llvm-objdump. I was imagining that there would be a function that would take an object file and return a list of (PLT entry address, symbol) pairs. That would seem to be sufficient for llvm-objdump as well as for this code.

LGTM

LGTM

I'd prefer if we did something like this rather than changing the linker. I see gcc's non-conformance with the coff spec as a bug and if there aren't any real users of gcc+lld I'd prefer us not to have workarounds for that case.

As I mentioned in https://bugs.llvm.org/show_bug.cgi?id=38281 I don't see a good reason to do this. Also extern_weak is not an appropriate linkage for definitions, only declarations.