[PATCH] D64930: [ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D64930#1636634 <https://reviews.llvm.org/D64930#1636634>, @ruiu wrote:

> In D64930#1636625 <https://reviews.llvm.org/D64930#1636625>, @MaskRay wrote:
>
> > In D64930#1626816 <https://reviews.llvm.org/D64930#1626816>, @MaskRay wrote:
> >
> > > In D64930#1626651 <https://reviews.llvm.org/D64930#1626651>, @peter.smith wrote:
> > >
> > > > I'm personally in favour of accepting D64930 <https://reviews.llvm.org/D64930> and D64906 <https://reviews.llvm.org/D64906> (I think that there is also D65865 <https://reviews.llvm.org/D65865> on X86) as I think that this is an important size optimization already performed by Gold and BFD. Thinking about how to progress this further, I think it would be good to write a summary of where each of these reviews is at and what the plan is for this feature. For example:
> > >
> > >
> > > Thanks for pushing forward this patch series!
> > >
> > > > - Have all the comments been addressed, I think the main concern was about TLS?
> > >
> > > All addressed. There was concern about TLS but I aligned p_vaddr/p_offset to p_align of PT_TLS.
> > >
> > > > - Which targets do we want this to apply to, all of them, or just those with a 64k page size?
> > >
> > > All, probably except x86_64 (with max-page-size=4096).
> > >
> > > `bool enable = config->emachine != EM_X86_64 || config->maxPageSize != 4096;`
> > >
> > > > - What is the dependency tree of the reviews, I think D64906 <https://reviews.llvm.org/D64906> is a pre-requisite for all of them?
> > >
> > > A tree with a single internal node:) Yes, D64906 <https://reviews.llvm.org/D64906> is a pre-requisite for all of them.
> > >
> > >   D64906 base, PPC64
> > >     D64930 AArch64 47 tests
> > >     D65865 i386 33 tests
> > >     Dxxxxx ARM (30~50 tests)
> > >     Dxxxxx MIPS
> > >     rLLDxxxxx RISC-V <10 tests
> > >     rLLD PPC32 <10 tests
> > >     Dxxxxx x86_64 with maxpagesize != 4096
> > >
> > >
> > > Each new target can be enabled independently. The code change is trivial:
> > >
> > >   -      bool enable = config->emachine == EM_PPC64;
> > >   +      bool enable =
> > >   +          config->emachine == EM_AARCH64 || config->emachine == EM_PPC64;
> > >
> > >
> > > The only problem is the number of the affected tests. Due to the layout change nature of the patch series, lots of addresses have to be updated. x86_64 with maxpagesize=4096 is estimated to affect 300+ tests.
> > >
> > > > - Does anyone have any objection, or are they happy for individual people active in a backend to approve that part?
> > >
> > > We haven't heard from MIPS yet. @atanasyan
> >
> >
> > @ruiu Let's move D64906 <https://reviews.llvm.org/D64906>, D64930 <https://reviews.llvm.org/D64930> and D65865 <https://reviews.llvm.org/D65865> forward?
>
>
> Yes, I'm in favor of these patches. Please move forward. Are there any remaining concerns?


Great! No concerns as explained above.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D64930