emacs-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: master a99ba59aa02: ; * src/pdumper.c (dump_do_fixup): Pacify GCC.


From: Pip Cet
Subject: Re: master a99ba59aa02: ; * src/pdumper.c (dump_do_fixup): Pacify GCC.
Date: Thu, 30 Jan 2025 11:20:31 +0000

Pip Cet <pipcet@protonmail.com> writes:

> I'll prepare and post (not push!) a patch to see what that would look
> like.  But, Stefan (and others), feel perfectly free to ignore the patch
> and discuss the issue in English first, if that's better :-)

This patch series (one revert per commit) restores the previous status
of pdumper.c:

>From e26b852c854af0492d5561e6c7a5125759dd2874 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@protonmail.com>
Subject: [PATCH 1/3] Revert "; * src/pdumper.c (dump_do_fixup): Pacify GCC."

This reverts commit a99ba59aa02ef8cfd314737950b6cd8d97015925.
---
 src/pdumper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index bfa790b963a..9f0447eb5aa 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -3990,7 +3990,7 @@ dump_do_fixup (struct dump_context *ctx,
   Lisp_Object arg = dump_pop (&fixup);
   eassert (NILP (fixup));
   dump_seek (ctx, dump_fixup_offset);
-  intptr_t dump_value UNINIT;
+  intptr_t dump_value;
   bool do_write = true;
   switch (type)
     {
-- 
2.47.1


>From f82854e53e79d16c6bd28dffe55fb46ce87ef0e3 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@protonmail.com>
Subject: [PATCH 2/3] Revert "Pacify -Wanalyzer-use-of-uninitialized-value"

This reverts commit 1ed769a3cb753a86badba8a2878fa788a6fdc1f8.
---
 src/pdumper.c | 91 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 62 insertions(+), 29 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index 9f0447eb5aa..45a44db0243 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2292,12 +2292,11 @@ dump_float (struct dump_context *ctx, const struct 
Lisp_Float *lfloat)
 }
 
 static dump_off
-dump_fwd_int (struct dump_context *ctx, void const *fwdptr)
+dump_fwd_int (struct dump_context *ctx, const struct Lisp_Intfwd *intfwd)
 {
 #if CHECK_STRUCTS && !defined HASH_Lisp_Intfwd_4D887A7387
 # error "Lisp_Intfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
-  struct Lisp_Intfwd const *intfwd = fwdptr;
   dump_emacs_reloc_immediate_intmax_t (ctx, intfwd->intvar, *intfwd->intvar);
   struct Lisp_Intfwd out;
   dump_object_start (ctx, &out, sizeof (out));
@@ -2307,12 +2306,11 @@ dump_fwd_int (struct dump_context *ctx, void const 
*fwdptr)
 }
 
 static dump_off
-dump_fwd_bool (struct dump_context *ctx, void const *fwdptr)
+dump_fwd_bool (struct dump_context *ctx, const struct Lisp_Boolfwd *boolfwd)
 {
 #if CHECK_STRUCTS && !defined (HASH_Lisp_Boolfwd_0EA1C7ADCC)
 # error "Lisp_Boolfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
-  struct Lisp_Boolfwd const *boolfwd = fwdptr;
   dump_emacs_reloc_immediate_bool (ctx, boolfwd->boolvar, *boolfwd->boolvar);
   struct Lisp_Boolfwd out;
   dump_object_start (ctx, &out, sizeof (out));
@@ -2322,12 +2320,11 @@ dump_fwd_bool (struct dump_context *ctx, void const 
*fwdptr)
 }
 
 static dump_off
-dump_fwd_obj (struct dump_context *ctx, void const *fwdptr)
+dump_fwd_obj (struct dump_context *ctx, const struct Lisp_Objfwd *objfwd)
 {
 #if CHECK_STRUCTS && !defined (HASH_Lisp_Objfwd_45D3E513DC)
 # error "Lisp_Objfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
-  struct Lisp_Objfwd const *objfwd = fwdptr;
   if (NILP (Fgethash (dump_off_to_lisp (emacs_offset (objfwd->objvar)),
                       ctx->staticpro_table,
                       Qnil)))
@@ -2340,12 +2337,12 @@ dump_fwd_obj (struct dump_context *ctx, void const 
*fwdptr)
 }
 
 static dump_off
-dump_fwd_buffer_obj (struct dump_context *ctx, void const *fwdptr)
+dump_fwd_buffer_obj (struct dump_context *ctx,
+                     const struct Lisp_Buffer_Objfwd *buffer_objfwd)
 {
 #if CHECK_STRUCTS && !defined (HASH_Lisp_Buffer_Objfwd_611EBD13FF)
 # error "Lisp_Buffer_Objfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
-  struct Lisp_Buffer_Objfwd const *buffer_objfwd = fwdptr;
   struct Lisp_Buffer_Objfwd out;
   dump_object_start (ctx, &out, sizeof (out));
   DUMP_FIELD_COPY (&out, buffer_objfwd, type);
@@ -2356,12 +2353,12 @@ dump_fwd_buffer_obj (struct dump_context *ctx, void 
const *fwdptr)
 }
 
 static dump_off
-dump_fwd_kboard_obj (struct dump_context *ctx, void const *fwdptr)
+dump_fwd_kboard_obj (struct dump_context *ctx,
+                     const struct Lisp_Kboard_Objfwd *kboard_objfwd)
 {
 #if CHECK_STRUCTS && !defined (HASH_Lisp_Kboard_Objfwd_CAA7E71069)
 # error "Lisp_Intfwd changed. See CHECK_STRUCTS comment in config.h."
 #endif
-  struct Lisp_Kboard_Objfwd const *kboard_objfwd = fwdptr;
   struct Lisp_Kboard_Objfwd out;
   dump_object_start (ctx, &out, sizeof (out));
   DUMP_FIELD_COPY (&out, kboard_objfwd, type);
@@ -2375,16 +2372,29 @@ dump_fwd (struct dump_context *ctx, lispfwd fwd)
 #if CHECK_STRUCTS && !defined (HASH_Lisp_Fwd_Type_9CBA6EE55E)
 # error "Lisp_Fwd_Type changed. See CHECK_STRUCTS comment in config.h."
 #endif
-  typedef dump_off (*dump_fwd_fnptr) (struct dump_context *, void const *);
-  static dump_fwd_fnptr const dump_fwd_table[] = {
-    [Lisp_Fwd_Int] = dump_fwd_int,
-    [Lisp_Fwd_Bool] = dump_fwd_bool,
-    [Lisp_Fwd_Obj] = dump_fwd_obj,
-    [Lisp_Fwd_Buffer_Obj] = dump_fwd_buffer_obj,
-    [Lisp_Fwd_Kboard_Obj] = dump_fwd_kboard_obj,
-  };
+  void const *p = fwd.fwdptr;
+  dump_off offset;
 
-  return dump_fwd_table[XFWDTYPE (fwd)] (ctx, fwd.fwdptr);
+  switch (XFWDTYPE (fwd))
+    {
+    case Lisp_Fwd_Int:
+      offset = dump_fwd_int (ctx, p);
+      break;
+    case Lisp_Fwd_Bool:
+      offset = dump_fwd_bool (ctx, p);
+      break;
+    case Lisp_Fwd_Obj:
+      offset = dump_fwd_obj (ctx, p);
+      break;
+    case Lisp_Fwd_Buffer_Obj:
+      offset = dump_fwd_buffer_obj (ctx, p);
+      break;
+    case Lisp_Fwd_Kboard_Obj:
+      offset = dump_fwd_kboard_obj (ctx, p);
+      break;
+    }
+
+  return offset;
 }
 
 static dump_off
@@ -4534,19 +4544,26 @@ dump_anonymous_allocate_w32 (void *base,
 #  define MAP_ANONYMOUS MAP_ANON
 # endif
 
-static int const mem_prot_posix_table[] = {
-  [DUMP_MEMORY_ACCESS_NONE] = PROT_NONE,
-  [DUMP_MEMORY_ACCESS_READ] = PROT_READ,
-  [DUMP_MEMORY_ACCESS_READWRITE] = PROT_READ | PROT_WRITE,
-};
-
 static void *
 dump_anonymous_allocate_posix (void *base,
                                size_t size,
                                enum dump_memory_protection protection)
 {
   void *ret;
-  int mem_prot = mem_prot_posix_table[protection];
+  int mem_prot;
+
+  switch (protection)
+    {
+    case DUMP_MEMORY_ACCESS_NONE:
+      mem_prot = PROT_NONE;
+      break;
+    case DUMP_MEMORY_ACCESS_READ:
+      mem_prot = PROT_READ;
+      break;
+    case DUMP_MEMORY_ACCESS_READWRITE:
+      mem_prot = PROT_READ | PROT_WRITE;
+      break;
+    }
 
   int mem_flags = MAP_PRIVATE | MAP_ANONYMOUS;
   if (mem_prot != PROT_NONE)
@@ -4690,9 +4707,25 @@ dump_map_file_posix (void *base, int fd, off_t offset, 
size_t size,
                     enum dump_memory_protection protection)
 {
   void *ret;
-  int mem_prot = mem_prot_posix_table[protection];
-  int mem_flags = (protection == DUMP_MEMORY_ACCESS_READWRITE
-                  ? MAP_PRIVATE : MAP_SHARED);
+  int mem_prot;
+  int mem_flags;
+
+  switch (protection)
+    {
+    case DUMP_MEMORY_ACCESS_NONE:
+      mem_prot = PROT_NONE;
+      mem_flags = MAP_SHARED;
+      break;
+    case DUMP_MEMORY_ACCESS_READ:
+      mem_prot = PROT_READ;
+      mem_flags = MAP_SHARED;
+      break;
+    case DUMP_MEMORY_ACCESS_READWRITE:
+      mem_prot = PROT_READ | PROT_WRITE;
+      mem_flags = MAP_PRIVATE;
+      break;
+    }
+
   if (base)
     mem_flags |= MAP_FIXED;
 
-- 
2.47.1

>From b92027de63b1ef8aed7eb07bf29f99e713830aff Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@protonmail.com>
Subject: [PATCH 3/3] Revert "Prefer static switch-case checking in pdumper"

This reverts commit 6e2e7265a04f63f482db7fbdfd8e2519d8bfe03e.
---
 src/pdumper.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/pdumper.c b/src/pdumper.c
index 45a44db0243..71d82629b56 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -2392,6 +2392,8 @@ dump_fwd (struct dump_context *ctx, lispfwd fwd)
     case Lisp_Fwd_Kboard_Obj:
       offset = dump_fwd_kboard_obj (ctx, p);
       break;
+    default:
+      emacs_abort ();
     }
 
   return offset;
@@ -2523,6 +2525,8 @@ dump_symbol (struct dump_context *ctx,
     case SYMBOL_FORWARDED:
       dump_field_fixup_later (ctx, &out, symbol, &symbol->u.s.val.fwd);
       break;
+    default:
+      emacs_abort ();
     }
   dump_field_lv (ctx, &out, symbol, &symbol->u.s.function, WEIGHT_NORMAL);
   dump_field_lv (ctx, &out, symbol, &symbol->u.s.plist, WEIGHT_NORMAL);
@@ -3603,6 +3607,8 @@ dump_drain_cold_data (struct dump_context *ctx)
          dump_cold_native_subr (ctx, data);
          break;
 #endif
+        default:
+          emacs_abort ();
         }
     }
 
@@ -4067,6 +4073,8 @@ dump_do_fixup (struct dump_context *ctx,
         do_write = false;
         break;
       }
+    default:
+      emacs_abort ();
     }
   if (do_write)
     dump_write (ctx, &dump_value, sizeof (dump_value));
