Differential D95199
[ELF] Write output sections in PT_LOAD segment order Authored by pattop on Jan 21 2021, 7:04 PM.
Details Previously if sections in a link script were not in program segment The new out-of-order-sections test case previously failed with: ld.lld: error: section .foo file range overlaps with .bar
Diff Detail
Event TimelineComment Actions For out-of-order-sections.s, you could just swap foo and bar. The few updated tests seem to have undesired sh_offset changes. GNU ld's placement rules are much more complex. We implement sufficient parts to be compatible with a wide range of linker scripts and may require updating the linker scripts in some cases. Comment Actions Yes, this is true. I crafted the test case specifically to highlight the problem.
I examined these and to the best of my (limited) understanding the resultant offset changes looked ok, however, I may have missed something important.
My desire to have this case work comes from a real world embedded project. In conjunction with https://reviews.llvm.org/D95198 it makes lld far more usable in this context. Comment Actions If the direction of the patch is acceptable I can provide a detailed analysis of each required testcase adjustment. Comment Actions In fact, if https://reviews.llvm.org/D95198 is acceptable it is a separate justification for this change. In order to place program headers somewhere other than the lowest VMA in the program image something like this needs to happen. Comment Actions I don't agree that these are arbitrary issues. I proposed this and D95198 as I encountered these issues in the real world while porting projects which have been working with GNU ld for many years. Sure, this one can be worked around by reordering the link script, but it's certainly confusing for a user coming from GNU ld. Especially as the error message (pertaining to file range) does not really point to the underlying problem. It was very surprising to me that LLD requires sections in a link script to be in a special order. Comment Actions This looks to me it solves a specific corner case, but I really don't feel I have enough knowledges about embedded world/scripts to say whether it is a good or bad thing to support it. Comment Actions Apologies for the delay in responding. As I understand it, this would only cause a problem with PHDRS? Our implicit creation of program headers should already be in OutputSection order. Personally I'd be in favour of making our PHDRS support more closely match ld.bfd. Both Arm and RiscV are making more of an effort to push LLVM rather than GCC in the embedded space (https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm) and LLD is the linker of choice. Avoiding migration problems for embedded developers is helpful. Yes it is true that people can rewrite their linker scripts in most cases, however it normally needs an expert to debug the limitation from an overlap error message. For this particular change, I've made a few comments and I would like an explanation for some of the file-offset differences and the addition of foo implicit-program-header.test? I can't immediately see from the script why there would be any change in output section order.
Comment Actions Thanks for taking the time to review. This causes problems any time the sections are not in segment order. I agree that it would be good if lld's behaviours can more closely match BFD ld. https://lld.llvm.org/ even says that lld is supposed to be a drop in replacement for GNU linkers. I've commented on most of the test case changes and will update the patch to address your review comments.
Comment Actions Updated comments. Comment Actions Thanks for the update, I'll try and take a look later in the week. Will have to have a think about the zero size section case. If we haven't assigned it to a PT_LOAD and it defines no symbol it may be possible to not put it in the object file. Comment Actions No problem. Hopefully my comments on the test cases make some sense. Unfortunately some of your code comments moved around a bit when I updated the patch. Doing something with zero size sections could be a nice cleanup, but it looks like it would cause some churn in the tests. Either way, I think it's separate to this issue. Comment Actions I've had a look at a couple of the tests (not looked at the others) there look to be some ordering problems with some special cases like partition sections and .tbss. To find these I checked the order of writing sections with the new and old. As mentioned in the comment I don't know much about the partition mechanism, as such I'd recommend making sure that there were no explainable differences in those tests. Will try and find some more time to look at the others later in the week/weekend.
Comment Actions Maintain section address order as much as possible when assigning file offsets. This resolves most file offset adjustments in tests previously required due to The logic in assignFileOffsets could possibly be cleaner but I can't quite see Comment Actions Thanks for the updates. My day job has been very busy of late so I've not got a lot of spare time to look. As it stands I'm still a bit nervous about the test changes, it is also worth mentioing that I'd want at least one of grimar and MaskRay to agree on any changes in this area as they were both initially hesitant. I did have a thought on what might make this (and your other patch) easier to upstream. Instead of changing the code directly keep the existing code below: // Layout SHF_ALLOC sections before non-SHF_ALLOC sections. A non-SHF_ALLOC // will not occupy file offsets contained by a PT_LOAD. Given that the problem with ordering only exists with PHDR commands, then we can do something like (pseudo code): if linker script has program headers {
reorder Output Sections covered by PT_LOAD
}
...
// Layout SHF_ALLOC sections before non-SHF_ALLOC sections. A non-SHF_ALLOC
// will not occupy file offsets contained by a PT_LOAD.While it may be a bit more code, it is isolated to when PT_LOAD is used, which should limit the impact on the existing tests to the tests using PHDR. From reading the partition documentation (see below) this feature is incompatible with linker scripts and PHDR anyway. This should make it easier to review, although I can't speak for MaskRay and grimar here. For the partitions bit: References for the partition The author of partitions is pcc , he may be able to comment on the changes, although is also frequently busy. Comment Actions The change to partition-synthetic-sections.s is not ideal. Originally the sections have non-decreasing offsets. With the change, .part.end (NOBITS) is the first to be assigned an offset 0x3150. In the next iteration, sh_offset(part1) is 0x4000 (was: 0x3000), wasting one page. The change to implicit-program-header.test is not ideal, either. You can use the existing assembly, or add a .bar and change PT_NOTE to PT_LOAD. In both cases GNU ld places .foo before .text. The current behavior is similar. With this patch, .foo will be assigned an offset from the next page. In the presence of PHDRS, I think the new behavior is pretty much desired. It remains uncertain for me whether making it the behavior in the absence of PHDRS is the right choice. It gives my a feeling to address specific corner case while regressing normal case behaviors and greatly increasing complexity.
Comment Actions Thanks for the feedback! peter.smith's comment suggesting reordering sections made me rethink my approach to the problem. I don't think we can reorder outputSections as many parts of the code depend on this. What I have done instead is add a segment to section mapping in the PhdrEntry struct. I think this is a good approach as it allows us to sort the sections in a segment by address without affecting anything else. Doing this greatly simplifies the logic in assignFileOffsets(). As a further cleanup the firstSec and lastSec members of PhdrEntry can probably become simple inline functions returning the first and last entry in the sections vector. I didn't want to do this unless the direction is OK to simplify review. I have limited this to only affect the output when a PHDRS command is in use as per peter.smith's suggestion, but that won't cover all cases. A link script containing just a SECTIONS command can place sections at arbitrary addresses which also needs this new logic to work properly. Unfortunately, doing so with the current logic brings back some previously discussed undesirable side effects. As MaskRay said the new output of implicit-program-header.test isn't great. As far as I can tell the reason this happens is beause we place .dynsym .hash and .dynstr before .foo rather than after .text. I'm not sure how to resolve this. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||