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

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.

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> 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.

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

[Daniel Colascione]

On 3/13/21 9:37 PM, Eli Zaretskii wrote:

>> From: Pip Cet <pipcet@gmail.com>
>> 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.

/* Worst-case allocation granularity on any system that might load
    this dump.  */
static int
dump_get_page_size (void)
{
#if defined (WINDOWSNT) || defined (CYGWIN)
   return 64 * 1024;  /* Worst-case allocation granularity.  */
#else
   return getpagesize ();
#endif
}

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

[Andreas Schwab]

On Mär 13 2021, Daniel Colascione wrote:

> Maybe we should just use a minimum of 16kB here?

64kB would be better.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[Daniel Colascione]

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.

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

[Pip Cet]

On Sun, Mar 14, 2021 at 5:43 AM Daniel Colascione <dancol@dancol.org> wrote:
> On 3/13/21 9:37 PM, Eli Zaretskii wrote:
>
> >> From: Pip Cet <pipcet@gmail.com>
> >> 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

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

[Lars Ingebrigtsen]

Pip Cet <pipcet@gmail.com> writes:

> 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?

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?

diff --git a/src/pdumper.c b/src/pdumper.c
index 337742fda4..bdaba0269f 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -162,11 +162,7 @@ ptrdiff_t_to_dump_off (ptrdiff_t value)
 static int
 dump_get_page_size (void)
 {
-#if defined (WINDOWSNT) || defined (CYGWIN)
   return 64 * 1024;  /* Worst-case allocation granularity.  */
-#else
-  return getpagesize ();
-#endif
 }
 
 #define dump_offsetof(type, member)                             \


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Daniel Colascione <dancol@dancol.org>,  Eli Zaretskii <eliz@gnu.org>,
>   47125@debbugs.gnu.org
> 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.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> 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.

OK; pushed to Emacs 28 now, then.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no