Re: MPS: Forwording symbols

[Helmut Eller <eller.helmut@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 564 bytes --]

On Tue, Jun 18 2024, Eli Zaretskii wrote:

>> Could this be our old friend dump_discard_mem that is too lazy to check
>> error codes?  What happens if you remove the call to
>> dump_mmap_discard_contents in pdumper_load?
>
> Then the build succeeds.

With the patch below, dump_mmap_discard_contents calls the map->release
callback.  I think that was the original reason for having this
callback.  It also adds stricter error checking to dump_discard_mem, but
that function is now unused.  It's not clear to me, when it should be
used instead of dump_unmap_file.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-More-strict-error-checking-in-pdump_mmap_discard.patch --]
[-- Type: text/x-diff, Size: 2317 bytes --]

From 018e0b1f6b0f3c2e53d7a99b3f29e968470e3ef1 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 18 Jun 2024 21:15:32 +0200
Subject: [PATCH] More strict error checking in pdump_mmap_discard

* src/dumper.c (dump_discard_mem): Abort on error.
(dump_mmap_discard_contents): Call dump_mmap_release instead
of dump_discard_mem, which invokes the map specific callback.
---
 src/pdumper.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index 9147be38294..9800652da5f 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -4951,24 +4951,22 @@ dump_discard_mem (void *mem, size_t size)
       DWORD old_prot;
       (void) VirtualProtect (mem, size, PAGE_NOACCESS, &old_prot);
 #elif VM_SUPPORTED == VM_POSIX
+      int err = 0;
 # ifdef HAVE_POSIX_MADVISE
       /* Discard COWed pages.  */
-      (void) posix_madvise (mem, size, POSIX_MADV_DONTNEED);
+      err = posix_madvise (mem, size, POSIX_MADV_DONTNEED);
 # elif defined HAVE_MADVISE
-      (void) madvise (mem, size, MADV_DONTNEED);
+      err = madvise (mem, size, MADV_DONTNEED);
 #endif
+      if (err)
+	emacs_abort ();
       /* Release the commit charge for the mapping.  */
-      (void) mprotect (mem, size, PROT_NONE);
+      err = mprotect (mem, size, PROT_NONE);
+      if (err)
+	emacs_abort ();
 #endif
 }
 
-static void
-dump_mmap_discard_contents (struct dump_memory_map *map)
-{
-  if (map->mapping)
-    dump_discard_mem (map->mapping, map->spec.size);
-}
-
 static void
 dump_mmap_reset (struct dump_memory_map *map)
 {
@@ -4985,6 +4983,13 @@ dump_mmap_release (struct dump_memory_map *map)
   dump_mmap_reset (map);
 }
 
+static void
+dump_mmap_discard_contents (struct dump_memory_map *map)
+{
+  if (map->mapping)
+    dump_mmap_release (map);
+}
+
 /* Allows heap-allocated dump_mmap to "free" maps individually.  */
 struct dump_memory_map_heap_control_block
 {
@@ -5072,7 +5077,9 @@ dump_mmap_contiguous_heap (struct dump_memory_map *maps, int nr_maps,
 static void
 dump_mmap_release_mps (struct dump_memory_map *map)
 {
-  emacs_abort ();
+  /* FIXME: igc_on_pdump_loaded "knows" that DS_DISCARDABLE is unused.
+     Maybe come up with a nicer API.
+   */
 }
 
 /* Implement dump_mmap using mps_reserve and read.  */
-- 
2.39.2