Re: CHECK_STRUCTS/dmpstruct.h mechanism is broken.

[Paul Eggert <eggert@cs.ucla.edu>]

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

Daniel Colascione wrote:
> What would make the hashes easier to deal with? Some make target for
> updating them automatically? An easier way to disable the check at
> configure time? Transformation into a prominent runtime warning instead of
> a build break?

It's better to have the build break than to have a runtime check. Of the two 
other approaches you mentioned, I think a configure-time option is the better 
idea. Since these hashes are aimed at developers and are are not needed for 
ordinary builds from tarballs, I think the hashes should be checked on request. 
I resurrected the hashes by installing the attached patch and they are now 
checked with the longstanding 'configure' option --enable-checking=all or with 
the new 'configure' option --enable-checking=structs. Comments welcome as usual.

[-- Attachment #2: 0001-Bring-back-dmpstruct.h.patch --]
[-- Type: text/x-patch, Size: 17921 bytes --]

From 9994bf17cf532f2e1d4310341da7180342202191 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 10 Apr 2019 19:42:37 -0700
Subject: [PATCH] Bring back dmpstruct.h

Bring back the dmpstruct.h checking, and use it when
--enable-checking=structs is specified.  The checking can be helpful
to some developers, although it gets in the way of others and is
not needed for ordinary tarball builds.
* src/dmpstruct.awk: Restore this file, with mode 644 not 755.
* configure.ac: New option-arg --enable-checking=structs,
implied by --enable-checking.
(CHECK_STRUCTS): New macro and var.
* src/Makefile.in (CHECK_STRUCTS): New macro.
(dmpstruct_headers, dmpstruct.h, dmpstruct.h):
Restore these macros and rules.
(pdumper.o): Restore this dependency if $(CHECK_STRUCTS) is true.
(mostlyclean): Remove dmpstruct.h.
* src/pdumper.c [CHECK_STRUCTS]: Include dmpstruct.h,
and restore checks against hashes.
---
 .gitignore        |  1 +
 configure.ac      | 17 ++++++++--
 etc/NEWS          |  5 +++
 src/Makefile.in   | 13 +++++++-
 src/dmpstruct.awk | 45 ++++++++++++++++++++++++++
 src/pdumper.c     | 81 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 159 insertions(+), 3 deletions(-)
 create mode 100644 src/dmpstruct.awk

diff --git a/.gitignore b/.gitignore
index 98b8222180..88b29760b7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -186,6 +186,7 @@ src/emacs
 src/emacs-[0-9]*
 src/temacs
 src/fingerprint.c
+src/dmpstruct.h
 src/*.pdmp
 
 # Character-set info.
diff --git a/configure.ac b/configure.ac
index c93cfbbb59..1814a30cbc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -537,19 +537,21 @@ AC_DEFUN
 
 AC_ARG_ENABLE(checking,
 [AS_HELP_STRING([--enable-checking@<:@=LIST@:>@],
-		[enable expensive run-time checks.  With LIST,
+		[enable expensive checks.  With LIST,
 		 enable only specific categories of checks.
 		 Categories are: all,yes,no.
 		 Flags are: stringbytes, stringoverrun, stringfreelist,
-		 xmallocoverrun, conslist, glyphs])],
+		 structs, xmallocoverrun, conslist, glyphs])],
 [ac_checking_flags="${enableval}"],[])
 IFS="${IFS= 	}"; ac_save_IFS="$IFS"; IFS="$IFS,"
+CHECK_STRUCTS=false
 for check in $ac_checking_flags
 do
 	case $check in
 	# these set all the flags to specific states
 	yes)		ac_enable_checking=1 ;;
 	no)		ac_enable_checking= ;
+			CHECK_STRUCTS=false
 			ac_gc_check_stringbytes= ;
 	                ac_gc_check_string_overrun= ;
 	                ac_gc_check_string_free_list= ;
@@ -557,6 +559,7 @@ AC_DEFUN
 	                ac_gc_check_cons_list= ;
 			ac_glyphs_debug= ;;
 	all)		ac_enable_checking=1 ;
+			CHECK_STRUCTS=true
 			ac_gc_check_stringbytes=1 ;
 	                ac_gc_check_string_overrun=1 ;
 	                ac_gc_check_string_free_list=1 ;
@@ -567,6 +570,7 @@ AC_DEFUN
 	stringbytes)	ac_gc_check_stringbytes=1 ;;
 	stringoverrun)	ac_gc_check_string_overrun=1 ;;
 	stringfreelist) ac_gc_check_string_free_list=1 ;;
+	structs)	CHECK_STRUCTS=true ;;
 	xmallocoverrun)	ac_xmalloc_overrun=1 ;;
 	conslist)	ac_gc_check_cons_list=1 ;;
 	glyphs)		ac_glyphs_debug=1 ;;
@@ -579,6 +583,15 @@ AC_DEFUN
   AC_DEFINE(ENABLE_CHECKING, 1,
 [Define to 1 if expensive run-time data type and consistency checks are enabled.])
 fi
+if $CHECK_STRUCTS; then
+  AC_DEFINE([CHECK_STRUCTS], 1,
+    [Define this to check whether someone updated the portable dumper
+     code after changing the layout of a structure that it uses.
+     If you change one of these structures, check that the pdumper.c
+     code is still valid, and update the pertinent hash in pdumper.c
+     by manually copying the hash from the newly-generated dmpstruct.h.])
+fi
+AC_SUBST([CHECK_STRUCTS])
 if test x$ac_gc_check_stringbytes != x ; then
   AC_DEFINE(GC_CHECK_STRING_BYTES, 1,
 [Define this temporarily to hunt a bug.  If defined, the size of
diff --git a/etc/NEWS b/etc/NEWS
index fbde6e0b66..9644c1ca22 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -84,6 +84,11 @@ The new command-line argument '--dump-file=FILE' allows to specify a
 non-default '.pdmp' file to load the state from; see the node "Initial
 Options" in the Emacs manual for more information.
 
++++
+** The new configure option '--enable-checking=structs' attempts to
+check that the portable dumper code has been updated to match the last
+change to one of the data structures that it relies on.
+
 \f
 * Startup Changes in Emacs 27.1
 
diff --git a/src/Makefile.in b/src/Makefile.in
index 6d6308fde6..2348c8dae4 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -331,6 +331,7 @@ BUILD_DETAILS =
 UNEXEC_OBJ = @UNEXEC_OBJ@
 
 DUMPING=@DUMPING@
+CHECK_STRUCTS = @CHECK_STRUCTS@
 
 # 'make' verbosity.
 AM_DEFAULT_VERBOSITY = @AM_DEFAULT_VERBOSITY@
@@ -456,6 +457,16 @@ ALLOBJS =
 all: emacs$(EXEEXT) $(pdmp) $(OTHER_FILES)
 .PHONY: all
 
+dmpstruct_headers=$(srcdir)/lisp.h $(srcdir)/buffer.h \
+	$(srcdir)/intervals.h $(srcdir)/charset.h $(srcdir)/bignum.h
+ifeq ($(CHECK_STRUCTS),true)
+pdumper.o: dmpstruct.h
+endif
+dmpstruct.h: $(srcdir)/dmpstruct.awk
+dmpstruct.h: $(libsrc)/make-fingerprint$(EXEEXT) $(dmpstruct_headers)
+	$(AM_V_GEN)POSIXLY_CORRECT=1 awk -f $(srcdir)/dmpstruct.awk \
+		$(dmpstruct_headers) > $@
+
 AUTO_DEPEND = @AUTO_DEPEND@
 DEPDIR = deps
 ifeq ($(AUTO_DEPEND),yes)
@@ -665,7 +676,7 @@ .PHONY:
 
 mostlyclean:
 	rm -f temacs$(EXEEXT) core ./*.core \#* ./*.o
-	rm -f fingerprint.c
+	rm -f dmpstruct.h fingerprint.c
 	rm -f emacs.pdmp
 	rm -f ../etc/DOC
 	rm -f bootstrap-emacs$(EXEEXT) $(bootstrap_pdmp)
diff --git a/src/dmpstruct.awk b/src/dmpstruct.awk
new file mode 100644
index 0000000000..55626cf8b2
--- /dev/null
+++ b/src/dmpstruct.awk
@@ -0,0 +1,45 @@
+# Copyright (C) 2018-2019 Free Software Foundation, Inc.
+#
+# This file is part of GNU Emacs.
+#
+# GNU Emacs is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or (at
+# your option) any later version.
+#
+# GNU Emacs is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+BEGIN {
+  print "/* Generated by dmpstruct.awk */"
+  print "#ifndef EMACS_DMPSTRUCT_H"
+  print "#define EMACS_DMPSTRUCT_H"
+  struct_name = ""
+  tmpfile = "dmpstruct.tmp"
+}
+# Match a type followed by optional syntactic whitespace
+/^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/ {
+  struct_name = $2
+  close (tmpfile)
+}
+/^(enum|struct|union) [a-zA-Z0-9_]+([\t ]|\/\*.*\*\/)*$/, /^(  )?};$/ {
+  print $0 > tmpfile
+}
+/^(  )?} *(GCALIGNED_STRUCT)? *;$/ {
+  if (struct_name != "") {
+    fflush (tmpfile)
+    cmd = "../lib-src/make-fingerprint -r " tmpfile
+    cmd | getline hash
+    close (cmd)
+    printf "#define HASH_%s_%.10s\n", struct_name, hash
+    struct_name = ""
+  }
+}
+END {
+  print "#endif /* EMACS_DMPSTRUCT_H */"
+}
diff --git a/src/pdumper.c b/src/pdumper.c
index 3aa941221d..600c5b3ca3 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -46,6 +46,10 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "thread.h"
 #include "bignum.h"
 
