- User Since
- Apr 11 2016, 8:56 AM (54 w, 22 h)
I've created D32485 for an alternative implementation that just makes the link order sections orphans. I think that this ends up being a bit simpler overall.
The main difficulty in creating and adding the Orphan OutputSectionCommand in addOrphanSections() is that we don't have a good idea of where in the order of OutputSectionCommands to insert them. The placeOrphanSections() is intentionally run immediately after sortSections() has been called so that it can insert the OutputSectionCommands in the right place. There is a comment in Writer.c in sortSections() that says:
// The way we define an order then is: // * First put script sections at the start and sort the script and // non-script sections independently. // * Move each non-script section to its preferred position. We try // to put each section in the last position where it it can share // a PT_LOAD.
It may be possible to create the Orphan OutputSectionCommands early and have the comparison routine work out the ordering, although there is a comment in the same block as the one above that claims that coming up with a strict weak ordering to get the desired result is not easy.
I can think of some options, although each has their own trade-off. Apologies for the length of the answer.
Fri, Apr 21
I don't have any objections to it being removed. It can be reintroduced if we need it later.
I've spotted what I think is one mistake, R_AARCH64_P32_TLSDESC_ADR_PREL19 should be R_AARCH64_P32_TLSDESC_ADR_PREL21. I've put some suggestions for some missing test cases as well. Other than that I can't spot any obvious problems.
Wed, Apr 19
Now committed r300687.
I agree that the code is now redundant and can be removed. It used to be the case that addIgnored() would define the symbol even when there was an existing definition and cause a duplicate error. It looks like this was later fixed in -r287125 Don't error if __tls_get_addr is defined.
Thu, Apr 13
Marking as abandoned as this patch is for communication only.
Thanks for the review. I'll hold off on committing this till I get back from the UK Easter public holidays just in case this causes problems outside of the tests and I'm not around to fix them.
Tue, Apr 11
Updated diff to address review comments.
Apologies for leaving PHDRS in, I had originally attempted to merge fixSectionAlignments() into FabricateSectionAlignments(), but decided against it.
Mon, Apr 10
Fri, Apr 7
Yes, we can use Config->Shared instead, thanks for pointing that out. I've updated to use Config->Shared and have put back the use of TlsOffsetRel in the Got, it could be possible to add another field to make it use a more regular relocation code, but I'm not sure that is a net win in readability. I'm happy to make the change if it is important to others though.
I've used the regular relocation code for this change.
Thu, Apr 6
I've split this into 3 reviews:
D31748 Split handleNoRelaxTlsRelocation into ARM and Mips specific impls
D31749 Fix ARM TLS global dynamic TlsOffsetRel for non-preemtible symbols
D31751 Tidy up handleARMTlsRelocation [NFC]
To separate out the refactoring from the fix.
Wed, Apr 5
I think at least some of the difficulty in understanding the code here is that there are too many somewhat arbitrary ARM and Mips specific bits of code here. I've updated the diff to split handleNoRelaxTlsRelocation() into an ARM and Mips specific function, this allows us to take out some of the special cases.
Rebased to take account of changes in D31654
Rebase to account for changes made in D31654
Rebased patch after earlier changes made to D31654
Rebasing patch on top of recent linkerscript refactoring.
That looks good to me. I agree that it is the better solution to not assume anything about the relative locations of .got and .got.plt
Tue, Apr 4
Abandoning this revision in favour of D31656, which is more general and can handle range extension thunks.
Mon, Apr 3
Fri, Mar 31
Ok now merged as r299212
I can do, although there isn't any special permissions to commit to lld, if you can commit to llvm then you can commit to lld. Let me know if you'd prefer to do it yourself? Otherwise I'll do it in an hour.
My understanding is that this test failed when a change went in to llvm-mc to not emit the mapping symbol $d.0 when the section contains only data. The test here is checking for the $d.0. Adding the nop forces the creation of a $a.0 which then forces the creation of the $d.0, this will make the test more robust against llvm-mc changes.
Wed, Mar 29
I think the lld test in question is expecting a $d.0 mapping symbol because up to now this is what llvm-objdump has been outputting. It is not strictly necessary for the test which is checking the behaviour of the R_ARM_TARGET1 relocation in response to the --target1-rel flag. It just so happens that .text has been used for the test for what looks like convenience. Strictly speaking we would never see a R_ARM_TARGET1 relocation in an executable section (Should only be present in SHT_INIT_ARRAY or SHT_FINI_ARRAY) sections so I don't see any problem with fixing the lld test to match the new llvm-objdump output.
Mar 24 2017
I haven't got a good answer for why !isPreemptible() is there. My best guess is that it started as isPreemptible(), there was originally no !Config->Pic, when the !Config->Pic got added this made the !isPreemptible() redundant in that case. The isPreemptible() is important in the Target->TlsOffsetRel case as even for a shared library the linker will know the offset of the Symbol in the TLS block if the symbol isn't preemptible.
Mar 23 2017
Hello, thanks for giving me the chance to review.
Mar 7 2017
Apologies, I'm a bit confused about the status of this patch after the last comment/status change, I'll assume this is still waiting for further comments for now.
Mar 6 2017
Mar 2 2017
Rebased and updated the patch.
Feb 22 2017
Thanks for the comments, I'll pick this back up when I get back to the office next week. It is helpful to know what the intentions are with respect to naming.
Feb 20 2017
Thank you very much for the comments, I've updated the diff to incorporate the suggestions.
Feb 17 2017
Updated diff to:
- Rebased on top of D30085 (not reviewed yet) which allows us to add local symbols after globals
- Moved the addition of symbols to the GNU Hash Table to a separate member function that can be called immediately after the dynamic symbols have been added.
- Change finalizeAllocSize() to updateAllocSize(). The intention is that this will be called each time thunks are added, and immediately prior to thunks being added, to prevent the MipsGot missing an additional page.
- Hopefully addressed any review comments that still apply
Feb 16 2017
Thank you both for the comments. I'm thinking that it may be better to rework this a little bit and resubmit as the MipsGot::PageIndex calculation breaks my assertion that adding range extension thunks can't affect the size of the SyntheticSections. The size of the MipsGot is proportional to the size of the OutputSections size (modulo 64k), while the amount of Thunks added is likely to be smaller than 64k adding a Thunk could potentially extend over a page boundary.
Feb 15 2017
Feb 8 2017
Updated the diff with an attempt to use a single class for both the Plt and IPlt. I've used HeaderSize to distinguish between them as that removes a few conditional statements.
Thank you very much for the comments. I've updated the diff to make the requested changes.
Feb 7 2017
Feb 1 2017
Thanks for the update, I don't have any more comments.
Jan 31 2017
Jan 27 2017
I've checked that the relocation name is correct. I'm a bit suspicious of allowing the fixup_aarch64_ldst_imm12_scale8 in LP32, with a 32-bit sized GOT I think that this is likely to be a programming mistake that we should probably error on. I'm not hugely familiar with lp32 programming so there may be a case for it that I'm missing?
Thanks for the suggestions. Now committed r293283. I'll now go about thinking about how best to add range extension thunks.
Jan 26 2017
I think that those changes address my previous comments, thanks for the update. I've found a couple of what look like typos in the comments. I'm not familiar with rtdyld's organizational and coding conventions so probably best that let an owner take it from here.
Thanks very much for the comments.
Jan 25 2017
Jan 24 2017
Updated patch to remove Saver.save for mapping symbol entries.
That is possible although in many cases the disassembler can derive that information without an additional symbol. For example there is some functionality in GNU objdump to display the symbol name of the PLT from some hard-coded choice of PLT size made when building objdump. For example, from hello world compiled and linked with ld.bfd:
Jan 23 2017
Thanks for the comments. I've updated the diff.
Thank you for the comments. Updated to use std::stable_partition and rebased on top of D29021.
Removed all code in SymbolTable and rebased the patch on top of D29021.
Jan 20 2017
That looks correct to me according to ELF for the 64-bit ARM architecture definitions of R_AARCH_64_LDST8_ABS_LO12_NC and R_AARCH_64_LDST16_ABS_LO12_NC. Will be worth waiting to see if there are any other comments.
I've not added any uses of adding a local symbol to a synthetic section. I've checked that it works by adding mapping symbols to ARM PLT entries, this is quite a large diff due to test updates so I've not included it in the review. I've attached the patch for reference
Jan 18 2017
Jan 17 2017
Jan 4 2017
I think the code changes and test look fine, but I think you should get this approved by one of the approvers of https://reviews.llvm.org/D6079, which is where the x86_64 large code model change was made along with the justification for it.
Thanks for the review, committed revision 290951
Jan 3 2017
Rebased patch to account for last couple of weeks changes, no functional changes. Happy New Year!
Dec 15 2016
Thanks for the update. I've added the comments.
Dec 14 2016
I think it depends on what the precise meaning of Shared and Undefined. At the moment I think the distinction is:
- The symbol is defined in a known shared object.
- The symbol details such as type, binding and version are known.
- The shared object has been added as a dependency to the output.
- The shared object defining the symbol can be found from the SymbolBody.
- We have a reference to a symbol but no definition.
- When linking a shared library we assume that the dynamic loader will find a suitable definition from the executable/shared library or error.
- In all other cases (besides relocatable -r) an undefined non-weak symbol is an error.
- There is no input file associated with the symbol.
Dec 13 2016
Thanks for the review. My first idea was to put a flag in Config, but that didn't seem right either. If we were wanting to merge the build attributes sections we'd probably want to do so via a SyntheticSection so I thought it would be easier to start that way.
The relocation calculation looks right, but I'm not convinced about the ulittle64_t with the information I have right now. I suggest that if D27609 is approved this should follow.
Dec 12 2016
Dec 9 2016
I've altered the commit message to say glibc ld.so. Hope that's ok. R289198