bug#47125: 28.0.50; pdumper assumes compile time page size remains valid

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#47125: 28.0.50; pdumper assumes compile time page size remains valid

Pip Cet
I'm running Debian GNU/Linux (the Linux part is not provided by
Debian) on an Apple M1-based machine. This currently involves running
a kernel compiled with a 16 KB page size (the only fully functional
kernel is currently available as a binary as recompilation of the
alleged source fails to produce a fully working kernel).

The Debian-packaged Emacs version does not start. Compiling from
scratch works fine.

After some investigation, this is because pdumper assumes that an
address aligned according to the page size at build time is
sufficiently aligned for mmap to work with the MAP_FIXED flag, when it
comes to loading the dump. That's not true because the Debian Emacs
was apparently built with a 4 KB page size, so it will not run on a
system with a 16 KB page size.

I've confirmed that I get the same error on current master if I modify
getpagesize to return 4096 rather than the correct value.

I think it would be best to handle this case gracefully, and I thought
pdumper already did that, but it appears to simply fail.

There are good reasons for increasing the page size, so this is likely
to happen more often and on other architectures with varying page
sizes. We're currently enforcing a page size of 64 KB on Windows, so
maybe it already has.



Reply | Threaded
Open this post in threaded view
|

bug#47125: 28.0.50; pdumper assumes compile time page size remains valid

Eli Zaretskii
> From: Pip Cet <[hidden email]>
> Date: Sat, 13 Mar 2021 21:38:16 +0000
>
> I'm running Debian GNU/Linux (the Linux part is not provided by
> Debian) on an Apple M1-based machine. This currently involves running
> a kernel compiled with a 16 KB page size (the only fully functional
> kernel is currently available as a binary as recompilation of the
> alleged source fails to produce a fully working kernel).
>
> The Debian-packaged Emacs version does not start. Compiling from
> scratch works fine.
>
> After some investigation, this is because pdumper assumes that an
> address aligned according to the page size at build time is
> sufficiently aligned for mmap to work with the MAP_FIXED flag, when it
> comes to loading the dump. That's not true because the Debian Emacs
> was apparently built with a 4 KB page size, so it will not run on a
> system with a 16 KB page size.
>
> I've confirmed that I get the same error on current master if I modify
> getpagesize to return 4096 rather than the correct value.
>
> I think it would be best to handle this case gracefully, and I thought
> pdumper already did that, but it appears to simply fail.
>
> There are good reasons for increasing the page size, so this is likely
> to happen more often and on other architectures with varying page
> sizes.

CC'ing Daniel, in case he has comments and/or suggestions.

> We're currently enforcing a page size of 64 KB on Windows

We do? can you point me to the code which does that?

> so maybe it already has.



Reply | Threaded
Open this post in threaded view
|

bug#47125: 28.0.50; pdumper assumes compile time page size remains valid

Daniel Colascione-5

On 3/13/21 11:34 PM, Andreas Schwab wrote:
> On Mär 13 2021, Daniel Colascione wrote:
>
>> Maybe we should just use a minimum of 16kB here?
> 64kB would be better.

Sure.




Reply | Threaded
Open this post in threaded view
|

bug#47125: 28.0.50; pdumper assumes compile time page size remains valid

Pip Cet
In reply to this post by Eli Zaretskii
On Sun, Mar 14, 2021 at 5:43 AM Daniel Colascione <[hidden email]> wrote:

> On 3/13/21 9:37 PM, Eli Zaretskii wrote:
>
> >> From: Pip Cet <[hidden email]>
> >> Date: Sat, 13 Mar 2021 21:38:16 +0000
> >>
> >> I'm running Debian GNU/Linux (the Linux part is not provided by
> >> Debian) on an Apple M1-based machine. This currently involves running
> >> a kernel compiled with a 16 KB page size (the only fully functional
> >> kernel is currently available as a binary as recompilation of the
> >> alleged source fails to produce a fully working kernel).
> >>
> >> The Debian-packaged Emacs version does not start. Compiling from
> >> scratch works fine.
> >>
> >> After some investigation, this is because pdumper assumes that an
> >> address aligned according to the page size at build time is
> >> sufficiently aligned for mmap to work with the MAP_FIXED flag, when it
> >> comes to loading the dump. That's not true because the Debian Emacs
> >> was apparently built with a 4 KB page size, so it will not run on a
> >> system with a 16 KB page size.
> >>
> >> I've confirmed that I get the same error on current master if I modify
> >> getpagesize to return 4096 rather than the correct value.
> >>
> >> I think it would be best to handle this case gracefully, and I thought
> >> pdumper already did that, but it appears to simply fail.
> >>
> >> There are good reasons for increasing the page size, so this is likely
> >> to happen more often and on other architectures with varying page
> >> sizes.
> > CC'ing Daniel, in case he has comments and/or suggestions.
>
> We should modify this function to do what the doc comment says then. :-)
> I'm not sure if there's any reliable way to know what the worst case
> allocation granularity actually is: a quick grep through /usr/include
> didn't turn up anything. Maybe we should just use a minimum of 16kB
> here? It's not as if we'd be wasting a ton of RAM doing so.

Linux also offers 64KB pages, so I believe Andreas is correct, that
would be better.

Should we verify that getpagesize isn't problematic when loading the dump?

Thanks
Pip



Reply | Threaded
Open this post in threaded view
|

bug#47125: 28.0.50; pdumper assumes compile time page size remains valid

Eli Zaretskii
> From: Lars Ingebrigtsen <[hidden email]>
> Cc: Daniel Colascione <[hidden email]>,  Eli Zaretskii <[hidden email]>,
>   [hidden email]
> Date: Sun, 28 Mar 2021 17:46:09 +0200
>
> This was two weeks ago, and there was no followup here.  Everybody
> seemed to agree that we should use 64K here...  So is the following
> patch correct?

Yes, I think so.