+#ifdef CHECK_STRUCTS
+# include "dmpstruct.h"
+#endif
+
 /*
   TODO:
 
@@ -2029,6 +2033,9 @@ dump_pseudovector_lisp_fields (struct dump_context *ctx,
 static dump_off
 dump_cons (struct dump_context *ctx, const struct Lisp_Cons *cons)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Cons_00EEE63F67)
+# error "Lisp_Cons changed. See CHECK_STRUCTS comment."
+#endif
   struct Lisp_Cons out;
   dump_object_start (ctx, &out, sizeof (out));
   dump_field_lv (ctx, &out, cons, &cons->u.s.car, WEIGHT_STRONG);
@@ -2041,6 +2048,9 @@ dump_interval_tree (struct dump_context *ctx,
                     INTERVAL tree,
                     dump_off parent_offset)
 {
+#if CHECK_STRUCTS && !defined (HASH_interval_1B38941C37)
+# error "interval changed. See CHECK_STRUCTS comment."
+#endif
   /* TODO: output tree breadth-first?  */
   struct interval out;
   dump_object_start (ctx, &out, sizeof (out));
@@ -2082,6 +2092,9 @@ dump_interval_tree (struct dump_context *ctx,
 static dump_off
 dump_string (struct dump_context *ctx, const struct Lisp_String *string)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_String_86FEA6EC7C)
