[PATCH] D128382: [LLD] Two tweaks to symbol ordering scheme

Fri Jul 8 10:50:39 PDT 2022

MaskRay added a comment.

In D128382#3639333 <https://reviews.llvm.org/D128382#3639333>, @smeenai wrote:

> In D128382#3637828 <https://reviews.llvm.org/D128382#3637828>, @yozhu wrote:
>
>> In D128382#3637822 <https://reviews.llvm.org/D128382#3637822>, @MaskRay wrote:
>>
>>> In D128382#3623746 <https://reviews.llvm.org/D128382#3623746>, @yozhu wrote:
>>>
>>>> In D128382#3620661 <https://reviews.llvm.org/D128382#3620661>, @MaskRay wrote:
>>>>
>>>>> In D128382#3611295 <https://reviews.llvm.org/D128382#3611295>, @yozhu wrote:
>>>>>
>>>>>> In D128382#3611207 <https://reviews.llvm.org/D128382#3611207>, @MaskRay wrote:
>>>>>>
>>>>>>> In D128382#3611193 <https://reviews.llvm.org/D128382#3611193>, @yozhu wrote:
>>>>>>>
>>>>>>>> In D128382#3610276 <https://reviews.llvm.org/D128382#3610276>, @MaskRay wrote:
>>>>>>>>
>>>>>>>>> In D128382#3609648 <https://reviews.llvm.org/D128382#3609648>, @lanza wrote:
>>>>>>>>>
>>>>>>>>>> One interesting note Shoaib and I just discovered is that ~16.5% of the libraries in one of our major apps have `.text` sections starting on the first page of the dso. Placing hot sections at the front of the `.text` section would give us a slice of a "free page" that, with the current algorithm, would just be cold code that's less likely to be used. I imagine this isn't a substantial difference but it's probably slightly in favor of placing hot code first here.
>>>>>>>>>
>>>>>>>>> Thanks. This is fine, even if not that large. The patch should use `osec->flags & SHF_EXECINSTR` instead of `executableInputSections` for simplification. Then the complexity/benefit trade-off will look fine.
>>>>>>>>
>>>>>>>> I didn't check output section's attribute but instead check on each input section because:
>>>>>>>>
>>>>>>>> - It could be that the output section contains .text (RX) contributions as well as .rdata (R) contributions, like read-only data is merged into text section.
>>>>>>>
>>>>>>> This implies a linker script placing rodata into an executable output section.
>>>>>>> This is a corner case and the operation here does not matter. Just be simple.
>>>>>>
>>>>>> Even if it is a rare or corner case, it is still a valid case.  Counting `executableInputSections` doesn't add many cycles.  Why not just cover all the possible cases?
>>>>>
>>>>> For input rodata in an output executable section, since the for loop doesn't precisely track whether an input section is executable or not, it is as if all rodata are executable as well.
>>>>> Your `executableInputSections` rule doesn't cover more cases.
>>>>
>>>> Input section's attribute can be precisely tracked since the linker has no reason to change input section's attribute once it is read from input OBJ.  For example, if we have source code like
>>>>
>>>>   __attribute__((section(".text.rox")))
>>>>   const int x = 5;
>>>>   
>>>>   __attribute__((section(".text.row")))
>>>>   const int w = 5;
>>>>   
>>>>   int foo() { return x; }
>>>>   int bar() { return w; }
>>>>
>>>> and we compile it with `-fdata-sections -ffunction-sections` and then link the OBJ using a linker script like
>>>>
>>>>   SECTIONS
>>>>   {
>>>>     ttt 0x1000 :
>>>>     {
>>>>       tt.o (.text.*)
>>>>     }
>>>>   }
>>>>
>>>> Then in the `for` loop we will see two input sections are executable and the other two are not.
>>>
>>> I have mentioned that this is a corner case we don't necessarily try to optimize. It takes code.
>>
>> As @lanza  mentioned above that `~16.5% of the libraries in one of our major apps have .text sections starting on the first page of the dso`, this is actually **not** a corner case for us.  And given how much code are already there for symbol ordering in the linker, this change doesn't add that much overhead or complexity.
>>
>>> Note: even if some linker scripts combine rodata and code sections in one output section, such an output section is typically far smaller than `target->getThunkSectionSpacing()`
>>
>> Why typically it has to be far smaller?  The linker doesn't restrict the use of linker scripts for combining RO and X sections to form a **large** output section.  And typically code plus read-only data takes significant percentage of binary size.

My comment discusses how likely a user combines rodata and code in one output section. It's not about the extent of all rodata and code output sections.

>>> I don't think it is useful to special case `executableInputSections > 1`
>>
>> Well, it may not be useful to you or Google, but this is useful to us (as mentioned above) and this is the linker for open source community (not just you or your team).  What are the rules here to determine what is useful and what not?
>
> To clarify, I don't think there's an objection to the change in general now (per @MaskRay's earlier comment after @lanza posted the 16.5% number). It's just a question of whether to count individual executable input sections or use the output section's executable-ness to make the ordering determination.

Right. The code can spend fewer lines to address the issue. This is one of the moments I just wanted to commandeer the patch and satisfying the user needs instead of wasting more and more time arguing.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128382/new/

https://reviews.llvm.org/D128382