This concept doesn't explain the int __attribute((weak)) foo; trick used in this test, though.

Apple's platforms are not ELF and don't have the same rules as ELF systems. For instance Apple's platforms use two-level namespace where which imported symbol includes which shared library it is from. At runtime, dyld then only looks in that one shared library.

LGTM

Should there be a COMPILER_RT_ALIAS and COMPILER_RT_HIDDEN_ALIAS macro? Does this change effect the libcompiler_rt.dylib in the OS. In the dylib, I suspect the aliases should be global.

How many symbols are there today? Do we want this overhead of a function call for each? Could you instead return a count and array pointer (like argc and argv) or have a "foreach" function that takes a block and calls it back for each symbol?

Can this be conditionalized to not happen on macOS/iOS? I can see needing this on platforms where pthreads is optional, but it looks like this code will apply to macOS too, which introduces risk there.

LGTM

LGTM

LGTM

LGTM

Thanks for working on this!

LGTM

LGTM

All those comparisons in compareAtomSub() are part of the system linker on darwin. See:

http://www.opensource.apple.com/source/ld64/ld64-123.2.1/src/ld/passes/order_file.cpp

Leaving this pass as-is and adding a new simple pass that ELF (and maybe COFF) uses instead is fine with me.

Sure two passes is conceptually clean, but if it turns out that running sort() twice for mach-o makes linking slower for mach-o than it currently is, then that is not ideal.

There maybe lots of other parts of this LayoutPass that mach-o needs but ELF does not. Why not just prune away at this until you get pass that is fast enough for ELF. Then split this in to two passes. One for mach-o and one for ELF, and let the drivers choose the pass to use.