@@ -4525,6 +4533,8 @@ dump_anonymous_allocate_w32 (void *base,
       mem_type = MEM_COMMIT;
       mem_prot = PAGE_READWRITE;
       break;
+    default:
+      emacs_abort ();
     }
 
   ret = VirtualAlloc (base, size, mem_type, mem_prot);
@@ -4563,6 +4573,8 @@ dump_anonymous_allocate_posix (void *base,
     case DUMP_MEMORY_ACCESS_READWRITE:
       mem_prot = PROT_READ | PROT_WRITE;
       break;
+    default:
+      emacs_abort ();
     }
 
   int mem_flags = MAP_PRIVATE | MAP_ANONYMOUS;
@@ -4655,6 +4667,7 @@ dump_map_file_w32 (void *base, int fd, off_t offset, 
size_t size,
     case DUMP_MEMORY_ACCESS_READWRITE:
       protect = PAGE_WRITECOPY;        /* for Windows 9X */
       break;
+    default:
     case DUMP_MEMORY_ACCESS_NONE:
     case DUMP_MEMORY_ACCESS_READ:
       protect = PAGE_READONLY;
@@ -4682,6 +4695,8 @@ dump_map_file_w32 (void *base, int fd, off_t offset, 
size_t size,
     case DUMP_MEMORY_ACCESS_READWRITE:
       map_access = FILE_MAP_COPY;
       break;
+    default:
+      emacs_abort ();
     }
 
   ret = MapViewOfFileEx (section,
@@ -4724,6 +4739,8 @@ dump_map_file_posix (void *base, int fd, off_t offset, 
size_t size,
       mem_prot = PROT_READ | PROT_WRITE;
       mem_flags = MAP_PRIVATE;
       break;
+    default:
+      emacs_abort ();
     }
 
   if (base)
@@ -5590,6 +5607,8 @@ dump_do_emacs_relocation (const uintptr_t dump_base,
         memcpy (emacs_ptr_at (reloc.emacs_offset), &lv, sizeof (lv));
         break;
       }
+    default:
+      fatal ("unrecognied relocation type %d", (int) reloc.type);
     }
 }
 
-- 
2.47.1

It includes the "unrecognied" typo, which should be fixed in a further
commit, along with some similarly minor issues.

It restores type safety and aborts rather than attempting an
out-of-bounds array access when an enum changes under us.

It doesn't revert:

d7bdaa4170cd6115a6e3b4d872d8a428397fb2d8 Handle unknown units provided by the 
rsvg library (bug#75712)

because that code looks correct to me (if it's not, please fix!)

It doesn't touch:

8d471adecef540d49807dad114f7a11fb3fe2a95 Fix subtle problem with updating 
'font_style_table'

which adds a redundant "default:" label, incorrectly implying that there
are additional possible values "prop" can reasonably have at that point.
That commit is being discussed elsewhere.

If there are still compiler warnings after this patch series, please let
me know.  It builds without warnings here, but GCC warnings are fickle
and depend on the precise optimization options used (I recently
attempted building with -Os, and there are warnings that show up only
with that option, not -O0 or -O2).

Pip




reply via email to

[Prev in Thread] Current Thread [Next in Thread]