feat: Implement SORT() directive for linker scripts#1857
feat: Implement SORT() directive for linker scripts#1857Abh10705 wants to merge 1 commit intowild-linker:mainfrom
Conversation
- Introduces Harvester engine in layout.rs to collect and sort sections lexicographically. - Implements physical section reordering in elf_writer.rs with O(1) skip logic for optimized performance. - Adds comprehensive integration test and 50k-section stress test verification. - Verified physical byte layout is contiguous and alphabetical via objdump. Benchmark (Warm cache): - GNU ld: 0.221s - wild: 0.093s Fixes wild-linker#1661
| input: &mut &'input BStr, | ||
| ) -> winnow::Result<SectionPattern<'input>> { | ||
| // Handling SORT(...) wrapper: SORT is an alias for SORT_BY_NAME in GNU ld. | ||
| if input.starts_with(b"SORT") { |
There was a problem hiding this comment.
I'm pretty sure there's a function somewhere in this file that can be used to combine this check with the consumption of the relevant prefix.
| // Consume "SORT" | ||
| "SORT".parse_next(input)?; | ||
| skip_comments_and_whitespace(input)?; | ||
| // Expect '(' |
There was a problem hiding this comment.
This comment doesn't seem to tell us much we couldn't tell by looking at the next line. Best to remove such comments
|
|
||
| Ok(SectionPattern { name, sorted: true }) | ||
| } else { | ||
| // Original behavior: bare pattern |
There was a problem hiding this comment.
Comments in code should be written for the reader of the code post-merging, not for the reader of the PR. To someone coming back to this code after the PR is merged, talking about "Original behaviour" won't make much sense. The exception is a temporary comment to the reviewer that you intend to remove before merging, but those should generally be clear that that's what they are.
| __attribute__((used, section(".text.func_b"))) int func_b() { return 2; } | ||
|
|
||
| __attribute__((naked)) void _start() { | ||
| asm volatile( |
There was a problem hiding this comment.
Can you look at a few other tests to see how we normally structure them? This test is quite different and for example won't work on anything other than x86_64.
| for file in &mut group.files { | ||
| if let FileLayoutState::Object(obj) = file { | ||
| // Steal the sticky notes | ||
| let notes = &obj.script_sorted_sections; |
There was a problem hiding this comment.
In the linker, "notes" generally refers to GNU notes sections. Is that what this is working with? If not, then it might be better to use a different name
| size: capacity, | ||
| alignment: part_id.alignment(output_sections), | ||
| mem_offset: 0, | ||
| name: note.name.to_vec(), |
There was a problem hiding this comment.
For performance reasons, we try to avoid copying things to the heap unless absolutely necessary. I don't think this copy should be necessary
| sframe_ranges.push(offset..offset + len); | ||
| } | ||
|
|
||
| if let Some(harvested) = resources.harvested_sections_registry.get(&(self.file_id, sec_idx)) { |
There was a problem hiding this comment.
The vast majority of linking won't be using the SORT feature. In those cases, we want the performance to be basically unchanged from before the sort feature was implemented. Doing a hashmap lookup here will very likely have an effect on performance, so we should look for alternatives.
| pub(crate) name: Vec<u8>, | ||
| } | ||
|
|
||
| fn harvest_and_sort_script_sections<'data, P: Platform>( |
There was a problem hiding this comment.
This function looks pretty expensive and runs on a single thread. It looks like it currently always runs, even if sorted sections are not in use. It's OK if linking is slowed down when section sorting is active, but we definitely shouldn't slow down when it's inactive.
| // Sort the harvested sections according to the requested criteria. | ||
| // Linearize them in memory, starting from the base offset of their respective output section part, and advancing by the size of each section. | ||
| let mut script_sorted_sections = script_sorted_sections; | ||
| script_sorted_sections.sort_by(|a, b| a.name.cmp(&b.name)); |
There was a problem hiding this comment.
I think sort_by_key can probably be used here and would be simpler
| for sec in &mut script_sorted_sections { | ||
| sec.mem_offset = current_offset; // Assign the real hex address! | ||
| current_offset += sec.size; // Advance the address by the function's size | ||
| // Adding to our instant-lookup HashMap |
There was a problem hiding this comment.
Hashmap lookups definitely aren't instant. In the context of the linker, they're actually pretty expensive and something that we only tend to use when we have exhausted less expensive options. When we do use hashmaps, we generally select one of the faster hash algorithms. I haven't yet put too much thought into whether this hashmap is actually needed. Have you considered what alternatives might exist?
2 participants
Introduces Harvester engine in layout.rs to collect and sort sections alphabetically.
Implements physical section reordering in elf_writer.rs with O(1) skip logic for optimized performance.
Adds comprehensive integration test and 50k-section stress test verification.
Verified physical byte layout is contiguous and alphabetical via objdump.
Benchmark :
GNU ld: 0.221s
wild: 0.093s
Fixes #1661