From what I recall, ELF has the semantics that you cannot just remove an atom in the middle of a section, because there is not enough relocations in the section to fix up the remaining (e.g CALLs to static functions in the same section don't have relocations on them, so if the distance changed because some atom between the two was removed, the CALL now be to the new location). The group reference was a way to keep all atoms in a section together.

but they went away after adding -lc++abi.

It will just work. libstdc++.dylib re-exports symbols from libc++abi.dylib:

I see. Given my comment in the previous patch about parallelism, I was expecting to see a change in the latest patch about when parsing happens. But you changed mach-o parseFile() a while ago to instantiate a File object, but not parse it until doParse() was invoked. So all is good. Just the parseFile() name confusing me.

It still looks like mach-o files are parsed immediately (not in parallel). If that is intentional, because reworking the mach-o logic is complicated, then leave a FIXME comment there.

This seems to have lost the parallel reading/parsing of input files. It looks like parseFile() called by the driver is triggering the actual parsing and parse() called by the Driver in parallel just returns the error code from the parse.

Two comments:

1. This refactoring has taken so many steps, I'm beginning to think one big patch where we can see the end design might have been better...
2. The ErrorFile concept makes me wonder if that should be the approach for all parseFile() implementations. Rather than having a result parameter is that is a vector they append to and returning an error_code, they simply return a vector of Files and any errors are encoded as ErrorFiles in that vector.

The libcompiler_rt.dylib in the OS is built using darwin_bni.mk. I don't know anything that uses darwin_fat.mk.

LGTM

LGTM

The static linker does not need to interpret LC_RPATH in linked dylibs, so there is no need to write a test case that it can.

I see what you are trying to do, but it leaves the pass-generated atoms in an odd state. All the other atoms (from .o files) still have their file() method returning their original file. But the pass-generated atoms now have their file() return the temp mergedFile. When inspecting atoms in the Writer, it is nice to be able to see where they came from. This patch looses that.

The method Reader::parseFile() no longer has an appropriate name (since it no longer parses). It now just constructs a File object.

commited in r 223528

Colin's calculation is based on the bumpvector containing only the *pointer* to the Reference. The current implementation has a vector< SimpleReference>, so the wasted space is not just a pointer size (repeatedly doubled). A SimpleReference is 64 bytes in size, so that value keeps getting doubled (and wasted).

Won't "RefList _reference" leak the nodes of the linked list

But using the BumpVector makes the code cleaner too right ?

Only because you are adding new BumpVector code. We could also add a new BumpList class to ADT and that would make the lld code very clean too.

A downside to BumpVector is that as the vector grows (by adding References), the capacity is doubled by bump allocating a new chunk. All the previous allocations (e.g. if current at 8 elements, the allocs for 1, 2, and 4) are wasted space in the bumpptr pool.

Even if you add a std::vector<std::unique_ptr<Reference>> to lld::File (so the SimpleReferences are deleted when the File is deleted), you still need a std::vector<SimpleReference*> in each SimpleDefineAtom to track which references are used by that atom. And the allocations for that vector would be leaked if the Atom is BumpPtr allocated.

This still seems too complicated. But it is a step in the right direction, so LGTM.

Can you just generalize ShouldInstrumentGlobal() to return false for all cstring_literals sections? The linker is always going to coalesce sections of that type.

The big problem I still see is that this does not fix the issue of multiple different file kinds in one InputElement.

Regarding testing, if you are ok limiting the test to run on darwin, you could have .ll files which you compile and use the system linker to produce the final executable which then has its debug map processed.

The common code looks good to me.

Funny. I just ran into this myself linking libc for arm64. Some of the .o files inside static archives are not 8-byte aligned, so the read64() is asserting. I think the read64() assert may be too aggressive. It may need to fallback to memcpy() for reading unaligned data, rather that asserting.

I'm not a clang driver guy, but it would probably be cleaner to leave AddLinkRuntimeLib() as is, and instead add the CmdArgs.push_back()s inside the if (Sanitize.needsAsanRt()).

LGTM

committed in r220330

The ArchHandlers look much better!

How come in the ArchHandler_x86_64 you use:

write32(loc, value, false);

but inside ArchHandler_arm64 you use:

write32(loc, value, _isBig);

How come in the ArchHandler_x86_64 you use:

write32(loc, value, false);

but inside

LGTM

LGTM

LGTM

The MachOObjectFile constructor is already walking the load commands. If there is a LC_UUID, you can have the constructor save it off in an ivar.

committed in r219260

fixes from latest Rafael comments.

Rework to use getFileAux()

committed in r218893

committed in r218718

should we add a special case to the reader to convert them to weak undefines?

That is what I was thinking. But you should investigate how this works with the gnu linker. Does the linker really hard code "__tls_get_addr" to be special? Or does it always implicitly link with ld.so?

to complete linking an executable, all symbols must be resolved and we cannot finish it with undefined references

I guess I was not clear. I did not mean where does the *definition* of tls_get_addr come from. I was asking where does the *undefine* for tls_get_addr come from? If that undefine is "weak", that tells the linker that if no definition is found, to assume it was in some DSO. Seems like that is what you are special casing tls_get_addr to do.

One area of linking not fleshed out in lld is the "canBeNullAtBuildtime" attribute for UndefinedSymbols. That was intended to model ELF weak undefines. Is that how tls_get_addr works with gnu ld? If so, then getting canBeNullAtBuildtime to work in general could solve this specific issue without needing to special case tls_get_addr.

There are two issues:

committed in r218463

Add doxygen comments and unit tests.

Is there some C++ language feature we can use to mark the Twine class in a way that causes a warning/error on these incorrect usages?

1. Don't most build system invoke the compiler to invoke the linker? How are you overriding the linker? Can you redirect to a script that invokes lld with -flavor and $@
2. Yes, shifting argv is not right. We need the -flavor and its arg removed, so argv[0] is still the command name.

The original design was that -flavor had to be the first arg (so incrementing argv worked). But now the use of ParseArgs() means that -flavor is being searched for everywhere in the arg list. That is a mismatch.

I assume that preload() return immediately, and that it is expected to spin off some thread to parse an archive member? If so, we have no overall throttle on how many threads will be started (a hundred undefines could spin up 100 threads). Also, how is the archive reader to coordinate if the Resolver gets to the point it really wants an object file to fulfill and undefine but some other thread is busy parsing that member?

Interesting. That is the way I used to write C++, but I thought the LLVM convention preferred anonymous namespaces. After reading the current LLVM convention doc, I see that static is better in this case. I'll retrain my fingers :-)

LGTM

committed as r217566

Adding support for "ld" on *nix meaning use-gnu-driver is reasonable, but it is an additional feature (which I have no way to test).

A few nits, but over all a great start.

LGTM

committed in r217112