[PATCH] D137620: [BOLT] Don't align .text to pageAlign

Thu Nov 17 12:56:33 PST 2022

maksfb added a comment.

In D137620#3934664 <https://reviews.llvm.org/D137620#3934664>, @treapster wrote:

>> To me, it appears there are two outstanding issues that we need to solve. One is that we use large pages by default. Without hugify/hottext it doesn't make sense unless dynamic loader supports large pages.
>
> My understanding is that newer kernels/loaders support large pages and can properly map them without the use of -hugify, so it makes sense to use large pages by default. I wouldn't call it an issue, if you don't like it you can always use -no-huge-pages.

Could you point to the kernel patch/version that enables it? It will be good to test the functionality.

> In D137620#3934504 <https://reviews.llvm.org/D137620#3934504>, @maksfb wrote:
>
>> Just using 64-byte alignment and relying on the containing segment being aligned to the page size will not always work. One example of the latter is "-use-old-text".
>
> Why it will not work with -use-old-text? The whole point of this option is to put new text in place of old, and by trying to align it to 2 MB you make it unachievable unless the binary was prelinked with huge padding.

What exactly is "unachievable"? For one of our apps, we use ICF to save ~20MB of code and the new code fits into the original space with 2MB alignment.

> I think this patch just makes defaults more reasonable, otherwise you can always align however you want.

>From the performance perspective, the default alignment of 64 bytes for .text does not sound reasonable to me . It should be page-aligned.

> And i also don't understand why MaxAlignBytes might be better

It has an advantage to explicitly express the intention of not using more than MaxAlignBytes for alignment. The current patch has the assumption of having "small" program header before .text in a page-aligned segment which makes it fragile.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137620