Subject: Re: bug#75451: scratch/igc: Enable CHECK_STRUCTS

From: Pip Cet <[email-masked]>
Date: Fri, 10 Jan 2025 19:56:46 +0000

"Stefan Kangas" <[email-masked]> writes:

> Pip Cet <[email-masked]> writes:
>
>> This isn't strictly about the scratch/igc branch, but I personally think
>> struct hashes should be checked in all builds, mismatches should be
>> downgraded to #warnings, and --enable-checking=all could include
>> -Werror=cpp.  (So the warnings would still abort a build with
>> --enable-checking=all, but they'd *also* show up in regular builds.)
>
> As I was continuing to work on CHECK_STRUCTS, I stumbled into this and
> re-read parts of the old discussion linked below:
>
> commit 9994bf17cf532f2e1d4310341da7180342202191
> Author: Paul Eggert <[email-masked]>
> Date:   Wed Apr 10 19:42:37 2019 -0700
>
>     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.
>
> commit 44a39e3e761c0774cd1bb9360db7f49e1d66ec06
> Author: Paul Eggert <[email-masked]>
> Date:   Tue Apr 9 15:42:10 2019 -0700
>
>     Remove dmpstruct.h
>
>     The hassles of updating the dmpstruct.h-using code bit me again.
>     These updates are more trouble than they’re worth.  See:
>     https://lists.gnu.org/r/emacs-devel/2019-03/msg00122.html
>     As I’m the main person who’s made changes in this area since
>     dmpstruct.h was introduced, I’m the most motivated to clean up
>     the situation.
>
> I'm curios to know what is the experience while developing scratch/igc:
> have these checks been very helpful?

I can't really say that much about scratch/igc, but I'm modifying the
relevant structs a lot, on several branches.  I'm quite forgetful, but I
can't remember a time when the #error saved me; that's why I'd prefer a
#warning (but I understand the reluctance to downgrade errors into
warnings, thus my proposal of using -Werror=cpp).

And I did check code into scratch/igc which broke the --enable-checking
build.  A warning would most likely have prevented that.

So this is another weird case of "let's extend this, then remove the
original usage": IIUC, the reason for having pdumper include the errors
is that it was expected people would forget to adjust pdumper.c and test
only the unexec version.  Once unexec is removed, we can simply remove
the errors in pdumper.c.  scratch/igc is in a similar situation as
pdumper was when it was merged: it'll be optional for a long time, so we
need to take extra care to ensure that no one modifies the core
structures but forgets to update igc.c

(For example, right now both igc.c and alloc.c fail to scan the
"command_modes" member of struct Lisp_Subr when we're compiling without
native compilation: if someone were to use that field for a non-fixnum
Lisp object, that someone would run into weird bugs and crashes and
eventually figure out they need to fix alloc.c, but probably not igc.c,
so we'd need to know about such changes.  The hashing wouldn't
necessarily save us, but it might, because they'd probably reorder the
Lisp_Subr fields at the same time :-) ).

I can't really say whether it would make more sense for nativecomp to
simply create .eln files that are specific to one compiled Emacs rather
than trying to reuse old files if the ABI hash matches, but either that
or including the header hash would have saved me some trouble.

> Meanwhile, for enums, I'm wondering if something like this wouldn't make
> more sense?  (Or perhaps in addition to CHECK_STRUCTS.  Hmm.)

> diff --git a/src/igc.c b/src/igc.c
> index 37929765522..6c8ad55878c 100644
> --- a/src/igc.c
> +++ b/src/igc.c
> @@ -2770,6 +2776,8 @@ fix_vector (mps_ss_t ss, struct Lisp_Vector *v)
>  #endif
>  	IGC_FIX_CALL_FN (ss, struct Lisp_Vector, v, fix_vectorlike);
>  	break;
> +      default:
> +	emacs_abort ();
>        }
>    }
>    MPS_SCAN_END (ss);

Doesn't GCC have an option to turn non-exhaustive enum switches into an
error?  I concur with https://www.jwz.org/doc/java.html:

        that life-saving warning, ``enumeration value `x' not handled in
        switch''

It might be best to build Emacs with -Ddefault=nodefault.

Pip