> On 4. Apr 2023, at 16:03, Ard Biesheuvel <a...@kernel.org> wrote: > > On Tue, 4 Apr 2023 at 13:10, Rebecca Cran <rebe...@bsdio.com > <mailto:rebe...@bsdio.com>> wrote: >> >> On 4/4/23 1:22 AM, Ard Biesheuvel wrote: >>> On Mon, 3 Apr 2023 at 22:33, Rebecca Cran <rebe...@bsdio.com >>> <mailto:rebe...@bsdio.com>> wrote: >>>> As part of my work on the toolchain definitions, I've come across a >>>> situation where ld.lld fails to align sections correctly, due to it >>>> being invoked via clang with the '-n' option, which causes GenFw to fail >>>> with "Section address not aligned to its own alignment.". >>>> >>> Stupid question: if it breaks stuff, why do you use -n ? >> >> As far as I can see, clang adds it automatically when it invokes ld.lld. >> > > Ah right, fair enough. > >>>> The following messages are printed: >>>> >>>> >>>> ld.lld: warning: -z common-page-size set, but paging disabled by omagic >>>> or nmagic ld.lld: warning: address (0x558) of section .data is not a >>>> multiple of alignment (16) >>>> >>>> I tracked the problem down to GccBase.lds and ClangBase.lds, which have: >>>> >>>> /* * The alignment of the .data section should be less than or equal to >>>> the * alignment of the .text section. This ensures that the relative >>>> offset * between these sections is the same in the ELF and the PE/COFF >>>> versions of * this binary. */ .data ALIGN(ALIGNOF(.text)) : >>>> ALIGN(CONSTANT(COMMONPAGESIZE)) { *(.data .data.* .gnu.linkonce.d.*) >>>> *(.bss .bss.*) } >>>> >>>> I can work around the problem by removing "ALIGN(ALIGNOF(.text)", but >>>> I'm wondering if our use of COMMONPAGESIZE/MAXPAGESIZE is correct. >>>> >>> What do you mean by 'correct'? The intent is clearly to declare the >>> mapping granule size, and for SEC/PEI binaries that execute in place >>> from flash, the MMU page size is not the most useful quantum here. >> >> On Discord, I think Marvin was saying that we shouldn't be using >> COMMONPAGESIZE, but MAXPAGESIZE. >> > > I don't have a preference for one over the other, although I fail to > see how this fixes the -n problem above. > >> Since .text is aligned to COMMONPAGESIZE, I think I can remove the >> ALIGN(ALIGNOF(.text)) and keep the ALIGN(CONSTANT(COMMONPAGESIZE))? >> > > No. The alignment of .text could exceed COMMONPAGESIZE; this happens > e.g., with AArch64 vector tables which are 2k aligned, when they are > emitted into a SEC or PEIM module which only has 32 byte alignment by > default. > > In this case, the PE/COFF section alignment will be 2k, and so .data > should start at a 2k aligned address as well. > >> >>> 'Page size' is highly context specific, and toolchain people are >>> (imho) usually quite quick to call something abuse if it does not >>> match their narrow definition of how a compiler or linker should be >>> used. For the same reason, it has been so difficult to get a compiler >>> to understand that the desire for position independent code does not >>> imply that we want GOTs, or care about ELF symbol preemption or >>> copy-on-write footprint of relocatable segments. In general, the bare >>> metal use case (which includes EFI) is quite poorly understood by many >>> people working on toolchains. >>> >>>> I'm wondering what the correct approach is here: should we do something >>>> similar to how we set PECOFF_HEADER_SIZE and define a SECTION_ALIGNMENT >>>> symbol? >>> We cannot, that is the reason for using the page size switches here: >>> using a symbol to set the location pointer is fine, but using a symbol >>> to set the alignment of a section is not. >>> >>>> Or, as discussed on Discord should we just use >>>> CONSTANT(MAXPAGESIZE) and ignore how it's normally used to specify the >>>> maximum allowable page size for an executable? >>>> >>> Note that (when I last checked), the only effect of setting -z >>> xxx-page-size is that those macros assume the associated value in the >>> linker script. Nothing else in the linker changes behavior (with the >>> exception of the warning you are seeing) >>> >>> So claiming abuse because the provided value does not match the page >>> size of an OS that might also run on the same system is strenuous, and >>> I think our use of it is fine. AIUI, the reason for having >>> ClangBase.lds in addition to GccBase.lds is the fact that LLD does not >>> support common but only max page size, so I think it should be fine to >>> merge the two, and use max-page-size everywhere. >> >> Thanks for the explanation. This is probably an error on my part, but >> when I tried to use max-page-size everywhere, the AARCH64 build of >> ArmVirtQemu failed, with GenFw saying "AARCH64 small code model requires >> identical ELF and PE/COFF section offsets modulo 4 KB." >> > > Let me know if you manage to reproduce that - I'd like to understand > what exactly is happening there. > >> LLD added support for common-page-size in 2019 >> (https://reviews.llvm.org/D56205), so I think we can keep the current >> usage of common and max page size and just combine GccBase.lds and >> ClangBase.lds. >> > > Yes, that would be preferable.
It seems your mail server ate my mail as spam: https://edk2.groups.io/g/devel/message/102464 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#102486): https://edk2.groups.io/g/devel/message/102486 Mute This Topic: https://groups.io/mt/98045342/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
