- User Since
- Jan 3 2013, 3:07 PM (341 w, 5 d)
May 6 2019
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.
Jan 25 2019
Apr 10 2018
Oct 31 2016
Sep 30 2016
Jul 19 2016
Jan 7 2016
Dec 11 2015
Dec 10 2015
Dec 8 2015
Jul 24 2015
Jun 24 2015
May 20 2015
Mar 2 2015
Thanks for working on this!
Mar 1 2015
Feb 13 2015
Feb 5 2015
Jan 27 2015
All those comparisons in compareAtomSub() are part of the system linker on darwin. See:
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.
Jan 26 2015
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.
Jan 14 2015
but they went away after adding -lc++abi.
Jan 13 2015
It will just work. libstdc++.dylib re-exports symbols from libc++abi.dylib:
Jan 12 2015
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.
Jan 7 2015
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.
Jan 6 2015
- 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...
- 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.
Jan 5 2015
Dec 23 2014
The libcompiler_rt.dylib in the OS is built using darwin_bni.mk. I don't know anything that uses darwin_fat.mk.
Dec 19 2014
Dec 18 2014
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.
Dec 12 2014
The method Reader::parseFile() no longer has an appropriate name (since it no longer parses). It now just constructs a File object.
Dec 5 2014
commited in r 223528
Dec 4 2014
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.
Dec 3 2014
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.
Dec 2 2014
Can you just generalize ShouldInstrumentGlobal() to return false for all cstring_literals sections? The linker is always going to coalesce sections of that type.
Nov 21 2014
The big problem I still see is that this does not fix the issue of multiple different file kinds in one InputElement.
Nov 12 2014
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.
Nov 6 2014
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.
Oct 29 2014
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()).
Oct 27 2014
Oct 21 2014
Oct 20 2014
Oct 16 2014
The ArchHandlers look much better!
Oct 15 2014
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);
Oct 14 2014
Oct 13 2014
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.
Oct 7 2014
committed in r219260
fixes from latest Rafael comments.
Rework to use getFileAux()
Oct 2 2014
committed in r218893
Oct 1 2014
Sep 30 2014
committed in r218718
Sep 26 2014
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.
Sep 25 2014
There are two issues:
committed in r218463
Sep 19 2014
Add doxygen comments and unit tests.
Sep 18 2014
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?
Sep 17 2014
- 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 $@
- Yes, shifting argv is not right. We need the -flavor and its arg removed, so argv 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.
Sep 12 2014
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 :-)
Sep 11 2014
Sep 10 2014
committed as r217566
Sep 9 2014
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.
Sep 5 2014
Sep 3 2014
committed in r217112
Just add a StringMap ivar to ELFLinkingContext. Have notifySymbolTableCoalesce() add the atom name to the map when overriding a weak atom. In buildDynamicSymbolTable() check for atom->dynamicExport() OR (atom->scope() == scopeGlobal and its name is in the StringMap).
Sep 2 2014
LGTM. Thanks for tracking this down.
Aug 29 2014
committed in r216808