| Message ID | 20240103171502.1358371-4-bugaevc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64-gnu port | expand |
* Sergey Bugaev: > diff --git a/elf/dl-support.c b/elf/dl-support.c > index 2f502c8b..6375dc95 100644 > --- a/elf/dl-support.c > +++ b/elf/dl-support.c > @@ -135,7 +135,11 @@ void *_dl_random; > #include <dl-procruntime.c> > #include <dl-procinfo.c> > > -size_t _dl_pagesize = EXEC_PAGESIZE; > +size_t _dl_pagesize > +#ifdef EXEC_PAGESIZE > + = EXEC_PAGESIZE > +#endif > +; I think the intent here is to initialize _dl_pagesize with a conservative default, to avoid initialization ordering issues. EXEC_PAGESIZE is supposed to be largest supported page size. Thanks, Florian
* Florian Weimer: > * Sergey Bugaev: > >> diff --git a/elf/dl-support.c b/elf/dl-support.c >> index 2f502c8b..6375dc95 100644 >> --- a/elf/dl-support.c >> +++ b/elf/dl-support.c >> @@ -135,7 +135,11 @@ void *_dl_random; >> #include <dl-procruntime.c> >> #include <dl-procinfo.c> >> >> -size_t _dl_pagesize = EXEC_PAGESIZE; >> +size_t _dl_pagesize >> +#ifdef EXEC_PAGESIZE >> + = EXEC_PAGESIZE >> +#endif >> +; > > I think the intent here is to initialize _dl_pagesize with a > conservative default, to avoid initialization ordering issues. > EXEC_PAGESIZE is supposed to be largest supported page size. This was committed without addressing the comment above. Thanks, Florian
Hello, On Mon, Mar 25, 2024 at 2:58 PM Florian Weimer <fweimer@redhat.com> wrote: > > I think the intent here is to initialize _dl_pagesize with a > > conservative default, to avoid initialization ordering issues. > > EXEC_PAGESIZE is supposed to be largest supported page size. > > This was committed without addressing the comment above. yes, I also didn't expect this to get pushed until we come to an agreement here. On topic: I understand that this must have been done this way because of potential initialization order issues (and the mail I linked also makes this guess). The question is, is that (working around initialization order issues) actually required, or is that a leftover from something that's no longer relevant (or perhaps never was)? Things seem to still work with this patch on aarch64-gnu for me, both in SHARED and !SHARED, though I obviously didn't test every potential codepath. I was hoping that some broader testing, such as running the testsuite on CI on existing ports, could answer that question comprehensively -- though now I realize that since this patch leaves the initialization as-is when EXEC_PAGESIZE is defined (i.e. everywhere but on aarch64-gnu), CI wouldn't catch any issues that removing it would cause. We could define EXEC_PAGESIZE to some conservative value on aarch64-gnu too, if it turns out that this little workaround is really required. But it seems cleaner to make sure we don't need to, as Roland's email suggests, and introducing a new port that doesn't have a fixed page size (and doesn't come with an EXEC_PAGESIZE value already defined in kernel headers) seems to be a good opportunity to do that. That's my reasoning here. Sergey
* Sergey Bugaev: > We could define EXEC_PAGESIZE to some conservative value on > aarch64-gnu too, if it turns out that this little workaround is really > required. But it seems cleaner to make sure we don't need to, as > Roland's email suggests, and introducing a new port that doesn't have > a fixed page size (and doesn't come with an EXEC_PAGESIZE value > already defined in kernel headers) seems to be a good opportunity to > do that. That's my reasoning here. But the ELF image must be laid out with certain expectations regarding the maximum support page size. Otherwise, something (kernel or dynamic linker) needs to perform copies or upgrade conflicting permissions within one page to a superset of all permissions. I don't think we have code for that today, and we wouldn't necessarily want to implement that, I think. Thanks, Florian
Hello, On Wed, Apr 10, 2024 at 2:57 PM Florian Weimer <fweimer@redhat.com> wrote: > * Sergey Bugaev: > > > We could define EXEC_PAGESIZE to some conservative value on > > aarch64-gnu too, if it turns out that this little workaround is really > > required. But it seems cleaner to make sure we don't need to, as > > Roland's email suggests, and introducing a new port that doesn't have > > a fixed page size (and doesn't come with an EXEC_PAGESIZE value > > already defined in kernel headers) seems to be a good opportunity to > > do that. That's my reasoning here. > > But the ELF image must be laid out with certain expectations regarding > the maximum support page size. Otherwise, something (kernel or dynamic > linker) needs to perform copies or upgrade conflicting permissions > within one page to a superset of all permissions. I don't think we have > code for that today, and we wouldn't necessarily want to implement that, > I think. Certainly -- and I wouldn't expect the kernel or the dynamic linker to do anything about it other than reporting an error. I think what you're saying is you consider EXEC_PAGESIZE to indeed describe/define this required alignment of ELF segments (as the name suggests). If I adopt that view, then yes, having EXEC_PAGESIZE makes sense, and it makes some sense to use it as a conservative page size value to use while the real value is not yet available (assuming there is a real need for that). The way I've been viewing it -- based on the fact that neither Linux's nor glibc's (dynamic) nor BFD's nor LLVM's (static) linkers use it for that purpose -- is that it's just some PAGE_SIZE-like definition that's unrelated to binary loading (despite its name -- perhaps it has been historically related to segment alignment in some old versions of Linux?) that has been co-opted by glibc for pre-initializing dl_pagesize, for dubious reasons. It also seems to be a Linux- (and x86 Hurd) specific thing; I cannot find it in the BSDs. Which one is it? Sergey
Hello, Sergey Bugaev, le lun. 25 mars 2024 15:24:14 +0300, a ecrit: > On Mon, Mar 25, 2024 at 2:58 PM Florian Weimer <fweimer@redhat.com> wrote: > > > I think the intent here is to initialize _dl_pagesize with a > > > conservative default, to avoid initialization ordering issues. > > > EXEC_PAGESIZE is supposed to be largest supported page size. > > > > This was committed without addressing the comment above. > > yes, I also didn't expect this to get pushed until we come to an agreement here. Sorry for this, I have reverted the change for now. Samuel
diff --git a/elf/dl-support.c b/elf/dl-support.c index 2f502c8b..6375dc95 100644 --- a/elf/dl-support.c +++ b/elf/dl-support.c @@ -135,7 +135,11 @@ void *_dl_random; #include <dl-procruntime.c> #include <dl-procinfo.c> -size_t _dl_pagesize = EXEC_PAGESIZE; +size_t _dl_pagesize +#ifdef EXEC_PAGESIZE + = EXEC_PAGESIZE +#endif +; size_t _dl_minsigstacksize = CONSTANT_MINSIGSTKSZ; diff --git a/elf/rtld.c b/elf/rtld.c index 4f494b79..98b042c0 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -358,7 +358,9 @@ struct rtld_global_ro _rtld_global_ro attribute_relro = ._dl_debug_fd = STDERR_FILENO, ._dl_lazy = 1, ._dl_fpu_control = _FPU_DEFAULT, +#ifdef EXEC_PAGESIZE ._dl_pagesize = EXEC_PAGESIZE, +#endif ._dl_inhibit_cache = 0, ._dl_profile_output = "/var/tmp", diff --git a/libio/libioP.h b/libio/libioP.h index 1af287b1..1a7f547e 100644 --- a/libio/libioP.h +++ b/libio/libioP.h @@ -852,7 +852,7 @@ extern off64_t _IO_seekpos_unlocked (FILE *, off64_t, int) # define MAP_ANONYMOUS MAP_ANON # endif -# if !defined(MAP_ANONYMOUS) || !defined(EXEC_PAGESIZE) +# if !defined(MAP_ANONYMOUS) # undef _G_HAVE_MMAP # define _G_HAVE_MMAP 0 # endif
We would like to avoid statically defining any specific page size on aarch64-gnu, and instead make sure that everything uses the dynamic page size, available via vm_page_size and GLRO(dl_pagesize). There are currently a few places in glibc that require EXEC_PAGESIZE to be defined. Per Roland's suggestion [0], drop the static GLRO(dl_pagesize) initializers (for now, only if EXEC_PAGESIZE is not defined), and don't require EXEC_PAGESIZE definition for libio to enable mmap usage. [0]: https://mail.gnu.org/archive/html/bug-hurd/2011-10/msg00035.html Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> --- I opted to play it safe and only disable the static GLRO(dl_pagesize) initializers when EXEC_PAGESIZE is not defined (i.e. on aarch64-gnu). I can change this to be unconditional if that's considered OK. elf/dl-support.c | 6 +++++- elf/rtld.c | 2 ++ libio/libioP.h | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-)