+# error "Lisp_String changed. See CHECK_STRUCTS comment."
+#endif
   /* If we have text properties, write them _after_ the string so that
      at runtime, the prefetcher and cache will DTRT. (We access the
      string before its properties.).
@@ -2125,6 +2138,10 @@ dump_string (struct dump_context *ctx, const struct Lisp_String *string)
 static dump_off
 dump_marker (struct dump_context *ctx, const struct Lisp_Marker *marker)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Marker_642DBAF866)
+# error "Lisp_Marker changed. See CHECK_STRUCTS comment."
+#endif
+
   START_DUMP_PVEC (ctx, &marker->header, struct Lisp_Marker, out);
   dump_pseudovector_lisp_fields (ctx, &out->header, &marker->header);
   DUMP_FIELD_COPY (out, marker, need_adjustment);
@@ -2144,6 +2161,9 @@ dump_marker (struct dump_context *ctx, const struct Lisp_Marker *marker)
 static dump_off
 dump_overlay (struct dump_context *ctx, const struct Lisp_Overlay *overlay)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Overlay_72EADA9882)
+# error "Lisp_Overlay changed. See CHECK_STRUCTS comment."
+#endif
   START_DUMP_PVEC (ctx, &overlay->header, struct Lisp_Overlay, out);
   dump_pseudovector_lisp_fields (ctx, &out->header, &overlay->header);
   dump_field_lv_rawptr (ctx, out, overlay, &overlay->next,
@@ -2169,6 +2189,9 @@ static dump_off
 dump_finalizer (struct dump_context *ctx,
                 const struct Lisp_Finalizer *finalizer)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Finalizer_D58E647CB8)
+# error "Lisp_Finalizer changed. See CHECK_STRUCTS comment."
+#endif
   START_DUMP_PVEC (ctx, &finalizer->header, struct Lisp_Finalizer, out);
   /* Do _not_ call dump_pseudovector_lisp_fields here: we dump the
      only Lisp field, finalizer->function, manually, so we can give it
@@ -2188,6 +2211,9 @@ struct bignum_reload_info
 static dump_off
 dump_bignum (struct dump_context *ctx, Lisp_Object object)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Bignum_661945DE2B)
+# error "Lisp_Bignum changed. See CHECK_STRUCTS comment."
+#endif
   const struct Lisp_Bignum *bignum = XBIGNUM (object);
   START_DUMP_PVEC (ctx, &bignum->header, struct Lisp_Bignum, out);
   verify (sizeof (out->value) >= sizeof (struct bignum_reload_info));
@@ -2223,6 +2249,9 @@ dump_bignum (struct dump_context *ctx, Lisp_Object object)
 static dump_off
 dump_float (struct dump_context *ctx, const struct Lisp_Float *lfloat)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Float_50A7B216D9)
+# error "Lisp_Float changed. See CHECK_STRUCTS comment."
+#endif
   eassert (ctx->header.cold_start);
   struct Lisp_Float out;
   dump_object_start (ctx, &out, sizeof (out));
@@ -2233,6 +2262,9 @@ dump_float (struct dump_context *ctx, const struct Lisp_Float *lfloat)
 static dump_off
 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."
+#endif
   dump_emacs_reloc_immediate_intmax_t (ctx, intfwd->intvar, *intfwd->intvar);
   struct Lisp_Intfwd out;
   dump_object_start (ctx, &out, sizeof (out));
@@ -2244,6 +2276,9 @@ dump_fwd_int (struct dump_context *ctx, const struct Lisp_Intfwd *intfwd)
 static dump_off
 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."
+#endif
   dump_emacs_reloc_immediate_bool (ctx, boolfwd->boolvar, *boolfwd->boolvar);
   struct Lisp_Boolfwd out;
   dump_object_start (ctx, &out, sizeof (out));
@@ -2255,6 +2290,9 @@ dump_fwd_bool (struct dump_context *ctx, const struct Lisp_Boolfwd *boolfwd)
 static dump_off
 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."
+#endif
   if (NILP (Fgethash (dump_off_to_lisp (emacs_offset (objfwd->objvar)),
                       ctx->staticpro_table,
                       Qnil)))
@@ -2270,6 +2308,9 @@ static dump_off
 dump_fwd_buffer_obj (struct dump_context *ctx,
                      const struct Lisp_Buffer_Objfwd *buffer_objfwd)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Buffer_Objfwd_13CA6B04FC)
+# error "Lisp_Buffer_Objfwd changed. See CHECK_STRUCTS comment."
+#endif
   struct Lisp_Buffer_Objfwd out;
   dump_object_start (ctx, &out, sizeof (out));
   DUMP_FIELD_COPY (&out, buffer_objfwd, type);
@@ -2283,6 +2324,9 @@ static dump_off
 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."
+#endif
   struct Lisp_Kboard_Objfwd out;
   dump_object_start (ctx, &out, sizeof (out));
   DUMP_FIELD_COPY (&out, kboard_objfwd, type);
@@ -2293,6 +2337,9 @@ dump_fwd_kboard_obj (struct dump_context *ctx,
 static dump_off
 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."
+#endif
   void const *p = fwd.fwdptr;
   dump_off offset;
 
@@ -2324,6 +2371,9 @@ static dump_off
 dump_blv (struct dump_context *ctx,
           const struct Lisp_Buffer_Local_Value *blv)
 {
+#if CHECK_STRUCTS && !defined HASH_Lisp_Buffer_Local_Value_3C363FAC3C
+# error "Lisp_Buffer_Local_Value changed. See CHECK_STRUCTS comment."
+#endif
   struct Lisp_Buffer_Local_Value out;
   dump_object_start (ctx, &out, sizeof (out));
   DUMP_FIELD_COPY (&out, blv, local_if_set);
@@ -2386,6 +2436,13 @@ dump_symbol (struct dump_context *ctx,
              Lisp_Object object,
              dump_off offset)
 {
+#if CHECK_STRUCTS && !defined HASH_Lisp_Symbol_999DC26DEC
+# error "Lisp_Symbol changed. See CHECK_STRUCTS comment."
+#endif
+#if CHECK_STRUCTS && !defined (HASH_symbol_redirect_ADB4F5B113)
+# error "symbol_redirect changed. See CHECK_STRUCTS comment."
+#endif
+
   if (ctx->flags.defer_symbols)
     {
       if (offset != DUMP_OBJECT_ON_SYMBOL_QUEUE)
@@ -2475,6 +2532,9 @@ static dump_off
 dump_vectorlike_generic (struct dump_context *ctx,
 			 const union vectorlike_header *header)
 {
+#if CHECK_STRUCTS && !defined (HASH_vectorlike_header_00A5A4BFB2)
+# error "vectorlike_header changed. See CHECK_STRUCTS comment."
+#endif
   const struct Lisp_Vector *v = (const struct Lisp_Vector *) header;
   ptrdiff_t size = header->size;
   enum pvec_type pvectype = PSEUDOVECTOR_TYPE (v);
@@ -2632,6 +2692,9 @@ dump_hash_table (struct dump_context *ctx,
                  Lisp_Object object,
                  dump_off offset)
 {
+#if CHECK_STRUCTS && !defined HASH_Lisp_Hash_Table_EF95ED06FF
+# error "Lisp_Hash_Table changed. See CHECK_STRUCTS comment."
+#endif
   const struct Lisp_Hash_Table *hash_in = XHASH_TABLE (object);
   bool is_stable = dump_hash_table_stable_p (hash_in);
   /* If the hash table is likely to be modified in memory (either
@@ -2697,6 +2760,9 @@ dump_hash_table (struct dump_context *ctx,
 static dump_off
 dump_buffer (struct dump_context *ctx, const struct buffer *in_buffer)
 {
+#if CHECK_STRUCTS && !defined HASH_buffer_E34A11C6B9
+# error "buffer changed. See CHECK_STRUCTS comment."
+#endif
   struct buffer munged_buffer = *in_buffer;
   struct buffer *buffer = &munged_buffer;
 
@@ -2830,6 +2896,9 @@ dump_buffer (struct dump_context *ctx, const struct buffer *in_buffer)
 static dump_off
 dump_bool_vector (struct dump_context *ctx, const struct Lisp_Vector *v)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Vector_3091289B35)
+# error "Lisp_Vector changed. See CHECK_STRUCTS comment."
+#endif
   /* No relocation needed, so we don't need dump_object_start.  */
   dump_align_output (ctx, DUMP_ALIGNMENT);
   eassert (ctx->offset >= ctx->header.cold_start);
