[PATCH] D129107: [BOLT][HUGIFY] adds huge pages support of PIE/no-PIE binaries

Fri Aug 19 11:40:07 PDT 2022

rafauler added a comment.

In D129107#3731109 <https://reviews.llvm.org/D129107#3731109>, @yavtuk wrote:

> In D129107#3730258 <https://reviews.llvm.org/D129107#3730258>, @rafauler wrote:
>
>> Thanks for explaining, Alexey.
>>
>> Here's what I understood:  from the left side, if we take the last allocated address according to PT_LOAD segments, and then try to put a new segment right after it, aligned to 2MB, old loader somehow ignores p_align and loads that into a 4KB address. But if we give 2MB more space, it does align. This seems quite arbitrary, do you confirm this is what you're seeing and that this is the right fix? If yes, this is odd enough that I think it makes sense for us to put this under a special option -hugify=oldkernel.
>
> Yes, you are right, it's the bug inside kernel loader. Maybe we should exclude the support for old kernel, until someone ask about it?
>
>> From the right side, we create multiple ELF sections in the same segment and that crashes the loader. The loader would like to see a single segment aligned at 2MB. Is that it? This also would be better under a special -hugify=oldkernel option.
>>
>> And then restore the behavior in the hugify library to copy all code instead of just hot code, otherwise we're going to crash if -hugify is used instead of -hugify=oldkernel.
>>
>> When you guard the code under -hugify=oldkernel, please add a comment linking to this diff for the discussion, so somebody reading the code has the background to understand why this was added in the first place.  Weird behaviors such as this need to be thoroughly documented, so it is clear we're compensating for a bug in the system.
>
> Rafael are you sure that we should copy all section from RE segment?
> Below the screenshot of .text and .text.cold sections for one of the binaries which I have
>
> F24189483: image.png <https://reviews.llvm.org/F24189483>

Sorry, I wasn't clear. Not all sections, but restore the previous behavior (the behavior that we have right now in trunk), which fills the remaining of the huge page with the start of cold. If we don't do this, we crash. If you remove the right side padding, you will notice the crash.

I'm fine with either (keeping the old kernel support under a flag, or removing it).  Btw thanks for your work, I appreciate it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129107