@@ -2844,6 +2913,9 @@ dump_bool_vector (struct dump_context *ctx, const struct Lisp_Vector *v)
 static dump_off
 dump_subr (struct dump_context *ctx, const struct Lisp_Subr *subr)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Subr_594AB72B54)
+# error "Lisp_Subr changed. See CHECK_STRUCTS comment."
+#endif
   struct Lisp_Subr out;
   dump_object_start (ctx, &out, sizeof (out));
   DUMP_FIELD_COPY (&out, subr, header.size);
@@ -2880,6 +2952,9 @@ dump_vectorlike (struct dump_context *ctx,
                  Lisp_Object lv,
                  dump_off offset)
 {
+#if CHECK_STRUCTS && !defined (HASH_pvec_type_549C833A54)
+# error "pvec_type changed. See CHECK_STRUCTS comment."
+#endif
   const struct Lisp_Vector *v = XVECTOR (lv);
   switch (PSEUDOVECTOR_TYPE (v))
     {
@@ -2987,6 +3062,9 @@ dump_vectorlike (struct dump_context *ctx,
 static dump_off
 dump_object (struct dump_context *ctx, Lisp_Object object)
 {
+#if CHECK_STRUCTS && !defined (HASH_Lisp_Type_E2AD97D3F7)
+# error "Lisp_Type changed. See CHECK_STRUCTS comment."
+#endif
 #ifdef ENABLE_CHECKING
   /* Vdead is extern only when ENABLE_CHECKING.  */
   eassert (!EQ (object, Vdead));
@@ -3089,6 +3167,9 @@ dump_object_for_offset (struct dump_context *ctx, Lisp_Object object)
 static dump_off
 dump_charset (struct dump_context *ctx, int cs_i)
 {
+#if CHECK_STRUCTS && !defined (HASH_charset_317C49E291)
+# error "charset changed. See CHECK_STRUCTS comment."
+#endif
   dump_align_output (ctx, alignof (int));
   const struct charset *cs = charset_table + cs_i;
   struct charset out;
-- 
2.17.1